Introduce macro static_asser_unreachable() to check that a program
point is considered unreachable by the static analysis performed by the
compiler, even at optimization level -O0.

The use of such macro will lead to one of the following outcomes:
- the program point identified by the macro is considered unreachable,
  then the compiler removes the macro;
- the program point identified by the macro is not considered
  unreachable, then the compiler does not remove the macro, which will
  lead to a failure in the build process caused by an assembler error.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/include/xen/compiler.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 16d554f2a5..ad0520f5d4 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -64,6 +64,14 @@
 # define fallthrough        do {} while (0)  /* fallthrough */
 #endif
 
+/*
+ * Add the following macro to check that a program point is considered
+ * unreachable by the static analysis performed by the compiler,
+ * even at optimization level -O0.
+ */
+#define static_assert_unreachable() \
+    asm(".error \"unreachable program point reached\"");
+
 #ifdef __clang__
 /* Clang can replace some vars with new automatic ones that go in .data;
  * mark all explicit-segment vars 'used' to prevent that. */
-- 
2.34.1

On 22.01.2024 14:48, Federico Serafini wrote:
> Introduce macro static_asser_unreachable() to check that a program
> point is considered unreachable by the static analysis performed by the
> compiler, even at optimization level -O0.

Is it really intended to limit use of this macro to cases where even
at -O0 the compiler would eliminate respective code? Note that right
now even debug builds are done with some optimization, and some of
the DCE we're relying depends on that (iirc).

> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -64,6 +64,14 @@
>  # define fallthrough        do {} while (0)  /* fallthrough */
>  #endif
>  
> +/*
> + * Add the following macro to check that a program point is considered
> + * unreachable by the static analysis performed by the compiler,
> + * even at optimization level -O0.
> + */
> +#define static_assert_unreachable() \
> +    asm(".error \"unreachable program point reached\"");

Did you check the diagnostic that results when this check actually
triggers? I expect it will be not really obvious from the message
you introduce where the issue actually is. I expect we will want
to use some of __FILE__ / __LINE__ / __FUNCTION__ to actually
supply such context.

Also: Stray semicolon and (nit) missing blanks.

Finally I wonder about case: We have ASSERT_UNREACHABLE() and it
may be indicated to use all uppercase her as well.

Jan

On 22/01/24 15:02, Jan Beulich wrote:
> On 22.01.2024 14:48, Federico Serafini wrote:
>> Introduce macro static_asser_unreachable() to check that a program
>> point is considered unreachable by the static analysis performed by the
>> compiler, even at optimization level -O0.
> 
> Is it really intended to limit use of this macro to cases where even
> at -O0 the compiler would eliminate respective code? Note that right
> now even debug builds are done with some optimization, and some of
> the DCE we're relying depends on that (iirc).

I'll remove this restriction.


> 
>> --- a/xen/include/xen/compiler.h
>> +++ b/xen/include/xen/compiler.h
>> @@ -64,6 +64,14 @@
>>   # define fallthrough        do {} while (0)  /* fallthrough */
>>   #endif
>>   
>> +/*
>> + * Add the following macro to check that a program point is considered
>> + * unreachable by the static analysis performed by the compiler,
>> + * even at optimization level -O0.
>> + */
>> +#define static_assert_unreachable() \
>> +    asm(".error \"unreachable program point reached\"");
> 
> Did you check the diagnostic that results when this check actually
> triggers? I expect it will be not really obvious from the message
> you introduce where the issue actually is. I expect we will want
> to use some of __FILE__ / __LINE__ / __FUNCTION__ to actually
> supply such context.

The assembler error comes with file and line information, for example:

./arch/x86/include/asm/uaccess.h: Assembler messages:
./arch/x86/include/asm/uaccess.h:377: Error: unreachable program point 
reached

At line 377 there is an use of get_unsafe_size() where I passed a wrong
size as argument. Is that clear enough?

What do you think about modifying the message as follows:
"unreachability static assert failed."


> 
> Also: Stray semicolon and (nit) missing blanks.

It is not clear to me where are the missing blanks.


> 
> Finally I wonder about case: We have ASSERT_UNREACHABLE() and it
> may be indicated to use all uppercase her as well.

Ok.


-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

On 24.01.2024 09:20, Federico Serafini wrote:
> On 22/01/24 15:02, Jan Beulich wrote:
>> On 22.01.2024 14:48, Federico Serafini wrote:
>>> --- a/xen/include/xen/compiler.h
>>> +++ b/xen/include/xen/compiler.h
>>> @@ -64,6 +64,14 @@
>>>   # define fallthrough        do {} while (0)  /* fallthrough */
>>>   #endif
>>>   
>>> +/*
>>> + * Add the following macro to check that a program point is considered
>>> + * unreachable by the static analysis performed by the compiler,
>>> + * even at optimization level -O0.
>>> + */
>>> +#define static_assert_unreachable() \
>>> +    asm(".error \"unreachable program point reached\"");
>>
>> Did you check the diagnostic that results when this check actually
>> triggers? I expect it will be not really obvious from the message
>> you introduce where the issue actually is. I expect we will want
>> to use some of __FILE__ / __LINE__ / __FUNCTION__ to actually
>> supply such context.
> 
> The assembler error comes with file and line information, for example:
> 
> ./arch/x86/include/asm/uaccess.h: Assembler messages:
> ./arch/x86/include/asm/uaccess.h:377: Error: unreachable program point 
> reached
> 
> At line 377 there is an use of get_unsafe_size() where I passed a wrong
> size as argument. Is that clear enough?

Hmm, yes, looks like it might be sufficient. Mentioning __FUNCTION__ may
still add value, though. But I see now that __FILE__ / __LINE__ are
already covered for.

> What do you think about modifying the message as follows:
> "unreachability static assert failed."

I'm okay-ish with the original text, and I like it slightly better than
this new suggestion. If we want "static assert" in the output, then maybe
"static assertion failed: unreachable".

>> Also: Stray semicolon and (nit) missing blanks.
> 
> It is not clear to me where are the missing blanks.

Just like for other keywords:

    asm ( ".error \"unreachable program point reached\"" )

Jan