[PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Miguel Ojeda]

`__compiletime_assert` declares a fake `extern` function
which appears (to the compiler) to be called when the test fails.

Therefore, compilers may emit possibly-uninitialized warnings
in some cases, even if it will be an error anyway (for compilers
supporting the `error` attribute, e.g. GCC and Clang >= 14)
or a link failure (for those that do not, e.g. Clang < 14).

Annotating the fake function as `__noreturn` gives them
the information they need to avoid the warning,
e.g. see https://godbolt.org/z/x1v69jjYY.

Link: https://lore.kernel.org/llvm/202110100514.3h9CI4s0-lkp@intel.com/
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
 include/linux/compiler_types.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index b6ff83a714ca..ca1a66b8cd2f 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -298,7 +298,13 @@ struct ftrace_likely_data {
 #ifdef __OPTIMIZE__
 # define __compiletime_assert(condition, msg, prefix, suffix)		\
 	do {								\
-		extern void prefix ## suffix(void) __compiletime_error(msg); \
+		/*							\
+		 * __noreturn is needed to give the compiler enough	\
+		 * information to avoid certain possibly-uninitialized	\
+		 * warnings (regardless of the build failing).		\
+		 */							\
+		__noreturn extern void prefix ## suffix(void)		\
+			__compiletime_error(msg);			\
 		if (!(condition))					\
 			prefix ## suffix();				\
 	} while (0)
-- 
2.33.1

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Peter Zijlstra]

On Thu, Oct 14, 2021 at 03:23:31PM +0200, Miguel Ojeda wrote:
> `__compiletime_assert` declares a fake `extern` function
> which appears (to the compiler) to be called when the test fails.
> 
> Therefore, compilers may emit possibly-uninitialized warnings
> in some cases, even if it will be an error anyway (for compilers
> supporting the `error` attribute, e.g. GCC and Clang >= 14)
> or a link failure (for those that do not, e.g. Clang < 14).
> 
> Annotating the fake function as `__noreturn` gives them
> the information they need to avoid the warning,
> e.g. see https://godbolt.org/z/x1v69jjYY.
> 
> Link: https://lore.kernel.org/llvm/202110100514.3h9CI4s0-lkp@intel.com/
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
>  include/linux/compiler_types.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index b6ff83a714ca..ca1a66b8cd2f 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -298,7 +298,13 @@ struct ftrace_likely_data {
>  #ifdef __OPTIMIZE__
>  # define __compiletime_assert(condition, msg, prefix, suffix)		\
>  	do {								\
> -		extern void prefix ## suffix(void) __compiletime_error(msg); \
> +		/*							\
> +		 * __noreturn is needed to give the compiler enough	\
> +		 * information to avoid certain possibly-uninitialized	\
> +		 * warnings (regardless of the build failing).		\
> +		 */							\
> +		__noreturn extern void prefix ## suffix(void)		\
> +			__compiletime_error(msg);			\
>  		if (!(condition))					\
>  			prefix ## suffix();				\
>  	} while (0)

Should we not convert this to _Static_assert, now that all supported
compilers are of recent enough vintage to support that?

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Nick Desaulniers]

