Message-ID: <73c1f2160909141654w51df78n4f2319cfd2f9362e@mail.gmail.com>
Date:	Mon, 14 Sep 2009 19:54:57 -0400
From:	Brian Gerst <brgerst@...il.com>
To:	David Daney <ddaney@...iumnetworks.com>
Cc:	torvalds@...ux-foundation.org, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH 11/11] Use unreachable() in asm-generic/bug.h for 
	!CONFIG_BUG case.

On Mon, Sep 14, 2009 at 7:28 PM, David Daney <ddaney@...iumnetworks.com> wrote:
> Brian Gerst wrote:
>>
>> On Mon, Sep 14, 2009 at 5:55 PM, David Daney <ddaney@...iumnetworks.com>
>> wrote:
>>>
>>> The subject says it all (most).  The only drawback here is that for a
>>> pre-GCC-5.4 compiler, instead of expanding to nothing we now expand
>>> BUG() to an endless loop.  Before the patch when configured with
>>> !CONFIG_BUG() you might get some warnings, but the code would be
>>> small.  After the patch there are no warnings, but there is an endless
>>> loop at each BUG() site.
>>>
>>> Of course for the GCC-4.5 case we get the best of both worlds.
>>>
>>> Signed-off-by: David Daney <ddaney@...iumnetworks.com>
>>> Suggested-by: Ingo Molnar <mingo@...e.hu>
>>> CC: Ingo Molnar <mingo@...e.hu>
>>> ---
>>>  include/asm-generic/bug.h |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>>> index 4b67559..e952242 100644
>>> --- a/include/asm-generic/bug.h
>>> +++ b/include/asm-generic/bug.h
>>> @@ -89,11 +89,11 @@ extern void warn_slowpath_null(const char *file,
>>> const int line);
>>>
>>>  #else /* !CONFIG_BUG */
>>>  #ifndef HAVE_ARCH_BUG
>>> -#define BUG() do {} while(0)
>>> +#define BUG() unreachable()
>>>  #endif
>>>
>>>  #ifndef HAVE_ARCH_BUG_ON
>>> -#define BUG_ON(condition) do { if (condition) ; } while(0)
>>> +#define BUG_ON(condition) do { if (condition) unreachable(); } while (0)
>>>  #endif
>>>
>>>  #ifndef HAVE_ARCH_WARN_ON
>>> --
>>
>> This seems wrong to me.  Wouldn't you always want to do the endless
>> loop?  In the absence of an arch-specific method to jump to an
>> exception handler, it isn't really unreachable.  On gcc 4.5 this would
>> essentially become a no-op.
>>
>
> Several points:
>
> * When you hit a BUG() you are screwed.
>
> * When you configure with !CONFIG_BUG you are asserting that you don't want
> to try to trap on BUG();.
>
> The existing code just falls through to whatever happens to follow the
> BUG().  This is not what the programmer intended, but the person that chose
> !CONFIG_BUG decided that they would like undefined behavior in order to save
> a few bytes of code.
>
> With the patch one of two things will happen:
>
> pre-GCC-4.5) We will now enter an endless loop and not fall through. This
> makes the code slightly larger than pre patch.
>
> post-GCC-4.5) We do something totally undefined.  It will not necessarily
> fall through to the code after the BUG()  It could really end up doing
> almost anything.  On the plus side, we save a couple of bytes of code and
> eliminate some compiler warnings.
>
> If you don't like it, don't configure with !CONFIG_BUG.  But the patch
> doesn't really change the fact that hitting a BUG() with !CONFIG_BUG leads
> to undefined behavior.  It only makes the case where you don't hit BUG()
> nicer.
>
> David Daney
>

Let me rephrase this.  The original BUG() is simply a no-op, not an
infinite loop.  GCC will optimize it away (and possibly other dead
code around it).  Adding unreachable() makes the code do potentially
unpredictable things.  It's not necessary.  The same goes for BUG_ON.
In that case the test does get optimized away too, but is still needed
to silence warnings about unused variables, etc.

--
Brian Gerst
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists