From: Jan Beulich <hidden> Date: 2009-09-29 09:28:38
quoted
quoted
Hollis Blanchard 09/29/09 2:00 AM >>>
First, I think there is a real bug here, and the code should read like
this (to match the comment):
/* type has to be known at build time for optimization */
- BUILD_BUG_ON(__builtin_constant_p(type));
+ BUILD_BUG_ON(!__builtin_constant_p(type));
However, I get the same build error *both* ways, i.e.
__builtin_constant_p(type) evaluates to both 0 and 1? Either that, or
the new BUILD_BUG_ON() macro isn't working...
No, at this point of the compilation process it's neither zero nor one,
it's simply considered non-constant by the compiler at that stage
(this builtin is used for optimization, not during parsing, and the
error gets generated when the body of the function gets parsed,
not when code gets generated from it).
Jan
On Tue, Sep 29, 2009 at 11:28 AM, Jan Beulich [off-list ref] wrote:
quoted
quoted
quoted
Hollis Blanchard =A009/29/09 2:00 AM >>>
First, I think there is a real bug here, and the code should read like
this (to match the comment):
=A0 =A0/* type has to be known at build time for optimization */
- =A0 =A0BUILD_BUG_ON(__builtin_constant_p(type));
+ =A0 =A0BUILD_BUG_ON(!__builtin_constant_p(type));
However, I get the same build error *both* ways, i.e.
__builtin_constant_p(type) evaluates to both 0 and 1? Either that, or
the new BUILD_BUG_ON() macro isn't working...
No, at this point of the compilation process it's neither zero nor one,
it's simply considered non-constant by the compiler at that stage
(this builtin is used for optimization, not during parsing, and the
error gets generated when the body of the function gets parsed,
not when code gets generated from it).
Jan
then maybe
if(__builtin_constant_p(type))
BUILD_BUG_ON(1);
would work?
Roel
On Tue, 2009-09-29 at 10:28 +0100, Jan Beulich wrote:
quoted
quoted
quoted
Hollis Blanchard 09/29/09 2:00 AM >>>
First, I think there is a real bug here, and the code should read like
this (to match the comment):
/* type has to be known at build time for optimization */
- BUILD_BUG_ON(__builtin_constant_p(type));
+ BUILD_BUG_ON(!__builtin_constant_p(type));
However, I get the same build error *both* ways, i.e.
__builtin_constant_p(type) evaluates to both 0 and 1? Either that, or
the new BUILD_BUG_ON() macro isn't working...
No, at this point of the compilation process it's neither zero nor one,
it's simply considered non-constant by the compiler at that stage
(this builtin is used for optimization, not during parsing, and the
error gets generated when the body of the function gets parsed,
not when code gets generated from it).
I think I see what you're saying. Do you have a fix to suggest?
--
Hollis Blanchard
IBM Linux Technology Center
From: Jan Beulich <hidden> Date: 2009-09-30 06:29:40
quoted
quoted
roel kluin [off-list ref] 29.09.09 11:51 >>>
On Tue, Sep 29, 2009 at 11:28 AM, Jan Beulich [off-list ref] =
wrote:
quoted
quoted
quoted
quoted
Hollis Blanchard 09/29/09 2:00 AM >>>
First, I think there is a real bug here, and the code should read like
this (to match the comment):
/* type has to be known at build time for optimization */
- BUILD_BUG_ON(__builtin_constant_p(type));
+ BUILD_BUG_ON(!__builtin_constant_p(type));
However, I get the same build error *both* ways, i.e.
__builtin_constant_p(type) evaluates to both 0 and 1? Either that, or
the new BUILD_BUG_ON() macro isn't working...
No, at this point of the compilation process it's neither zero nor one,
it's simply considered non-constant by the compiler at that stage
(this builtin is used for optimization, not during parsing, and the
error gets generated when the body of the function gets parsed,
not when code gets generated from it).
Jan
then maybe
if(__builtin_constant_p(type))
BUILD_BUG_ON(1);
would work?
Definitely not - this would result in the compiler *always* generating an
error.
Jan
On Tue, 2009-09-29 at 10:28 +0100, Jan Beulich wrote:
quoted
quoted
quoted
quoted
Hollis Blanchard 09/29/09 2:00 AM >>>
First, I think there is a real bug here, and the code should read like
this (to match the comment):
/* type has to be known at build time for optimization */
- BUILD_BUG_ON(__builtin_constant_p(type));
+ BUILD_BUG_ON(!__builtin_constant_p(type));
However, I get the same build error *both* ways, i.e.
__builtin_constant_p(type) evaluates to both 0 and 1? Either that, or
the new BUILD_BUG_ON() macro isn't working...
=20
No, at this point of the compilation process it's neither zero nor one,
it's simply considered non-constant by the compiler at that stage
(this builtin is used for optimization, not during parsing, and the
error gets generated when the body of the function gets parsed,
not when code gets generated from it).
I think I see what you're saying. Do you have a fix to suggest?
The one Rusty suggested the other day may help here. I don't like it
as a drop-in replacement for BUILD_BUG_ON() though (due to it
deferring the error generated to the linking stage), I'd rather view
this as an improvement to MAYBE_BUILD_BUG_ON() (which should
then be used here).
Jan
On Tue, 2009-09-29 at 10:28 +0100, Jan Beulich wrote:
quoted
quoted
quoted
quoted
Hollis Blanchard 09/29/09 2:00 AM >>>
First, I think there is a real bug here, and the code should read like
this (to match the comment):
/* type has to be known at build time for optimization */
- BUILD_BUG_ON(__builtin_constant_p(type));
+ BUILD_BUG_ON(!__builtin_constant_p(type));
However, I get the same build error *both* ways, i.e.
__builtin_constant_p(type) evaluates to both 0 and 1? Either that, or
the new BUILD_BUG_ON() macro isn't working...
No, at this point of the compilation process it's neither zero nor one,
it's simply considered non-constant by the compiler at that stage
(this builtin is used for optimization, not during parsing, and the
error gets generated when the body of the function gets parsed,
not when code gets generated from it).
I think I see what you're saying. Do you have a fix to suggest?
The one Rusty suggested the other day may help here. I don't like it
as a drop-in replacement for BUILD_BUG_ON() though (due to it
deferring the error generated to the linking stage), I'd rather view
this as an improvement to MAYBE_BUILD_BUG_ON() (which should
then be used here).
Can you be more specific?
I have no idea what Rusty suggested where. I can't even guess what
MAYBE_BUILD_BUG_ON() is supposed to do (sounds like a terrible name).
All I know is that this used to build...
--
Hollis Blanchard
IBM Linux Technology Center
On Wed, 2009-09-30 at 07:35 +0100, Jan Beulich wrote:
quoted
The one Rusty suggested the other day may help here. I don't like it
as a drop-in replacement for BUILD_BUG_ON() though (due to it
deferring the error generated to the linking stage), I'd rather view
this as an improvement to MAYBE_BUILD_BUG_ON() (which should
then be used here).
Can you be more specific?
I have no idea what Rusty suggested where. I can't even guess what
I'm attaching Rusty's response I was referring to.
MAYBE_BUILD_BUG_ON() is supposed to do (sounds like a terrible name).
Agreed - but presumably better than just deleting the bogus instances
altogether...
Jan
Rusty's version of BUILD_BUG_ON() does indeed fix the build break, and
also exposes the bug in kvmppc_account_exit_stat(). So to recap:
original: built but didn't work
Jan's: doesn't build
Rusty's: builds and works
Where do you want to go from here?
--
Hollis Blanchard
IBM Linux Technology Center
On Mon, 2009-10-05 at 07:58 +0100, Jan Beulich wrote:
On Wed, 2009-09-30 at 07:35 +0100, Jan Beulich wrote:
quoted
The one Rusty suggested the other day may help here. I don't like it
as a drop-in replacement for BUILD_BUG_ON() though (due to it
deferring the error generated to the linking stage), I'd rather view
this as an improvement to MAYBE_BUILD_BUG_ON() (which should
then be used here).
Can you be more specific?
I have no idea what Rusty suggested where. I can't even guess what
I'm attaching Rusty's response I was referring to.
quoted
MAYBE_BUILD_BUG_ON() is supposed to do (sounds like a terrible name).
Agreed - but presumably better than just deleting the bogus instances
altogether...
Jan
email message attachment
quoted
-------- Forwarded Message --------
From: Rusty Russell <redacted>
To: Jan Beulich <redacted>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fix BUILD_BUG_ON() and a couple of bogus uses
of it
Date: Wed, 23 Sep 2009 10:27:00 +0930
On Wed, 19 Aug 2009 01:29:25 am Jan Beulich wrote:
quoted
gcc permitting variable length arrays makes the current construct
used for BUILD_BUG_ON() useless, as that doesn't produce any diagnostic
if the controlling expression isn't really constant. Instead, this
patch makes it so that a bit field gets used here. Consequently, those
uses where the condition isn't really constant now also need fixing.
Note that in the gfp.h, kmemcheck.h, and virtio_config.h cases
MAYBE_BUILD_BUG_ON() really just serves documentation purposes - even
if the expression is compile time constant (__builtin_constant_p()
yields true), the array is still deemed of variable length by gcc, and
hence the whole expression doesn't have the intended effect.
Signed-off-by: Jan Beulich <redacted>
We used to use an undefined symbol here; diagnostics are worse but it catches
more stuff.
Perhaps a hybrid is the way to go?
#ifndef __OPTIMIZE__
#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
#else
/* If it's a constant, catch it at compile time, otherwise at link time. */
extern int __build_bug_on_failed;
#define BUILD_BUG_ON(condition) \
do { \
((void)sizeof(char[1 - 2*!!(condition)])); \
if (condition) __build_bug_on_failed = 1; \
} while(0)
#endif
Thanks,
Rusty.
On Fri, 2009-10-09 at 12:14 -0700, Hollis Blanchard wrote:
Rusty's version of BUILD_BUG_ON() does indeed fix the build break, and
also exposes the bug in kvmppc_account_exit_stat(). So to recap:
original: built but didn't work
Jan's: doesn't build
Rusty's: builds and works
Where do you want to go from here?
Jan, what are your thoughts? Your BUILD_BUG_ON patch has broken the
build, and we still need to fix it.
--
Hollis Blanchard
IBM Linux Technology Center
On Fri, 2009-10-09 at 12:14 -0700, Hollis Blanchard wrote:
quoted
Rusty's version of BUILD_BUG_ON() does indeed fix the build break, and
also exposes the bug in kvmppc_account_exit_stat(). So to recap:
=20
original: built but didn't work
Jan's: doesn't build
Rusty's: builds and works
=20
Where do you want to go from here?
Jan, what are your thoughts? Your BUILD_BUG_ON patch has broken the
build, and we still need to fix it.
My perspective is that it just uncovered already existing brokenness. And
honestly, I won't be able to get to look into this within the next days. =
(And
btw., when I run into issues with other people's code changes, quite
frequently I'm told to propose a patch, so I'm also having some
philosophical problem understanding why I can't simply expect the same
when people run into issues with changes I made, especially in cases like
this where it wasn't me introducing the broken code.) So, if this can wait
for a couple of days, I can try to find time to look into this. Otherwise, =
I'd
rely on someone running into the actual issue to implement a solution.
Jan
On Fri, 2009-10-09 at 12:14 -0700, Hollis Blanchard wrote:
quoted
Rusty's version of BUILD_BUG_ON() does indeed fix the build break, and
also exposes the bug in kvmppc_account_exit_stat(). So to recap:
original: built but didn't work
Jan's: doesn't build
Rusty's: builds and works
Where do you want to go from here?
Jan, what are your thoughts? Your BUILD_BUG_ON patch has broken the
build, and we still need to fix it.
My perspective is that it just uncovered already existing brokenness. And
honestly, I won't be able to get to look into this within the next days. (And
btw., when I run into issues with other people's code changes, quite
frequently I'm told to propose a patch, so I'm also having some
philosophical problem understanding why I can't simply expect the same
when people run into issues with changes I made, especially in cases like
this where it wasn't me introducing the broken code.) So, if this can wait
for a couple of days, I can try to find time to look into this. Otherwise, I'd
rely on someone running into the actual issue to implement a solution.
Sorry, I thought it was clear, but to be more explicit: I propose the
following patch, which replaces the current BUILD_BUG_ON implementation
with Rusty's version.
@@ -677,18 +677,19 @@ struct sysinfo {char_f[20-2*sizeof(long)-sizeof(int)];/* Padding: libc5 uses this.. */};-/* Force a compilation error if condition is true */-#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))--/* Force a compilation error if condition is constant and true */-#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))--/* Force a compilation error if condition is true, but also produce a-result(ofvalue0andtypesize_t),sotheexpressioncanbeused-e.g.inastructureinitializer(orwhere-everelsecommaexpressions-aren'tpermitted).*/-#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))-#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))+#ifndef __OPTIMIZE__+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))+#else+/* If it's a constant, catch it at compile time, otherwise at link time. */+externint__build_bug_on_failed;+#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)+#define BUILD_BUG_ON(condition) \+do{\+((void)sizeof(char[1-2*!!(condition)]));\+if(condition)__build_bug_on_failed=1;\+}while(0)+#define MAYBE_BUILD_BUG_ON(condition) BUILD_BUG_ON(condition)+#endif/* Trap pasters of __FUNCTION__ at compile-time */#define __FUNCTION__ (__func__)
From: Rusty Russell <hidden> Date: 2009-10-20 01:12:41
On Tue, 20 Oct 2009 04:49:29 am Hollis Blanchard wrote:
On Thu, 2009-10-15 at 08:27 +0100, Jan Beulich wrote:
quoted
My perspective is that it just uncovered already existing brokenness.
Sorry, I thought it was clear, but to be more explicit: I propose the
following patch, which replaces the current BUILD_BUG_ON implementation
with Rusty's version.
OK, I switched my brain back on. Yeah, I agree: we may still want
BUILD_OR_RUNTIME_BUG_ON one day, but I like this.
It's just missing the giant comment that it needs :)
/**
* BUILD_BUG_ON - break compile if a condition is true.
* @cond: the condition which the compiler should know is false.
*
* If you have some code which relies on certain constants being equal, or
* other compile-time-evaluated condition, you should use BUILD_BUG_ON to
* detect if someone changes it.
*
* The implementation uses gcc's reluctance to create a negative array, but
* gcc (as of 4.4) only emits that error for obvious cases (eg. not arguments
* to inline functions). So as a fallback we use the optimizer; if it can't
* prove the condition is false, it will cause a link error on the undefined
* "__build_bug_on_failed". This error is less neat, and can be harder to
* track down.
*/
Thanks!
Rusty.
On Tue, 2009-10-20 at 11:42 +1030, Rusty Russell wrote:
On Tue, 20 Oct 2009 04:49:29 am Hollis Blanchard wrote:
quoted
On Thu, 2009-10-15 at 08:27 +0100, Jan Beulich wrote:
quoted
My perspective is that it just uncovered already existing brokenness.
Sorry, I thought it was clear, but to be more explicit: I propose the
following patch, which replaces the current BUILD_BUG_ON implementation
with Rusty's version.
OK, I switched my brain back on. Yeah, I agree: we may still want
BUILD_OR_RUNTIME_BUG_ON one day, but I like this.
It's just missing the giant comment that it needs :)
/**
* BUILD_BUG_ON - break compile if a condition is true.
* @cond: the condition which the compiler should know is false.
*
* If you have some code which relies on certain constants being equal, or
* other compile-time-evaluated condition, you should use BUILD_BUG_ON to
* detect if someone changes it.
*
* The implementation uses gcc's reluctance to create a negative array, but
* gcc (as of 4.4) only emits that error for obvious cases (eg. not arguments
* to inline functions). So as a fallback we use the optimizer; if it can't
* prove the condition is false, it will cause a link error on the undefined
* "__build_bug_on_failed". This error is less neat, and can be harder to
* track down.
*/
Do you want to put together a signed-off patch Rusty? It's your code, so
I don't feel comfortable doing that.
Once we have that, can we remove the mysterious MAYBE_BUILD_BUG_ON
statements introduced in previous patches? (Does it BUG or doesn't it??)
--
Hollis Blanchard
IBM Linux Technology Center
From: Rusty Russell <hidden> Date: 2009-10-20 04:31:20
BUILD_BUG_ON used to use the optimizer to do code elimination or fail
at link time; it was changed to first the size of a negative array (a
nicer compile time error), then (in
8c87df457cb58fe75b9b893007917cf8095660a0) to a bitfield.
bitfields: needs a literal constant at parse time, and can't be put under
"if (__builtin_constant_p(x))" for example.
negative array: can handle anything, but if the compiler can't tell it's
a constant, silently has no effect.
link time: breaks link if the compiler can't determine the value, but the
linker output is not usually as informative as a compiler error.
If we use the negative-array-size method *and* the link time trick,
we get the ability to use BUILD_BUG_ON() under __builtin_constant_p()
branches, and maximal ability for the compiler to detect errors at
build time.
Signed-off-by: Rusty Russell <redacted>
@@ -683,12 +683,6 @@ struct sysinfo {char_f[20-2*sizeof(long)-sizeof(int)];/* Padding: libc5 uses this.. */};-/* Force a compilation error if condition is true */-#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))--/* Force a compilation error if condition is constant and true */-#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))-/* Force a compilation error if condition is true, but also produce aresult(ofvalue0andtypesize_t),sotheexpressioncanbeusede.g.inastructureinitializer(orwhere-everelsecommaexpressions
From: Américo Wang <hidden> Date: 2009-10-20 13:58:37
On Tue, Oct 20, 2009 at 02:15:33PM +1030, Rusty Russell wrote:
quoted hunk
BUILD_BUG_ON used to use the optimizer to do code elimination or fail
at link time; it was changed to first the size of a negative array (a
nicer compile time error), then (in
8c87df457cb58fe75b9b893007917cf8095660a0) to a bitfield.
bitfields: needs a literal constant at parse time, and can't be put under
"if (__builtin_constant_p(x))" for example.
negative array: can handle anything, but if the compiler can't tell it's
a constant, silently has no effect.
link time: breaks link if the compiler can't determine the value, but the
linker output is not usually as informative as a compiler error.
If we use the negative-array-size method *and* the link time trick,
we get the ability to use BUILD_BUG_ON() under __builtin_constant_p()
branches, and maximal ability for the compiler to detect errors at
build time.
Signed-off-by: Rusty Russell <redacted>
char _f[20-2*sizeof(long)-sizeof(int)]; /* Padding: libc5 uses this.. */
};
-/* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))
-
-/* Force a compilation error if condition is constant and true */
-#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))
-
/* Force a compilation error if condition is true, but also produce a
result (of value 0 and type size_t), so the expression can be used
e.g. in a structure initializer (or where-ever else comma expressions
@@ -696,6 +690,33 @@ struct sysinfo {
#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
+/**
+ * BUILD_BUG_ON - break compile if a condition is true.
+ * @cond: the condition which the compiler should know is false.
+ *
+ * If you have some code which relies on certain constants being equal, or
+ * other compile-time-evaluated condition, you should use BUILD_BUG_ON to
+ * detect if someone changes it.
+ *
+ * The implementation uses gcc's reluctance to create a negative array, but
+ * gcc (as of 4.4) only emits that error for obvious cases (eg. not arguments
+ * to inline functions). So as a fallback we use the optimizer; if it can't
+ * prove the condition is false, it will cause a link error on the undefined
+ * "__build_bug_on_failed". This error message can be harder to track down
+ * though, hence the two different methods.
+ */
+#ifndef __OPTIMIZE__
+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#else
+extern int __build_bug_on_failed;
Hmm, what exactly is __build_bug_on_failed?
+#define BUILD_BUG_ON(condition) \
+ do { \
+ ((void)sizeof(char[1 - 2*!!(condition)])); \
+ if (condition) __build_bug_on_failed = 1; \
+ } while(0)
+#endif
+#define MAYBE_BUILD_BUG_ON(condition) BUILD_BUG_ON(condition)
+
/* Trap pasters of __FUNCTION__ at compile-time */
#define __FUNCTION__ (__func__)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Jenkins <hidden> Date: 2009-10-20 14:43:14
On 10/20/09, Am=E9rico Wang [off-list ref] wrote:
On Tue, Oct 20, 2009 at 02:15:33PM +1030, Rusty Russell wrote:
quoted
BUILD_BUG_ON used to use the optimizer to do code elimination or fail
at link time; it was changed to first the size of a negative array (a
nicer compile time error), then (in
8c87df457cb58fe75b9b893007917cf8095660a0) to a bitfield.
bitfields: needs a literal constant at parse time, and can't be put under
"if (__builtin_constant_p(x))" for example.
negative array: can handle anything, but if the compiler can't tell it's
a constant, silently has no effect.
link time: breaks link if the compiler can't determine the value, but the
linker output is not usually as informative as a compiler error.
If we use the negative-array-size method *and* the link time trick,
we get the ability to use BUILD_BUG_ON() under __builtin_constant_p()
branches, and maximal ability for the compiler to detect errors at
build time.
Signed-off-by: Rusty Russell <redacted>
};
-/* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))
-
-/* Force a compilation error if condition is constant and true */
-#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))
-
/* Force a compilation error if condition is true, but also produce a
result (of value 0 and type size_t), so the expression can be used
e.g. in a structure initializer (or where-ever else comma expressions
@@ -696,6 +690,33 @@ struct sysinfo {
#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
+/**
+ * BUILD_BUG_ON - break compile if a condition is true.
+ * @cond: the condition which the compiler should know is false.
+ *
+ * If you have some code which relies on certain constants being equal, =
or
quoted
+ * other compile-time-evaluated condition, you should use BUILD_BUG_ON t=
o
quoted
+ * detect if someone changes it.
+ *
+ * The implementation uses gcc's reluctance to create a negative array,
but
+ * gcc (as of 4.4) only emits that error for obvious cases (eg. not
arguments
+ * to inline functions). So as a fallback we use the optimizer; if it
can't
+ * prove the condition is false, it will cause a link error on the
undefined
+ * "__build_bug_on_failed". This error message can be harder to track
down
+ * though, hence the two different methods.
+ */
+#ifndef __OPTIMIZE__
+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)])=
)
quoted
+#else
+extern int __build_bug_on_failed;
Hmm, what exactly is __build_bug_on_failed?
Well, we haven't added a definition for it in this patch. I'm sure
grep will tell you it wasn't defined before hand either. So any
reference to it is an error - which will be reported at link time.
quoted
+#define BUILD_BUG_ON(condition) \
+ do { \
+ ((void)sizeof(char[1 - 2*!!(condition)])); \
+ if (condition) __build_bug_on_failed =3D 1; \
If "condition" is known false at compile time, gcc -O will eliminate
the code which refers to __build_bug_on_failed. If it's not proved to
be false - it will break the build, which is exactly what we want
BUILD_BUG_ON to do.
quoted
+ } while(0)
+#endif
+#define MAYBE_BUILD_BUG_ON(condition) BUILD_BUG_ON(condition)
+
/* Trap pasters of __FUNCTION__ at compile-time */
#define __FUNCTION__ (__func__)
On Tue, 2009-10-20 at 14:15 +1030, Rusty Russell wrote:
BUILD_BUG_ON used to use the optimizer to do code elimination or fail
at link time; it was changed to first the size of a negative array (a
nicer compile time error), then (in
8c87df457cb58fe75b9b893007917cf8095660a0) to a bitfield.
bitfields: needs a literal constant at parse time, and can't be put under
"if (__builtin_constant_p(x))" for example.
negative array: can handle anything, but if the compiler can't tell it's
a constant, silently has no effect.
link time: breaks link if the compiler can't determine the value, but the
linker output is not usually as informative as a compiler error.
If we use the negative-array-size method *and* the link time trick,
we get the ability to use BUILD_BUG_ON() under __builtin_constant_p()
branches, and maximal ability for the compiler to detect errors at
build time.
Signed-off-by: Rusty Russell <redacted>
Thanks Rusty, this indeed fixes the problem.
Acked-by: Hollis Blanchard <redacted>
--
Hollis Blanchard
IBM Linux Technology Center
From: Américo Wang <hidden> Date: 2009-10-23 01:50:03
On Tue, Oct 20, 2009 at 10:43 PM, Alan Jenkins
[off-list ref] wrote:
On 10/20/09, Am=C3=A9rico Wang [off-list ref] wrote:
quoted
On Tue, Oct 20, 2009 at 02:15:33PM +1030, Rusty Russell wrote:
quoted
BUILD_BUG_ON used to use the optimizer to do code elimination or fail
at link time; it was changed to first the size of a negative array (a
nicer compile time error), then (in
8c87df457cb58fe75b9b893007917cf8095660a0) to a bitfield.
bitfields: needs a literal constant at parse time, and can't be put unde=
r
quoted
quoted
=C2=A0 =C2=A0 =C2=A0"if (__builtin_constant_p(x))" for example.
negative array: can handle anything, but if the compiler can't tell it's
=C2=A0 =C2=A0 =C2=A0a constant, silently has no effect.
link time: breaks link if the compiler can't determine the value, but th=
e
quoted
quoted
=C2=A0 =C2=A0 =C2=A0linker output is not usually as informative as a co=
mpiler error.
quoted
quoted
If we use the negative-array-size method *and* the link time trick,
we get the ability to use BUILD_BUG_ON() under __builtin_constant_p()
branches, and maximal ability for the compiler to detect errors at
build time.
Signed-off-by: Rusty Russell <redacted>
};
-/* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))
-
-/* Force a compilation error if condition is constant and true */
-#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))
-
/* Force a compilation error if condition is true, but also produce a
=C2=A0 =C2=A0result (of value 0 and type size_t), so the expression can=
be used
quoted
quoted
=C2=A0 =C2=A0e.g. in a structure initializer (or where-ever else comma =
expressions
quoted
quoted
@@ -696,6 +690,33 @@ struct sysinfo {
#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))
+/**
+ * BUILD_BUG_ON - break compile if a condition is true.
+ * @cond: the condition which the compiler should know is false.
+ *
+ * If you have some code which relies on certain constants being equal,=
or
quoted
quoted
+ * other compile-time-evaluated condition, you should use BUILD_BUG_ON =
to
quoted
quoted
+ * detect if someone changes it.
+ *
+ * The implementation uses gcc's reluctance to create a negative array,
but
+ * gcc (as of 4.4) only emits that error for obvious cases (eg. not
arguments
+ * to inline functions). =C2=A0So as a fallback we use the optimizer; i=
f it
quoted
quoted
can't
+ * prove the condition is false, it will cause a link error on the
undefined
+ * "__build_bug_on_failed". =C2=A0This error message can be harder to t=
rack
quoted
quoted
down
+ * though, hence the two different methods.
+ */
+#ifndef __OPTIMIZE__
+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]=
))
quoted
quoted
+#else
+extern int __build_bug_on_failed;
Hmm, what exactly is __build_bug_on_failed?
Well, we haven't added a definition for it in this patch. =C2=A0I'm sure
grep will tell you it wasn't defined before hand either. =C2=A0So any
reference to it is an error - which will be reported at link time.
On Tue, 2009-10-20 at 14:15 +1030, Rusty Russell wrote:
quoted hunk
BUILD_BUG_ON used to use the optimizer to do code elimination or fail
at link time; it was changed to first the size of a negative array (a
nicer compile time error), then (in
8c87df457cb58fe75b9b893007917cf8095660a0) to a bitfield.
bitfields: needs a literal constant at parse time, and can't be put under
"if (__builtin_constant_p(x))" for example.
negative array: can handle anything, but if the compiler can't tell it's
a constant, silently has no effect.
link time: breaks link if the compiler can't determine the value, but the
linker output is not usually as informative as a compiler error.
If we use the negative-array-size method *and* the link time trick,
we get the ability to use BUILD_BUG_ON() under __builtin_constant_p()
branches, and maximal ability for the compiler to detect errors at
build time.
Signed-off-by: Rusty Russell <redacted>
@@ -683,12 +683,6 @@ struct sysinfo {char_f[20-2*sizeof(long)-sizeof(int)];/* Padding: libc5 uses this.. */};-/* Force a compilation error if condition is true */-#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))--/* Force a compilation error if condition is constant and true */-#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))-/* Force a compilation error if condition is true, but also produce aresult(ofvalue0andtypesize_t),sotheexpressioncanbeusede.g.inastructureinitializer(orwhere-everelsecommaexpressions
I decided to try this in linux-next, but an x86_64 allmodconfig build
gave this (gcc 4.4.0):
ERROR: "__build_bug_on_failed" [drivers/net/virtio_net.ko] undefined!
ERROR: "__build_bug_on_failed" [drivers/block/virtio_blk.ko] undefined!
I assume that this is caused by the "MAYBE_BUILD_BUG_ON(fbit >= 32)" in
virtio_has_feature() (in include/linux/virtio_config.h) which is called
all over the place. Unfortunately, virtio_has_feature() gets uninlined
in those two files ...
I have taken the patch back out again for today.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-11-05 06:01:51
On Tue, 2009-10-20 at 14:15 +1030, Rusty Russell wrote:
BUILD_BUG_ON used to use the optimizer to do code elimination or fail
at link time; it was changed to first the size of a negative array (a
nicer compile time error), then (in
8c87df457cb58fe75b9b893007917cf8095660a0) to a bitfield.
What's the status with this patch ? The lack of it breaks my KVM stuff
in powerpc...
Cheers,
Ben.
quoted hunk
bitfields: needs a literal constant at parse time, and can't be put under
"if (__builtin_constant_p(x))" for example.
negative array: can handle anything, but if the compiler can't tell it's
a constant, silently has no effect.
link time: breaks link if the compiler can't determine the value, but the
linker output is not usually as informative as a compiler error.
If we use the negative-array-size method *and* the link time trick,
we get the ability to use BUILD_BUG_ON() under __builtin_constant_p()
branches, and maximal ability for the compiler to detect errors at
build time.
Signed-off-by: Rusty Russell <redacted>
@@ -683,12 +683,6 @@ struct sysinfo {char_f[20-2*sizeof(long)-sizeof(int)];/* Padding: libc5 uses this.. */};-/* Force a compilation error if condition is true */-#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))--/* Force a compilation error if condition is constant and true */-#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))-/* Force a compilation error if condition is true, but also produce aresult(ofvalue0andtypesize_t),sotheexpressioncanbeusede.g.inastructureinitializer(orwhere-everelsecommaexpressions
I decided to try this in linux-next, but an x86_64 allmodconfig build
gave this (gcc 4.4.0):
ERROR: "__build_bug_on_failed" [drivers/net/virtio_net.ko] undefined!
ERROR: "__build_bug_on_failed" [drivers/block/virtio_blk.ko] undefined!
I assume that this is caused by the "MAYBE_BUILD_BUG_ON(fbit >= 32)" in
virtio_has_feature() (in include/linux/virtio_config.h) which is called
all over the place. Unfortunately, virtio_has_feature() gets uninlined
in those two files ...
Huh? virtio_has_feature does:
if (__builtin_constant_p(fbit))
BUILD_BUG_ON(fbit >= 32);
else
BUG_ON(fbit >= 32);
So, if it's not a constant, gcc should throw away that first branch. If it
is, it should optimize it away (and no, these are not real bugs AFAICT).
I did a 4.3.3 allmodconfig with this patch on 64-bit a few days back and all
was fine. Will try 4.4.0 now.
Thanks,
Rusty.
From: Rusty Russell <hidden> Date: 2009-11-05 06:37:14
On Thu, 5 Nov 2009 04:58:36 pm Rusty Russell wrote:
I did a 4.3.3 allmodconfig with this patch on 64-bit a few days back and all
was fine. Will try 4.4.0 now.
4.4.1 seems OK (Ubuntu).
This is annoying. But I'll withdraw the patches; if there's another reason
that 4.4.0 is bad, we can ban it and reintroduce this. I don't think that
breaking this hack is enough to declare 4.4.0 verboten.
Thanks,
Rusty.
In Linus' tree (and linux-next) it looks like this:
static inline bool virtio_has_feature(const struct virtio_device *vdev,
unsigned int fbit)
{
/* Did you forget to fix assumptions on max features? */
MAYBE_BUILD_BUG_ON(fbit >= 32);
if (fbit < VIRTIO_TRANSPORT_F_START)
virtio_check_driver_offered_feature(vdev, fbit);
return test_bit(fbit, vdev->features);
}
So, if it's not a constant, gcc should throw away that first branch. If it
is, it should optimize it away (and no, these are not real bugs AFAICT).
Your version above may well fix the problem. Alternatively marking it
__always_inline way work as well.
I did a 4.3.3 allmodconfig with this patch on 64-bit a few days back and all
was fine. Will try 4.4.0 now.