Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] compiler_types: mark __compiletime_assert failure as
 __noreturn
To:     Nick Desaulniers <ndesaulniers@google.com>,
        Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Miguel Ojeda <ojeda@kernel.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        llvm@lists.linux.dev,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Joe Perches <joe@perches.com>
References: <20211014132331.GA4811@kernel.org>
 <YWhGQLHnA9BIVBpr@hirez.programming.kicks-ass.net>
 <CAKwvOdnkDUfRKzmLThQGW02Ew6x=KM0MQyHge7=kc673NYxo2g@mail.gmail.com>
 <CANiq72nt+8bCGAm8yhvTZfS64ovOi9_U=Gym7biUhdEsc3Neaw@mail.gmail.com>
 <CANiq72=UVCmjr1tpSwcoOkid5tWZirKCnV17_Peqy-LBmEQHEg@mail.gmail.com>
 <CAKwvOdkSFhzSwo2hfFjXXHypC8eU+VBCnRVjvNima7qx85z65Q@mail.gmail.com>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <e8d28845-3931-c122-f398-f3eaed9e659a@rasmusvillemoes.dk>
Date:   Fri, 15 Oct 2021 09:55:58 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <CAKwvOdkSFhzSwo2hfFjXXHypC8eU+VBCnRVjvNima7qx85z65Q@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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