On Thu, Oct 14, 2021 at 8:02 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Oct 14, 2021 at 03:23:31PM +0200, Miguel Ojeda wrote:
> > `__compiletime_assert` declares a fake `extern` function
> > which appears (to the compiler) to be called when the test fails.
> >
> > Therefore, compilers may emit possibly-uninitialized warnings
> > in some cases, even if it will be an error anyway (for compilers
> > supporting the `error` attribute, e.g. GCC and Clang >= 14)
> > or a link failure (for those that do not, e.g. Clang < 14).
> >
> > Annotating the fake function as `__noreturn` gives them
> > the information they need to avoid the warning,
> > e.g. see https://godbolt.org/z/x1v69jjYY.
> >
> > Link: https://lore.kernel.org/llvm/202110100514.3h9CI4s0-lkp@intel.com/
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > ---
> >  include/linux/compiler_types.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > index b6ff83a714ca..ca1a66b8cd2f 100644
> > --- a/include/linux/compiler_types.h
> > +++ b/include/linux/compiler_types.h
> > @@ -298,7 +298,13 @@ struct ftrace_likely_data {
> >  #ifdef __OPTIMIZE__
> >  # define __compiletime_assert(condition, msg, prefix, suffix)                \
> >       do {                                                            \
> > -             extern void prefix ## suffix(void) __compiletime_error(msg); \
> > +             /*                                                      \
> > +              * __noreturn is needed to give the compiler enough     \
> > +              * information to avoid certain possibly-uninitialized  \
> > +              * warnings (regardless of the build failing).          \
> > +              */                                                     \
> > +             __noreturn extern void prefix ## suffix(void)           \
> > +                     __compiletime_error(msg);                       \
> >               if (!(condition))                                       \
> >                       prefix ## suffix();                             \
> >       } while (0)
>
> Should we not convert this to _Static_assert, now that all supported
> compilers are of recent enough vintage to support that?

It's a good question; I'm pretty sure we had a thread with Rasmus on
the idea a while ago, and IIRC the answer is no.

Basically, we can't convert BUILD_BUG_ON to _Static_assert because
_Static_assert requires integer constant expressions (ICE) while many
expressions passed to BUILD_BUG_ON in the kernel require that
optimizations such as inlining run (they are not ICEs); BUILD_BUG_ON
is more flexible.  So you can't replace the guts of BUILD_BUG_ON
wholesale with _Static_assert (without doing anything else); it would
be preferable for kernel developers to use _Static_assert (I think we
have a macro, static_assert, too) in cases where they have ICEs rather
than BUILD_BUG_ON (though it flips the condition of the expression;
_Static_assert errors if the expression evaluates to false;
BUILD_BUG_ON when true), but I think there's too much muscle memory
around just using BUILD_BUG_ON that if you introduced something new,
folks wouldn't know to use that instead.

Probably a better demonstration would be to try it and observe some of
the spooky failures at build time that result.  We may be able to
separate the macro into two; BUILD_BUG_ON and BUILD_BUG_ON_OPT (or
whatever color bikeshed), where the former uses _Static_assert under
the hood, and the latter uses __attribute__((error)). Then we could go
about converting cases that could not use _Static_assert to use the
new macro, while the old macro is what folks still reach for first.

I'm not sure how worthwhile that yakshave would be, but at least the
front end of the compiler would error sooner in the case of
_Static_assert, FWIW (not much).  But I don't think we can ever
eliminate __attribute__((error)) from the kernel unless we're ok
outright removing asserts that aren't ICEs.  I would not recommend
that.  I would like to see more usage of static_assert, but I'm not
sure how best to promote that, and if it's worth discussing the subtle
distinction between BUILD_BUG_ON vs _Static_assert again and again and
again every time.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Miguel Ojeda]

On Thu, Oct 14, 2021 at 7:49 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> It's a good question; I'm pretty sure we had a thread with Rasmus on
> the idea a while ago, and IIRC the answer is no.

Yeah, I remember that too.

> Basically, we can't convert BUILD_BUG_ON to _Static_assert because
> _Static_assert requires integer constant expressions (ICE) while many
> expressions passed to BUILD_BUG_ON in the kernel require that
> optimizations such as inlining run (they are not ICEs); BUILD_BUG_ON
> is more flexible.  So you can't replace the guts of BUILD_BUG_ON
> wholesale with _Static_assert (without doing anything else); it would
> be preferable for kernel developers to use _Static_assert (I think we
> have a macro, static_assert, too) in cases where they have ICEs rather
> than BUILD_BUG_ON (though it flips the condition of the expression;
> _Static_assert errors if the expression evaluates to false;
> BUILD_BUG_ON when true), but I think there's too much muscle memory
> around just using BUILD_BUG_ON that if you introduced something new,
> folks wouldn't know to use that instead.

