| Message ID | 01c57c7e5131d699cf622be96fea7cd8e03c23f9.1705930767.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce and use static_assert_unreachable() | expand |
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.
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
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. */
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(+)