| Message ID | 20241114101402.156397-2-philipp.reisner@linbit.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add missing include in compiler.h | expand |
On Thu, Nov 14, 2024 at 11:14:02AM +0100, Philipp Reisner wrote: > compiler.h defines __must_be_array() and __must_be_cstr() and both > expand to BUILD_BUG_ON_ZERO(). build_bug.h defines BUILD_BUG_ON_ZERO(). > So far compiler.h lacks to include build_bug.h. > > Fix compiler.h by including build_bug.h. With that compiler.h and > build_bug.h depend on each other. > > Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com> > --- Fixes: ec0bbef66f86 ("Compiler Attributes: homogenize __must_be_array") What actually breaks? This commit is six years old. It's weird that we're only seeing build breakage now. Or did you just notice this while reviewing the code? regards, dan carpenter > include/linux/compiler.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index 4d4e23b6e3e7..2d75e4892316 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -3,6 +3,7 @@ > #define __LINUX_COMPILER_H > > #include <linux/compiler_types.h> > +#include <linux/build_bug.h> /* for BUILD_BUG_ON_ZERO() */ > > #ifndef __ASSEMBLY__ > > -- > 2.47.0 >
On Thu, Nov 14, 2024 at 11:40 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Thu, Nov 14, 2024 at 11:14:02AM +0100, Philipp Reisner wrote: > > compiler.h defines __must_be_array() and __must_be_cstr() and both > > expand to BUILD_BUG_ON_ZERO(). build_bug.h defines BUILD_BUG_ON_ZERO(). > > So far compiler.h lacks to include build_bug.h. > > > > Fix compiler.h by including build_bug.h. With that compiler.h and > > build_bug.h depend on each other. > > > > Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com> > > --- > > Fixes: ec0bbef66f86 ("Compiler Attributes: homogenize __must_be_array") > > What actually breaks? This commit is six years old. It's weird that we're only > seeing build breakage now. Or did you just notice this while reviewing the > code? > I am working on a compilation unit that includes linux/string.h. Compiling it breaks when using strscp(). That is since commit commit 559048d156ff3391c4b793779a824c9193e20442 Author: Kees Cook <kees@kernel.org> Date: Mon Aug 5 14:43:44 2024 -0700 Of course, my trivial workaround is including build_bug.h in my source; it is just not the proper way to fix this. best regards, Phil
On Thu, Nov 14, 2024 at 12:01:42PM +0100, Philipp Reisner wrote: > On Thu, Nov 14, 2024 at 11:40 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > On Thu, Nov 14, 2024 at 11:14:02AM +0100, Philipp Reisner wrote: > > > compiler.h defines __must_be_array() and __must_be_cstr() and both > > > expand to BUILD_BUG_ON_ZERO(). build_bug.h defines BUILD_BUG_ON_ZERO(). > > > So far compiler.h lacks to include build_bug.h. > > > > > > Fix compiler.h by including build_bug.h. With that compiler.h and > > > build_bug.h depend on each other. > > > > > > Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com> > > > --- > > > > Fixes: ec0bbef66f86 ("Compiler Attributes: homogenize __must_be_array") > > > > What actually breaks? This commit is six years old. It's weird that we're only > > seeing build breakage now. Or did you just notice this while reviewing the > > code? > > > > I am working on a compilation unit that includes linux/string.h. > Compiling it breaks when using strscp(). That is since commit > commit 559048d156ff3391c4b793779a824c9193e20442 > Author: Kees Cook <kees@kernel.org> > Date: Mon Aug 5 14:43:44 2024 -0700 > > Of course, my trivial workaround is including build_bug.h in my > source; it is just not the proper way to fix this. > Ah, okay. Thanks. I thought it might have broken in tree code. regards, dan carpenter
On Thu, 14 Nov 2024 at 02:23, Philipp Reisner <philipp.reisner@linbit.com> wrote: > > compiler.h defines __must_be_array() and __must_be_cstr() and both > expand to BUILD_BUG_ON_ZERO(). build_bug.h defines BUILD_BUG_ON_ZERO(). > So far compiler.h lacks to include build_bug.h. The bug is real, but.. > Fix compiler.h by including build_bug.h. With that compiler.h and > build_bug.h depend on each other. I hate how compiler.h would include build_bug.h, which - on the very first line - then in turn includes compiler.h. Does it *work*? Yes. The standard include guards stop the thing from recursing, and both headers only do create pre-processor defines, so the dependencies aren't ordered, but it's really really ugly to have these kinds of circular includes. I think a better fix would be to not use BUILD_BUG_ON_ZERO() at all, but just use _Static_assert() directly here, to make <linux/compiler.h> be more self-sufficient. The gcc docs say that __builtin_types_compatible_p() and __builtin_has_attribute() both return an integer constant expression, so that should be fine (the advantage of BUILD_BUG_ON_ZERO() is that it works in some contexts that aren't necessarily technically integer constant expressions - as long as they just evaluate to a constant). We historically used to avoid _Static_assert(), but that was for historical reasons (ie it's one of those things that didn't exist back in the day..) Hmm? Linus
On Thu, 14 Nov 2024 at 09:42, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > I think a better fix would be to not use BUILD_BUG_ON_ZERO() at all, > but just use _Static_assert() directly here, to make > <linux/compiler.h> be more self-sufficient. Damn. We can't do that, because we typically use this in contexts that require us to return zero (because the assertion part of an expression), and then that whole expression needs to be a constant integer expression. And because _Static_assert() isn't an expression, we'd need to wrap it with a statement expression or something. And *hat* we can't do in arbitrary contexts. Grr. I absolutely detest how bad the standard tools are. It's kind of sad how we need to build our own hacky BUILD_BUG_ON() to do this. There's probably some trick I'm missing, but yeah, it looks like we need our BUILD_BUG_ON_ZERO() thing with that crazy bitfield hack. Linus
On Thu, 14 Nov 2024 at 10:28, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > There's probably some trick I'm missing, but yeah, it looks like we > need our BUILD_BUG_ON_ZERO() thing with that crazy bitfield hack. .. and right after sending that, I figured out the trick. You can use 'static_assert()' inside a type definition. So the way to turn it into an expression is to just use the same 'sizeof(empty struct)' trick that we use for BUILD_BUG_ON_ZERO: #define Static_assert(a,msg) \ sizeof(struct{_Static_assert(a,msg);}) works as a way to make _Static_assert() be an expression. What a horrid hack. I don't know if this is worth it, but it does at least have the advantage of having a message, so that the error case can explain itself rather than get that odd "negative width in bit-field" error message. I dunno. Linus
diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 4d4e23b6e3e7..2d75e4892316 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -3,6 +3,7 @@ #define __LINUX_COMPILER_H #include <linux/compiler_types.h> +#include <linux/build_bug.h> /* for BUILD_BUG_ON_ZERO() */ #ifndef __ASSEMBLY__
compiler.h defines __must_be_array() and __must_be_cstr() and both expand to BUILD_BUG_ON_ZERO(). build_bug.h defines BUILD_BUG_ON_ZERO(). So far compiler.h lacks to include build_bug.h. Fix compiler.h by including build_bug.h. With that compiler.h and build_bug.h depend on each other. Signed-off-by: Philipp Reisner <philipp.reisner@linbit.com> --- include/linux/compiler.h | 1 + 1 file changed, 1 insertion(+)