Indeed, `BUILD_BUG_ON` requires the optimizer to see through whatever
you are trying to do. Way more powerful, but finicky too.

Another difference is that `_Static_assert` can be used in more places
(file scope, inside `struct`s...) for tests about e.g. sizes, i.e.
`BUILD_BUG_ON` is not a complete replacement either.

> Probably a better demonstration would be to try it and observe some of
> the spooky failures at build time that result.  We may be able to
> separate the macro into two; BUILD_BUG_ON and BUILD_BUG_ON_OPT (or
> whatever color bikeshed), where the former uses _Static_assert under
> the hood, and the latter uses __attribute__((error)). Then we could go
> about converting cases that could not use _Static_assert to use the
> new macro, while the old macro is what folks still reach for first.

That would be a nice to do, but I am not sure about introducing one
more macro about this... I think it would be simpler to submit patches
for moves into `static_assert` even if we have to "flip" the meaning.

> I'm not sure how worthwhile that yakshave would be, but at least the
> front end of the compiler would error sooner in the case of
> _Static_assert, FWIW (not much).  But I don't think we can ever
> eliminate __attribute__((error)) from the kernel unless we're ok
> outright removing asserts that aren't ICEs.  I would not recommend
> that.  I would like to see more usage of static_assert, but I'm not
> sure how best to promote that, and if it's worth discussing the subtle
> distinction between BUILD_BUG_ON vs _Static_assert again and again and
> again every time.

Perhaps we should add a comment in `BUILD_BUG*` about checking out
`static_assert` -- we have the comment in the latter, but those
reading the former will not realize the may be able to use the
latter...

Cheers,
Miguel

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Miguel Ojeda]

On Thu, Oct 14, 2021 at 8:33 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> That would be a nice to do, but I am not sure about introducing one
> more macro about this... I think it would be simpler to submit patches
> for moves into `static_assert` even if we have to "flip" the meaning.

Actually, what would be ideal is a compiler-backed lint that checks
whether it could be an `static_assert`, perhaps in clang-tidy?

It would also ensure things are kept clean.

Cheers,
Miguel

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Nick Desaulniers]

On Thu, Oct 14, 2021 at 11:41 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Thu, Oct 14, 2021 at 8:33 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > That would be a nice to do, but I am not sure about introducing one
> > more macro about this... I think it would be simpler to submit patches
> > for moves into `static_assert` even if we have to "flip" the meaning.

$ grep -r BUILD_BUG_ON | wc -l
3405

> Actually, what would be ideal is a compiler-backed lint that checks
> whether it could be an `static_assert`, perhaps in clang-tidy?

Oh, that is a good idea.  There is one already for recommending the
use of static_assert instead of assert.  That's actually very nice.

I was playing with trying to adapt clang-tidy's C++11 `auto` fixit to
work on GNU C code to automate the replacement of:

__typeof(x) y = (x);

with:

__auto_type y = (x);

in macros.  That's perhaps interesting, too.  Given the volume of code
in the kernel, I wouldn't waste time with one off patches; rather I'd
work on automation (since clang-tidy can be taught "fixits" to fix the
code in place, not just warn) so that we can better enable treewide
changes AND keep new instances from sneaking back in easier.

Let's see if I get an intern in 2022, maybe I can have them focus on
clang-tidy+kernel.

>
> It would also ensure things are kept clean.
>
> Cheers,
> Miguel



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Miguel Ojeda]

On Thu, Oct 14, 2021 at 8:55 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> $ grep -r BUILD_BUG_ON | wc -l
> 3405

Definitely -- I am assuming a significant part of the macro
invocations cannot be static asserts so that we would ended up with
churn anyway...

> Oh, that is a good idea.  There is one already for recommending the
> use of static_assert instead of assert.  That's actually very nice.

Happy to help!

> in the kernel, I wouldn't waste time with one off patches; rather I'd
> work on automation (since clang-tidy can be taught "fixits" to fix the
> code in place, not just warn) so that we can better enable treewide
> changes AND keep new instances from sneaking back in easier.

