Sometimes we want to check some expressions correctness in compile-time without
generating extra code. "(void)(e)" does not work if expression has side-effects.
This patch introduces macro unused_expression() which helps in this situation.
Cast to "long" required because sizeof does not work for bit-fields.
On 04/25/2012 07:26 PM, Konstantin Khlebnikov wrote:
> Sometimes we want to check some expressions correctness in compile-time without
> generating extra code. "(void)(e)" does not work if expression has side-effects.
> This patch introduces macro unused_expression() which helps in this situation.
Interesting, I am wondering why gcc doesn't eliminate the code as we pass either -O2 or -Os to it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Konstantin Khlebnikov <khlebni...@openvz.org> wrote:
> Sometimes we want to check some expressions correctness in compile-time without
> generating extra code. "(void)(e)" does not work if expression has side-effects.
> This patch introduces macro unused_expression() which helps in this situation.
> Cast to "long" required because sizeof does not work for bit-fields.
Thing is, if anyone ever has an expression-with-side-effects within
conditionally-compiled code then they probably have a bug, don't they?
I mean, as an extreme example
VM_BUG_ON(do_something_important());
is a nice little hand-grenade. Your patch will cause that (bad) code
to newly fail at runtime, but our coverage testing is so awful that it
would take a long time for the bug to be discovered.
It would be nice if we could cause the build to warn or outright fail
if the unused_expression() argument would have caused any code
generation. But I can't suggest how to do that.
Your changelogs assert that gcc is emitting code for these expressions,
but details are not presented. Please give examples - where is this
code generation coming from, what is causing it?
Bottom line: are these patches a workaround for gcc inadequacies, or
are they a bandaid covering up poor kernel code?
Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
> On Wed, Apr 25, 2012 at 13:26, Konstantin Khlebnikov
> <khlebni...@openvz.org> wrote:
> > Even if CONFIG_DEBUG_VM=n gcc genereates code for some VM_BUG_ON()
> That's because of the side effects of the expression
AFAICT (lkml.org appears to be having a meltdown), you've gone and
linked to this very thread.
Please try again, this time avoiding hyperlinks ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Konstantin Khlebnikov <khlebni...@openvz.org> wrote:
> Sometimes we want to check some expressions correctness in compile-time without
> generating extra code. "(void)(e)" does not work if expression has side-effects.
> This patch introduces macro unused_expression() which helps in this situation.
> Cast to "long" required because sizeof does not work for bit-fields.
+/*
+ * unused_expression() permits the compiler to check the validity of the
+ * expression but avoids the generation of any code, even if that expression
+ * has side-effects.
+ */
#define unused_expression(e) ((void)(sizeof((__force long)(e))))
#endif /* __LINUX_COMPILER_H */
but then decided I didn't want to apply the patches (yet?).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Fri, Apr 27, 2012 at 00:32, Andrew Morton <a...@linux-foundation.org> wrote:
> On Wed, 25 Apr 2012 16:40:32 +0200
> Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
>> On Wed, Apr 25, 2012 at 13:26, Konstantin Khlebnikov
>> <khlebni...@openvz.org> wrote:
>> > Even if CONFIG_DEBUG_VM=n gcc genereates code for some VM_BUG_ON()
>> That's because of the side effects of the expression
> AFAICT (lkml.org appears to be having a meltdown), you've gone and
> linked to this very thread.
> Please try again, this time avoiding hyperlinks ;)
Yeah, I noticed after the fact. I wanted to look up the definition of
unused_expression(), which obviously wasn't in my tree yet ;-)
Still, I think people started relyong on the side effects, didn't they?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> On Fri, Apr 27, 2012 at 00:32, Andrew Morton <a...@linux-foundation.org> wrote:
> > On Wed, 25 Apr 2012 16:40:32 +0200
> > Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
> >> On Wed, Apr 25, 2012 at 13:26, Konstantin Khlebnikov
> >> <khlebni...@openvz.org> wrote:
> >> > Even if CONFIG_DEBUG_VM=n gcc genereates code for some VM_BUG_ON()
> >> That's because of the side effects of the expression
> > AFAICT (lkml.org appears to be having a meltdown), you've gone and
> > linked to this very thread.
> > Please try again, this time avoiding hyperlinks ;)
> Yeah, I noticed after the fact. I wanted to look up the definition of
> unused_expression(), which obviously wasn't in my tree yet ;-)
> Still, I think people started relyong on the side effects, didn't they?
I hope not. If they are, they snuck it past me cleverly! A quick grep
of mm/*.c looks clean.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Andrew Morton wrote:
> On Wed, 25 Apr 2012 15:26:23 +0400
> Konstantin Khlebnikov<khlebni...@openvz.org> wrote:
>> Sometimes we want to check some expressions correctness in compile-time without
>> generating extra code. "(void)(e)" does not work if expression has side-effects.
>> This patch introduces macro unused_expression() which helps in this situation.
>> Cast to "long" required because sizeof does not work for bit-fields.
> +/*
> + * unused_expression() permits the compiler to check the validity of the
> + * expression but avoids the generation of any code, even if that expression
> + * has side-effects.
> + */
> #define unused_expression(e) ((void)(sizeof((__force long)(e))))
> #endif /* __LINUX_COMPILER_H */
> but then decided I didn't want to apply the patches (yet?).
I'm OK with that. But probably Hugh Dickins (now in CC) aggressively wants to
remove side-effects of VM_BUG_ON's in case CONFIG_DEBUG_VM=n
> Sometimes we want to check some expressions correctness in compile-time without
> generating extra code. "(void)(e)" does not work if expression has side-effects.
> This patch introduces macro unused_expression() which helps in this situation.
> Cast to "long" required because sizeof does not work for bit-fields.
"unused_expression()" doesn't sound like something that inhibits side
effects, especially since "unused" applied to an argument or variable
means "don't make a fuss over this not being used."
-hpa
-- H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
Andrew Morton wrote:
> On Wed, 25 Apr 2012 15:26:23 +0400
> Konstantin Khlebnikov<khlebni...@openvz.org> wrote:
>> Sometimes we want to check some expressions correctness in compile-time without
>> generating extra code. "(void)(e)" does not work if expression has side-effects.
>> This patch introduces macro unused_expression() which helps in this situation.
>> Cast to "long" required because sizeof does not work for bit-fields.
> Thing is, if anyone ever has an expression-with-side-effects within
> conditionally-compiled code then they probably have a bug, don't they?
> I mean, as an extreme example
> VM_BUG_ON(do_something_important());
> is a nice little hand-grenade. Your patch will cause that (bad) code
> to newly fail at runtime, but our coverage testing is so awful that it
> would take a long time for the bug to be discovered.
> It would be nice if we could cause the build to warn or outright fail
> if the unused_expression() argument would have caused any code
> generation. But I can't suggest how to do that.
> Your changelogs assert that gcc is emitting code for these expressions,
> but details are not presented. Please give examples - where is this
> code generation coming from, what is causing it?
for example VM_BUG_ON(!PageCompound(page) || !PageHead(page)); in
do_huge_pmd_wp_page() generates 114 bytes.
But they mostly disappears if I replace it with
-VM_BUG_ON(!PageCompound(page) || !PageHead(page));
+VM_BUG_ON(!PageCompound(page));
+VM_BUG_ON(!PageHead(page));
weird...
> Bottom line: are these patches a workaround for gcc inadequacies, or
> are they a bandaid covering up poor kernel code?
gcc do weird things, but sometimes it really cannot throw out code.
I hope we can do this safely for VM_BUG_ON(), anyway nobody disables real BUG_ON()
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Konstantin Khlebnikov <khlebni...@openvz.org> wrote:
> Andrew Morton wrote:
> > On Wed, 25 Apr 2012 15:26:23 +0400
> > Konstantin Khlebnikov<khlebni...@openvz.org> wrote:
> for example VM_BUG_ON(!PageCompound(page) || !PageHead(page)); in
> do_huge_pmd_wp_page() generates 114 bytes.
> But they mostly disappears if I replace it with
> -VM_BUG_ON(!PageCompound(page) || !PageHead(page));
> +VM_BUG_ON(!PageCompound(page));
> +VM_BUG_ON(!PageHead(page));
> weird...
OK, thanks. I'm inclined to apply the patchset as-is. If the
apparently mythical use of side-effects in VM_BUG_ON() really exist
then we deserve everything which happens to us as a result ;)
Please update the changelogs so they cover all the points which have been
discussed, add my little code comment then send out a v2, being sure to
cc everyone who has been involved in the discussion?
H. Peter Anvin wrote:
> On 04/25/2012 04:26 AM, Konstantin Khlebnikov wrote:
>> Sometimes we want to check some expressions correctness in compile-time without
>> generating extra code. "(void)(e)" does not work if expression has side-effects.
>> This patch introduces macro unused_expression() which helps in this situation.
>> Cast to "long" required because sizeof does not work for bit-fields.
> "unused_expression()" doesn't sound like something that inhibits side
> effects, especially since "unused" applied to an argument or variable
> means "don't make a fuss over this not being used."
> -hpa
Ok, in v2 I'll rename it into BUILD_BUG_ON_INVALID()
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> With this patch gcc throw out loop at the end of do_exit():
I think that's the least of our problems - that loop only exists to
get rid of a warning, and the return from that schedule() really is
supposed to be unreachable().
What makes me much more worried is that iirc, gcc will use
"unreachable()" to also get rid of function epilogues etc, which is
perfectly fine if it really is something entirely unreachable. But the
kernel use of BUG() is often as a kind of "assert()", and if gcc
generates actively bad code for it (and it does, with unreachable()),
it turns the turned-off BUG() into something *really* horrible that
makes for a debugging disaster.
So some pattern like
if (badness)
BUG();
would generate code that is actively insane, instead of (like now)
generating basically a no-op.
And THAT makes me go "Eww". "unreachable()" should only be used in
situations that really are very very unreachable() (eg after an "asm"
that goes off to la-la-land or a call to "exit()" in user land etc).
The kernel kinds of "assert" is *hopefully* not reachable, but if we
ever reach it, we don't want to make things worse. The BUG() was there
to give us a nicer debug message about what went wrong, and I think
your patch actively destroys that whole thing and makes for something
*worse*.
On Sat, 28 Apr 2012 09:10:18 +0400 Konstantin Khlebnikov <khlebni...@openvz.org> wrote:
> Konstantin Khlebnikov wrote:
> > This patch suppress some compiler warnings (if CONFIG_BUG=n)
> > "warning: control reaches end of non-void function"
> With this patch gcc throw out loop at the end of do_exit():
> schedule();
> BUG();
> /* Avoid "noreturn function does return". */
> for (;;)
> cpu_relax(); /* For when BUG is null */
> Is this ok? Probably not, and we need here some BUG_NORETURN() which
> really never returns even if CONFIG_BUG=n.
Probably we should do away with CONFIG_BUG.
CONFIG_BUG=n causes various compile-time warnings to come out when the
execution proceeds into places where we didn't intend and these aren't
worth fixing.
More seriously, I rather doubt that anyone ever sets it to n. And it
would be a pretty dumb thing to do - if your kernel has seriously
malfunctioned and it *knows* that it malfunctioned, it will just
blunder on anyway not telling anyone about it.
It doesn't seem worth any effort to support this thing.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Sometimes we want to check some expressions correctness in compile-time.
"(void)(e);" or "if (e);" can be dangerous if expression has side-effects, and
gcc sometimes generates a lot of code, even if the expression has no effect.
This patch introduces macro BUILD_BUG_ON_INVALID() for such checks,
it forces a compilation error if expression is invalid without any extra code.
[Cast to "long" required because sizeof does not work for bit-fields.]
+/*
+ * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the
+ * expression but avoids the generation of any code, even if that expression
+ * has side-effects.
+ */
+#define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
+
/**
* BUILD_BUG_ON - break compile if a condition is true.
* @condition: the condition which the compiler should know is false.
Even if CONFIG_DEBUG_VM=n gcc genereates code for some VM_BUG_ON()
for example VM_BUG_ON(!PageCompound(page) || !PageHead(page)); in
do_huge_pmd_wp_page() generates 114 bytes of code.
But they mostly disappears when I split this VM_BUG_ON into two:
-VM_BUG_ON(!PageCompound(page) || !PageHead(page));
+VM_BUG_ON(!PageCompound(page));
+VM_BUG_ON(!PageHead(page));
weird... but anyway after this patch code disappears completely.
