Re: [XEN PATCH 1/3] xen: introduce static_assert_unreachable()

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

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)