For automatic fixing we would need to be a bit smart due to the
negation w.r.t. "style", i.e. in most cases it should be easy to apply
it to the expression, e.g. from `!(x == 42)` to `x != 42`, but in
others it may require some "human touch".

There is also the possible mismatch of the usual style rules even if
we apply e.g. `clang-format` after it (one more reason to avoid human
formatting...).

But yeah, I think we should be able to cover the vast majority of them
easily. We can always send them as RFC patches and let folks adapt the
patch, then enable the warning/error by default after one release or
two.

> Let's see if I get an intern in 2022, maybe I can have them focus on
> clang-tidy+kernel.

It would be great to have someone adding more `linuxkernel-` checks!

Cheers,
Miguel

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Rasmus Villemoes]

On 14/10/2021 20.55, Nick Desaulniers wrote:
> On Thu, Oct 14, 2021 at 11:41 AM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> On Thu, Oct 14, 2021 at 8:33 PM Miguel Ojeda
>> <miguel.ojeda.sandonis@gmail.com> wrote:
>>>
>>> That would be a nice to do, but I am not sure about introducing one
>>> more macro about this... I think it would be simpler to submit patches
>>> for moves into `static_assert` even if we have to "flip" the meaning.
> 
> $ grep -r BUILD_BUG_ON | wc -l
> 3405
> 
>> Actually, what would be ideal is a compiler-backed lint that checks
>> whether it could be an `static_assert`, perhaps in clang-tidy?
> 
> Oh, that is a good idea.  There is one already for recommending the
> use of static_assert instead of assert.  That's actually very nice.

So I did

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index b6ff83a714ca..e212220216e8 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -295,12 +295,17 @@ struct ftrace_likely_data {
 # define __compiletime_object_size(obj) -1
 #endif

+#include <linux/const.h>
+
 #ifdef __OPTIMIZE__
 # define __compiletime_assert(condition, msg, prefix, suffix)          \
        do {                                                            \
                extern void prefix ## suffix(void)
__compiletime_error(msg); \
+               extern void prefix ## suffix ## ice(void)
__compiletime_warning("Ice ice baby"); \
                if (!(condition))                                       \
                        prefix ## suffix();                             \
+               if (__is_constexpr(condition))                          \
+                       prefix ## suffix ## ice();                      \
        } while (0)
 #else
 # define __compiletime_assert(condition, msg, prefix, suffix) do { }
while (0)

and that throws a gazillion warnings. Picking one at random shows that
container_of() has a BUILD_BUG_ON. So we could do

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2776423a587e..0a1969b11619 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -492,9 +492,9 @@ static inline void ftrace_dump(enum ftrace_dump_mode
oops_dump_mode) { }
  */
 #define container_of(ptr, type, member) ({                             \
        void *__mptr = (void *)(ptr);                                   \
-       BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
-                        !__same_type(*(ptr), void),                    \
-                        "pointer type mismatch in container_of()");    \
+       static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
+                     __same_type(*(ptr), void),                        \
+                     "pointer type mismatch in container_of()");       \
        ((type *)(__mptr - offsetof(type, member))); })

 /**
@@ -507,9 +507,9 @@ static inline void ftrace_dump(enum ftrace_dump_mode
oops_dump_mode) { }
  */
 #define container_of_safe(ptr, type, member) ({
        \
        void *__mptr = (void *)(ptr);                                   \
-       BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) &&   \
-                        !__same_type(*(ptr), void),                    \
-                        "pointer type mismatch in container_of()");    \
+       static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
+                     __same_type(*(ptr), void),                        \
+                     "pointer type mismatch in container_of_safe()");  \
        IS_ERR_OR_NULL(__mptr) ? ERR_CAST(__mptr) :                     \
                ((type *)(__mptr - offsetof(type, member))); })

[fixing the copy-pasto in container_of_safe while at it].

