From: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
To: Nick Desaulniers <ndesaulniers@google.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, Rasmus Villemoes <linux@rasmusvillemoes.dk>,
Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn
Date: Thu, 14 Oct 2021 20:33:30 +0200 [thread overview]
Message-ID: <CANiq72nt+8bCGAm8yhvTZfS64ovOi9_U=Gym7biUhdEsc3Neaw@mail.gmail.com> (raw)
In-Reply-To: <CAKwvOdnkDUfRKzmLThQGW02Ew6x=KM0MQyHge7=kc673NYxo2g@mail.gmail.com>
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
next prev parent reply other threads:[~2021-10-14 18:33 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-14 13:23 [PATCH] compiler_types: mark __compiletime_assert failure as __noreturn Miguel Ojeda
2021-10-14 15:01 ` Peter Zijlstra
2021-10-14 17:48 ` Nick Desaulniers
2021-10-14 18:33 ` Miguel Ojeda [this message]
2021-10-14 18:41 ` Miguel Ojeda
2021-10-14 18:55 ` Nick Desaulniers
2021-10-14 19:33 ` Miguel Ojeda
2021-10-15 7:55 ` Rasmus Villemoes
2021-10-15 8:11 ` Rasmus Villemoes
2021-10-15 12:36 ` Miguel Ojeda
2021-10-14 17:26 ` Nathan Chancellor
2021-10-14 17:36 ` Nick Desaulniers
2021-10-21 23:20 ` Miguel Ojeda
2021-12-02 6:12 ` Dan Carpenter
2021-12-02 6:24 ` Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANiq72nt+8bCGAm8yhvTZfS64ovOi9_U=Gym7biUhdEsc3Neaw@mail.gmail.com' \
--to=miguel.ojeda.sandonis@gmail.com \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=llvm@lists.linux.dev \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=ojeda@kernel.org \
--cc=peterz@infradead.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).