| Message ID | 1352844821-18952-8-git-send-email-daniel.santos@pobox.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, Nov 13, 2012 at 04:13:40PM -0600, danielfsantos@att.net wrote: > Prior to the introduction of __attribute__((error("msg"))) in gcc 4.3, > creating compile-time errors required a little trickery. > BUILD_BUG{,_ON} uses this attribute when available to generate > compile-time errors, but also uses the negative-sized array trick for > older compilers, resulting in two error messages in some cases. The > reason it's "some" cases is that as of gcc 4.4, the negative-sized array > will not create an error in some situations, like inline functions. > > This patch replaces the negative-sized array code with the new > __compiletime_error_fallback() macro which expands to the same thing > unless the the error attribute is available, in which case it expands to > do{}while(0), resulting in exactly one compile-time error on all > versions of gcc. > > Note that we are not changing the negative-sized array code for the > unoptimized version of BUILD_BUG_ON, since it has the potential to catch > problems that would be disabled in later versions of gcc were > __compiletime_error_fallback used. The reason is that that an > unoptimized build can't always remove calls to an error-attributed > function call (like we are using) that should effectively become dead > code if it were optimized. However, using a negative-sized array with a > similar value will not result in an false-positive (error). The only > caveat being that it will also fail to catch valid conditions, which we > should be expecting in an unoptimized build anyway. > > Signed-off-by: Daniel Santos <daniel.santos@pobox.com> > --- > include/linux/bug.h | 2 +- > include/linux/compiler.h | 5 +++++ > 2 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/include/linux/bug.h b/include/linux/bug.h > index dd4f506..125e744 100644 > --- a/include/linux/bug.h > +++ b/include/linux/bug.h > @@ -66,7 +66,7 @@ struct pt_regs; > __compiletime_error("BUILD_BUG_ON failed"); \ > if (__cond) \ > __build_bug_on_failed(); \ > - ((void)sizeof(char[1 - 2*!!(__cond)])); \ > + __compiletime_error_fallback(__cond); \ We're passing an already evaluated __cond here which gets doubly-negated again in __compiletime_error_fallback. If __compiletime_error_fallback is going to be called only from BUILD_BUG_ON, then its definition should be: do { ((void)sizeof(char[1 - 2 * (condition)])); } while (0) i.e., without the !!. > } while(0) > #endif > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h > index cbf6d9d..8e5b9d5 100644 > --- a/include/linux/compiler.h > +++ b/include/linux/compiler.h > @@ -298,7 +298,12 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect); > #endif > #ifndef __compiletime_error > # define __compiletime_error(message) > +# define __compiletime_error_fallback(condition) \ > + do { ((void)sizeof(char[1 - 2*!!(condition)])); } while (0) > +#else > +# define __compiletime_error_fallback(condition) do { } while (0) > #endif > + > /* > * Prevent the compiler from merging or refetching accesses. The compiler > * is also forbidden from reordering successive instances of ACCESS_ONCE(), > -- > 1.7.3.4 Thanks.
On 11/15/2012 09:08 AM, Borislav Petkov wrote: > On Tue, Nov 13, 2012 at 04:13:40PM -0600, danielfsantos@att.net wrote: >> Prior to the introduction of __attribute__((error("msg"))) in gcc 4.3, >> creating compile-time errors required a little trickery. >> BUILD_BUG{,_ON} uses this attribute when available to generate >> compile-time errors, but also uses the negative-sized array trick for >> older compilers, resulting in two error messages in some cases. The >> reason it's "some" cases is that as of gcc 4.4, the negative-sized array >> will not create an error in some situations, like inline functions. >> >> This patch replaces the negative-sized array code with the new >> __compiletime_error_fallback() macro which expands to the same thing >> unless the the error attribute is available, in which case it expands to >> do{}while(0), resulting in exactly one compile-time error on all >> versions of gcc. >> >> Note that we are not changing the negative-sized array code for the >> unoptimized version of BUILD_BUG_ON, since it has the potential to catch >> problems that would be disabled in later versions of gcc were >> __compiletime_error_fallback used. The reason is that that an >> unoptimized build can't always remove calls to an error-attributed >> function call (like we are using) that should effectively become dead >> code if it were optimized. However, using a negative-sized array with a >> similar value will not result in an false-positive (error). The only >> caveat being that it will also fail to catch valid conditions, which we >> should be expecting in an unoptimized build anyway. >> >> Signed-off-by: Daniel Santos <daniel.santos@pobox.com> >> --- >> include/linux/bug.h | 2 +- >> include/linux/compiler.h | 5 +++++ >> 2 files changed, 6 insertions(+), 1 deletions(-) >> >> diff --git a/include/linux/bug.h b/include/linux/bug.h >> index dd4f506..125e744 100644 >> --- a/include/linux/bug.h >> +++ b/include/linux/bug.h >> @@ -66,7 +66,7 @@ struct pt_regs; >> __compiletime_error("BUILD_BUG_ON failed"); \ >> if (__cond) \ >> __build_bug_on_failed(); \ >> - ((void)sizeof(char[1 - 2*!!(__cond)])); \ >> + __compiletime_error_fallback(__cond); \ > We're passing an already evaluated __cond here which gets doubly-negated > again in __compiletime_error_fallback. If __compiletime_error_fallback > is going to be called only from BUILD_BUG_ON, then its definition should > be: > > do { ((void)sizeof(char[1 - 2 * (condition)])); } while (0) > > i.e., without the !!. Yeah, I agree. Also, with the complexity, I think a few more comments can be helpful in compiler.h to clarify what these macros are for more specifically. On another note, I have a "part two" set of patches for bug.h & compiler*.h that does some other stuff (more cleanup & restructuring) and this is making me think about that more. My thought about __compiletime_error_fallback is that it should be called only from within compiler.h (as in the following patch) and basically just be a private macro. However, we still use the use the negative sized array trick for the unoptimized version of BUILD_BUG_ON (which may have limited meaning), and we also use a negative bit specifier on a bitfield in BUILD_BUG_ON_ZERO and BUILD_BUG_ON_NULL (which I treat some in my other patches as well). But my thought is that it may be helpful to encapsulate these tricks into (public) macros in compiler*.h, such as "compiletime_assert_negarray" and "compiletime_assert_negbitfiled" and then have __compiletime_error_fallback expand to compiletime_assert_negarray when it's needed and no-op when it's not. This doesn't have to be decided now, but it's just a thought you gave me. And in case you're wondering about the negative bit field, BUILD_BUG_ON_{ZERO,NULL} can't call BUILD_BUG_ON in a complex expression ({ exp; exp; }) because it is evaluated outside of a function body and gcc doesn't like that. Thus, the following macro would, sadly, result in errors: #define BUILD_BUG_ON_ZERO(exp) ({BUILD_BUG_ON(exp); 0;}) However, it would not be an error to call an __alwaysinline function that uses BUILD_BUG_ON, so I still have to explore that and make sure it works in all scenarios (and compilers). But back to *this* patch, I'll make that change now. Thanks! Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 15, 2012 at 01:44:45PM -0600, Daniel Santos wrote: > Yeah, I agree. Also, with the complexity, I think a few more comments > can be helpful in compiler.h to clarify what these macros are for more > specifically. From what I've read so far the comments are fine but if you want to be more descriptive then sure, by all means. Just don't let them grow out of proportion - keep them simple and succinct. > On another note, I have a "part two" set of patches for bug.h & > compiler*.h that does some other stuff (more cleanup & restructuring) > and this is making me think about that more. My thought about > __compiletime_error_fallback is that it should be called only from > within compiler.h (as in the following patch) and basically just be a > private macro. However, we still use the use the negative sized array > trick for the unoptimized version of BUILD_BUG_ON (which may have > limited meaning), and we also use a negative bit specifier on a bitfield > in BUILD_BUG_ON_ZERO and BUILD_BUG_ON_NULL (which I treat some in my > other patches as well). But my thought is that it may be helpful to > encapsulate these tricks into (public) macros in compiler*.h, such as > "compiletime_assert_negarray" and "compiletime_assert_negbitfiled" and > then have __compiletime_error_fallback expand to > compiletime_assert_negarray when it's needed and no-op when it's not. > > This doesn't have to be decided now, but it's just a thought you gave me. I dunno. I'm thinking that maybe making them more unreadable for no apparent reason might be simply too much. They're fine the way they are now, if you ask me. > And in case you're wondering about the negative bit field, > BUILD_BUG_ON_{ZERO,NULL} can't call BUILD_BUG_ON in a complex expression > ({ exp; exp; }) because it is evaluated outside of a function body and > gcc doesn't like that. Thus, the following macro would, sadly, result > in errors: > > #define BUILD_BUG_ON_ZERO(exp) ({BUILD_BUG_ON(exp); 0;}) > > However, it would not be an error to call an __alwaysinline function > that uses BUILD_BUG_ON, so I still have to explore that and make sure it > works in all scenarios (and compilers). Ok, I see your point. Think of it this way: if unifying those macros can only happen at the expense of readability or performance then maybe it is not worth the trouble. If, OTOH, you can think of something slick and pretty, then go ahead :). HTH.
diff --git a/include/linux/bug.h b/include/linux/bug.h index dd4f506..125e744 100644 --- a/include/linux/bug.h +++ b/include/linux/bug.h @@ -66,7 +66,7 @@ struct pt_regs; __compiletime_error("BUILD_BUG_ON failed"); \ if (__cond) \ __build_bug_on_failed(); \ - ((void)sizeof(char[1 - 2*!!(__cond)])); \ + __compiletime_error_fallback(__cond); \ } while(0) #endif diff --git a/include/linux/compiler.h b/include/linux/compiler.h index cbf6d9d..8e5b9d5 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -298,7 +298,12 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect); #endif #ifndef __compiletime_error # define __compiletime_error(message) +# define __compiletime_error_fallback(condition) \ + do { ((void)sizeof(char[1 - 2*!!(condition)])); } while (0) +#else +# define __compiletime_error_fallback(condition) do { } while (0) #endif + /* * Prevent the compiler from merging or refetching accesses. The compiler * is also forbidden from reordering successive instances of ACCESS_ONCE(),
Prior to the introduction of __attribute__((error("msg"))) in gcc 4.3, creating compile-time errors required a little trickery. BUILD_BUG{,_ON} uses this attribute when available to generate compile-time errors, but also uses the negative-sized array trick for older compilers, resulting in two error messages in some cases. The reason it's "some" cases is that as of gcc 4.4, the negative-sized array will not create an error in some situations, like inline functions. This patch replaces the negative-sized array code with the new __compiletime_error_fallback() macro which expands to the same thing unless the the error attribute is available, in which case it expands to do{}while(0), resulting in exactly one compile-time error on all versions of gcc. Note that we are not changing the negative-sized array code for the unoptimized version of BUILD_BUG_ON, since it has the potential to catch problems that would be disabled in later versions of gcc were __compiletime_error_fallback used. The reason is that that an unoptimized build can't always remove calls to an error-attributed function call (like we are using) that should effectively become dead code if it were optimized. However, using a negative-sized array with a similar value will not result in an false-positive (error). The only caveat being that it will also fail to catch valid conditions, which we should be expecting in an unoptimized build anyway. Signed-off-by: Daniel Santos <daniel.santos@pobox.com> --- include/linux/bug.h | 2 +- include/linux/compiler.h | 5 +++++ 2 files changed, 6 insertions(+), 1 deletions(-)