Basically, all BUILD_BUG_ONs like that that do type checking are very
obviously candidates for using static_assert instead (they very
naturally go at the end of all the locally declared variables, so one
won't hit the declaration-after-statement problem that can otherwise
prevent using static_assert).

> I was playing with trying to adapt clang-tidy's C++11 `auto` fixit to
> work on GNU C code to automate the replacement of:
> 
> __typeof(x) y = (x);
> 
> with:
> 
> __auto_type y = (x);
> 
> in macros.  That's perhaps interesting, too.  Given the volume of code
> in the kernel, I wouldn't waste time with one off patches;

Well, for the kind of macros that are used _everywhere_ a few one-off
patches might be in order. It's also interesting if one could measure
any speedup from switching those core macros to static_assert.

Rasmus

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Rasmus Villemoes]

On 14/10/2021 19.48, Nick Desaulniers wrote:
> On Thu, Oct 14, 2021 at 8:02 AM Peter Zijlstra <peterz@infradead.org> wrote:
>>

> I'm not sure how worthwhile that yakshave would be, 

A yakshave that would be worthwhile is to kill off the macro
compiletime_assert() completely - three is a crowd. It sounds like it
would be implemented in terms of _Static_assert, but it's actually
__attribute__(error). We can fold the definition of compiletime_assert
into BUILD_BUG_ON_MSG.

The users in rwonce.h should just be changed to static_assert, and then
there are very few random users left, which can either be static_assert
or BUILD_BUG_ON_MSG.

Why do we even have a no-op version if !__OPTIMIZE__? AFAIK there's no
CONFIG_O0 option, and such a build wouldn't be interesting at all - it
can't be expected to boot, and it would likely throw warnings left and
right.

Rasmus

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Miguel Ojeda]

On Fri, Oct 15, 2021 at 10:11 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> A yakshave that would be worthwhile is to kill off the macro
> compiletime_assert() completely - three is a crowd. It sounds like it
> would be implemented in terms of _Static_assert, but it's actually
> __attribute__(error). We can fold the definition of compiletime_assert
> into BUILD_BUG_ON_MSG.

Agreed, two should be enough.

> Why do we even have a no-op version if !__OPTIMIZE__? AFAIK there's no
> CONFIG_O0 option, and such a build wouldn't be interesting at all - it
> can't be expected to boot, and it would likely throw warnings left and
> right.

Yeah, I don't think it would compile as it is anyway.

Perhaps it is there for some kind of tooling? For a static analyzer or
something like sparse (if it didn't have its own define)...

But yeah, every use of it should have a comment explaining why it is
there, like crypto/jitterentropy.c does. There are a couple without
it.

Cheers,
Miguel

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Nathan Chancellor]

On Thu, Oct 14, 2021 at 03:23:31PM +0200, Miguel Ojeda wrote:
> `__compiletime_assert` declares a fake `extern` function
> which appears (to the compiler) to be called when the test fails.
> 
> Therefore, compilers may emit possibly-uninitialized warnings
> in some cases, even if it will be an error anyway (for compilers
> supporting the `error` attribute, e.g. GCC and Clang >= 14)
> or a link failure (for those that do not, e.g. Clang < 14).
> 
> Annotating the fake function as `__noreturn` gives them
> the information they need to avoid the warning,
> e.g. see https://godbolt.org/z/x1v69jjYY.
> 
> Link: https://lore.kernel.org/llvm/202110100514.3h9CI4s0-lkp@intel.com/
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  include/linux/compiler_types.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index b6ff83a714ca..ca1a66b8cd2f 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -298,7 +298,13 @@ struct ftrace_likely_data {
>  #ifdef __OPTIMIZE__
>  # define __compiletime_assert(condition, msg, prefix, suffix)		\
>  	do {								\
> -		extern void prefix ## suffix(void) __compiletime_error(msg); \
> +		/*							\
> +		 * __noreturn is needed to give the compiler enough	\
> +		 * information to avoid certain possibly-uninitialized	\
> +		 * warnings (regardless of the build failing).		\
> +		 */							\
> +		__noreturn extern void prefix ## suffix(void)		\
> +			__compiletime_error(msg);			\
>  		if (!(condition))					\
>  			prefix ## suffix();				\
>  	} while (0)
> -- 
> 2.33.1
> 

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Nick Desaulniers]

On Thu, Oct 14, 2021 at 6:25 AM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> `__compiletime_assert` declares a fake `extern` function
> which appears (to the compiler) to be called when the test fails.
>
> Therefore, compilers may emit possibly-uninitialized warnings
> in some cases, even if it will be an error anyway (for compilers
> supporting the `error` attribute, e.g. GCC and Clang >= 14)
> or a link failure (for those that do not, e.g. Clang < 14).
>
> Annotating the fake function as `__noreturn` gives them
> the information they need to avoid the warning,
> e.g. see https://godbolt.org/z/x1v69jjYY.

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Link: https://lore.kernel.org/llvm/202110100514.3h9CI4s0-lkp@intel.com/
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
>  include/linux/compiler_types.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index b6ff83a714ca..ca1a66b8cd2f 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -298,7 +298,13 @@ struct ftrace_likely_data {
>  #ifdef __OPTIMIZE__
>  # define __compiletime_assert(condition, msg, prefix, suffix)          \
>         do {                                                            \
> -               extern void prefix ## suffix(void) __compiletime_error(msg); \
> +               /*                                                      \
> +                * __noreturn is needed to give the compiler enough     \
> +                * information to avoid certain possibly-uninitialized  \
> +                * warnings (regardless of the build failing).          \
> +                */                                                     \
> +               __noreturn extern void prefix ## suffix(void)           \
> +                       __compiletime_error(msg);                       \
>                 if (!(condition))                                       \
>                         prefix ## suffix();                             \
>         } while (0)
> --
> 2.33.1
>
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Miguel Ojeda]

On Thu, Oct 14, 2021 at 3:25 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> `__compiletime_assert` declares a fake `extern` function
> which appears (to the compiler) to be called when the test fails.

Queuing this one through my tree.

Cheers,
Miguel

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Dan Carpenter]

On Thu, Oct 14, 2021 at 03:23:31PM +0200, Miguel Ojeda wrote:
> `__compiletime_assert` declares a fake `extern` function
> which appears (to the compiler) to be called when the test fails.
> 
> Therefore, compilers may emit possibly-uninitialized warnings
> in some cases, even if it will be an error anyway (for compilers
> supporting the `error` attribute, e.g. GCC and Clang >= 14)
> or a link failure (for those that do not, e.g. Clang < 14).
> 
> Annotating the fake function as `__noreturn` gives them
> the information they need to avoid the warning,
> e.g. see https://godbolt.org/z/x1v69jjYY.
> 
> Link: https://lore.kernel.org/llvm/202110100514.3h9CI4s0-lkp@intel.com/
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

This patch is kind of a headache for Smatch.

The patch basically turns BUILD_BUG() into BUG().  That's fine.

But it also turn BUILD_BUG_ON(!__builtin_constant_p(foo) into a BUG_ON().
A lot of places call BUILD_BUG_ON() to ensure that functions are inlined
and the build breaks if not.  For example, the FIELD_GET() macro will
trigger a build bug if "mask" is not a compile time constant.

   295  static __always_inline u32 cal_read_field(struct cal_dev *cal, u32 offset, u32 mask)
   296  {
   297          return FIELD_GET(mask, cal_read(cal, offset));
   298  }

Unfortunately, Smatch doesn't respect the __always_inline annotation so
now it thinks cal_read_field calls BUG() and records it as a noreturn
function.  And all the callers get marked as noreturn functions as well.

Could we make it so that BUILD_BUG() is a noreturn function and
BUILD_BUG_ON() is left as it was?

regards,
dan carpenter

Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn

[Dan Carpenter]

Never mind.  I can just add some code to Smatch to ignore compile time
asserts.

regards,
dan carpenter