Re: [PATCH] ARM: Convert BUG() to use unreachable()

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[David Daney]

Jamie Lokier wrote:
> Uwe Kleine-König wrote:
>> Use the new unreachable() macro instead of for(;;);
>>  	*(int *)0 = 0;
>>  
>>  	/* Avoid "noreturn function does return" */
>> -	for (;;);
>> +	unreachable();
> 
> Will GCC-4.5 remove ("optimise away") the *(int *)0 = 0 because it
> knows the branch of the code leading to unreachable can never be reached?
> 

I don't know the definitive answer, so I am sending to gcc@...

FYI: #define unreachable() __builtin_unreachable()


> If GCC-4.5 does not, are you sure a future version of GCC will never
> remove it?  In other words, is __builtin_unreachable() _defined_ in
> such a way that it cannot remove the previous assignment?
> 
> We have seen problems with GCC optimising away important tests for
> NULL pointers in the kernel, due to similar propagation of "impossible
> to occur" conditions, so it's worth checking with GCC people what the
> effect of this one would be.
> 
> In C, there is a general theoretical problem with back-propagation of
> optimisations from code with undefined behaviour.  In the case of
> __builtin_unreachable(), it would depend on all sorts of unclearly
> defined semantics whether it can remove a preceding *(int *)0 = 0.
> 
> I'd strongly suggest asking on the GCC list.  (I'd have mentioned this
> earlier, if I'd known about the patch for other architectures).
> 
> The documentation for __builtin_unreachable() only says the program is
> undefined if control flow reaches it.  In other words, it does not say
> what effect it can have on previous instructions, and I think it's
> quite likely that it has not been analysed in a case like this.
> 
> One thing that would give me a lot more confidence, because the GCC
> documentation does mention asm(), is this:
> 
>>       *(int *)0 = 0;
>>       /* Ensure unreachableness optimisations cannot propagate back. *I/
>>       __asm__ volatile("");
>>       /* Avoid "noreturn function does return" */
>>       unreachable();
> 
> -- Jamie

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Richard Guenther]

On Thu, Dec 17, 2009 at 6:09 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> Jamie Lokier wrote:
>>
>> Uwe Kleine-König wrote:
>>>
>>> Use the new unreachable() macro instead of for(;;);
>>>        *(int *)0 = 0;
>>>          /* Avoid "noreturn function does return" */
>>> -       for (;;);
>>> +       unreachable();
>>
>> Will GCC-4.5 remove ("optimise away") the *(int *)0 = 0 because it
>> knows the branch of the code leading to unreachable can never be reached?
>>
>
> I don't know the definitive answer, so I am sending to gcc@...
>
> FYI: #define unreachable() __builtin_unreachable()

It shouldn't as *(int *)0 = 0; might trap.  But if you want to be sure
use
   __builtin_trap ();
instead for the whole sequence (the unreachable is implied then).
GCC choses a size-optimal trap representation for your target then.

Richard.

>
>> If GCC-4.5 does not, are you sure a future version of GCC will never
>> remove it?  In other words, is __builtin_unreachable() _defined_ in
>> such a way that it cannot remove the previous assignment?
>>
>> We have seen problems with GCC optimising away important tests for
>> NULL pointers in the kernel, due to similar propagation of "impossible
>> to occur" conditions, so it's worth checking with GCC people what the
>> effect of this one would be.
>>
>> In C, there is a general theoretical problem with back-propagation of
>> optimisations from code with undefined behaviour.  In the case of
>> __builtin_unreachable(), it would depend on all sorts of unclearly
>> defined semantics whether it can remove a preceding *(int *)0 = 0.
>>
>> I'd strongly suggest asking on the GCC list.  (I'd have mentioned this
>> earlier, if I'd known about the patch for other architectures).
>>
>> The documentation for __builtin_unreachable() only says the program is
>> undefined if control flow reaches it.  In other words, it does not say
>> what effect it can have on previous instructions, and I think it's
>> quite likely that it has not been analysed in a case like this.
>>
>> One thing that would give me a lot more confidence, because the GCC
>> documentation does mention asm(), is this:
>>
>>>      *(int *)0 = 0;
>>>      /* Ensure unreachableness optimisations cannot propagate back. *I/
>>>      __asm__ volatile("");
>>>      /* Avoid "noreturn function does return" */
>>>      unreachable();
>>
>> -- Jamie
>
>

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Russell King - ARM Linux]

On Thu, Dec 17, 2009 at 06:17:11PM +0100, Richard Guenther wrote:
> On Thu, Dec 17, 2009 at 6:09 PM, David Daney <ddaney@caviumnetworks.com> wrote:
> > Jamie Lokier wrote:
> >>
> >> Uwe Kleine-König wrote:
> >>>
> >>> Use the new unreachable() macro instead of for(;;);
> >>>        *(int *)0 = 0;
> >>>          /* Avoid "noreturn function does return" */
> >>> -       for (;;);
> >>> +       unreachable();
> >>
> >> Will GCC-4.5 remove ("optimise away") the *(int *)0 = 0 because it
> >> knows the branch of the code leading to unreachable can never be reached?
> >>
> >
> > I don't know the definitive answer, so I am sending to gcc@...
> >
> > FYI: #define unreachable() __builtin_unreachable()
> 
> It shouldn't as *(int *)0 = 0; might trap.  But if you want to be sure
> use
>    __builtin_trap ();
> instead for the whole sequence (the unreachable is implied then).
> GCC choses a size-optimal trap representation for your target then.

How is "size-optimal trap" defined?  The point of "*(int *)0 = 0;" is
to cause a NULL pointer dereference which is trapped by the kernel to
produce a full post mortem and backtrace which is easily recognised
as a result of this code.

Having gcc decide on, maybe, an undefined instruction instead would be
confusing.

Let me put it another way: I want this function to terminate with an
explicit NULL pointer dereference in every case.

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Joe Buck]

On Thu, Dec 17, 2009 at 10:17:18AM -0800, Russell King - ARM Linux wrote:
> > It shouldn't as *(int *)0 = 0; might trap.  But if you want to be sure
> > use
> >    __builtin_trap ();
> > instead for the whole sequence (the unreachable is implied then).
> > GCC choses a size-optimal trap representation for your target then.
> 
> How is "size-optimal trap" defined?  The point of "*(int *)0 = 0;" is
> to cause a NULL pointer dereference which is trapped by the kernel to
> produce a full post mortem and backtrace which is easily recognised
> as a result of this code.

With something like __builtin_trap, the compiler knows that your intent
is to cause a trap.  But it's asking for trouble, and for future
flame wars between kernel developers and gcc developers, to put in
sequences that happen to do the right thing if the compiler does
no optimizations whatsoever, but that might be messed up by optimizations
because they are code sequences whose behavior is undefined.

Besides, didn't I see a whole bunch of kernel security patches related
to null pointer dereferences lately?  If page 0 can be mapped, you
suddenly won't get your trap.

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Russell King - ARM Linux]

On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote:
> Besides, didn't I see a whole bunch of kernel security patches related
> to null pointer dereferences lately?  If page 0 can be mapped, you
> suddenly won't get your trap.

Page 0 can not be mapped on ARM kernels since the late 1990s, and this
protection is independent of the generic kernel.

Milage may vary on other architectures, but that's not a concern here.

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Joe Buck]

On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote:
> On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote:
> > Besides, didn't I see a whole bunch of kernel security patches related
> > to null pointer dereferences lately?  If page 0 can be mapped, you
> > suddenly won't get your trap.
> 
> Page 0 can not be mapped on ARM kernels since the late 1990s, and this
> protection is independent of the generic kernel.
> 
> Milage may vary on other architectures, but that's not a concern here.

I don't understand, though, why you would want to implement a generally
useful facility (make the kernel trap so you can do a post-mortem
analysis) in a way that's only safe for the ARM port.

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Russell King - ARM Linux]

On Thu, Dec 17, 2009 at 11:14:01AM -0800, Joe Buck wrote:
> On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote:
> > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote:
> > > Besides, didn't I see a whole bunch of kernel security patches related
> > > to null pointer dereferences lately?  If page 0 can be mapped, you
> > > suddenly won't get your trap.
> > 
> > Page 0 can not be mapped on ARM kernels since the late 1990s, and this
> > protection is independent of the generic kernel.
> > 
> > Milage may vary on other architectures, but that's not a concern here.
> 
> I don't understand, though, why you would want to implement a generally
> useful facility (make the kernel trap so you can do a post-mortem
> analysis) in a way that's only safe for the ARM port.

The discussion at hand is about code contained in the ARM supporting
files (arch/arm/kernel/traps.c), rather than the generic kernel.

As such, it is not relevant to any architecture other than ARM.

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[David Daney]

Joe Buck wrote:
> On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote:
>> On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote:
>>> Besides, didn't I see a whole bunch of kernel security patches related
>>> to null pointer dereferences lately?  If page 0 can be mapped, you
>>> suddenly won't get your trap.
>> Page 0 can not be mapped on ARM kernels since the late 1990s, and this
>> protection is independent of the generic kernel.
>>
>> Milage may vary on other architectures, but that's not a concern here.
> 
> I don't understand, though, why you would want to implement a generally
> useful facility (make the kernel trap so you can do a post-mortem
> analysis) in a way that's only safe for the ARM port.
> 

Each Linux kernel architecture has in its architecture specific bug.h an 
implementation that is deemed by the architecture maintainers to work. 
As far as I know, few if any of these use __builtin_trap().

Some could be converted to __builtin_trap(), others cannot (x86 for 
example).  If we enhanced __builtin_trap() to take an argument for the 
trap code, MIPS could be converted.  But as it stands now 
__builtin_trap() is not very useful.

As more architectures start adding funky tables that get generated by 
the inline asm (as in x86), __builtin_trap() becomes less useful.

David Daney

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Jamie Lokier]

Joe Buck wrote:
> On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote:
> > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote:
> > > Besides, didn't I see a whole bunch of kernel security patches related
> > > to null pointer dereferences lately?  If page 0 can be mapped, you
> > > suddenly won't get your trap.
> > 
> > Page 0 can not be mapped on ARM kernels since the late 1990s, and this
> > protection is independent of the generic kernel.
> > 
> > Milage may vary on other architectures, but that's not a concern here.

It does not trap on at least one ARM-nommu kernel...

> I don't understand, though, why you would want to implement a generally
> useful facility (make the kernel trap so you can do a post-mortem
> analysis) in a way that's only safe for the ARM port.

The patch under discussion, which led to these questions and Cc
gcc@gcc.gnu.org, is specific to the ARM architecture and that is
Russell's focus, as ARM kernel maintainer.

But yes, the whole principle of how to trap and whether it's safe to
follow a null pointer dereference with __builtin_unreachable() applies
to all the other architectures too.

-- Jamie

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Russell King - ARM Linux]

On Thu, Dec 17, 2009 at 07:38:26PM +0000, Jamie Lokier wrote:
> Joe Buck wrote:
> > On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote:
> > > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote:
> > > > Besides, didn't I see a whole bunch of kernel security patches related
> > > > to null pointer dereferences lately?  If page 0 can be mapped, you
> > > > suddenly won't get your trap.
> > > 
> > > Page 0 can not be mapped on ARM kernels since the late 1990s, and this
> > > protection is independent of the generic kernel.
> > > 
> > > Milage may vary on other architectures, but that's not a concern here.
> 
> It does not trap on at least one ARM-nommu kernel...

I was going to say the following in a different reply but discarded it
because it wasn't relevant to the GCC list.

I regard ARM nommu as highly experimental, especially as the maintainer
vanished half way through merging it into mainline.  I know that there
are some parts of ARM nommu that are highly buggy - such as ARM940
support invalidating the entire data cache on any DMA transaction...
say goodbye stacked return addresses.

As such, I would not be surprised if the ARM nommu kernel has _lots_ of
weird and wonderful bugs.  I am not surprised that NULL pointer dereferences
don't work - its actually something I'd expect given that they have a
protection unit which the kernel doesn't apparently touch.

Maybe the protection unit code never got merged?  I've no idea.  As I
say, uclinux support got as far as being half merged and that's roughly
the state it's remained in ever since.

We don't even have any no-MMU configurations which the kernel builder
automatically tests for us.

Given the lack of progress/bug reporting on ARM uclinux, the lack of
platform support and the lack of configurations, my view is that there
is no one actually using it.  I know that I don't particularly take any
care with respect to uclinux when making changes to the MMU based kernels.
Why bother if apparantly no one's using it?

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Russell King - ARM Linux]

On Thu, Dec 17, 2009 at 07:48:37PM +0000, Russell King - ARM Linux wrote:
> Given the lack of progress/bug reporting on ARM uclinux, the lack of
> platform support and the lack of configurations, my view is that there
> is no one actually using it.  I know that I don't particularly take any
> care with respect to uclinux when making changes to the MMU based kernels.
> Why bother if apparantly no one's using it?

Jamie,

As you seem to be a user of uclinux on ARM, could you help ensure that
the support there is bug fixed and we at least have a configuration file
which kautobuild can use to provide feedback on the build status of
uclinux on ARM please?

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Jamie Lokier]

Russell King - ARM Linux wrote:
> Let me put it another way: I want this function to terminate with an
> explicit NULL pointer dereference in every case.

__builtin_trap cannot be used because the GCC manual says "The
mechanism used may vary from release to release so you should not rely
on any particular implementation".  It includes calling abort() as a
possible implementation - not ideal.

This is not related to GCC, but I have an ARM system here where
dereferencing NULL does not trap.  You guessed it, it doesn't have a
regular MMU.  But there are other addresses which do trap.  They would
be a much better choice for BUG().

(Aha!  Maybe that's why the kernel just behaves weirdly when it runs
out of memory and eventually triggers a watchdog reboot, with no panic
message.  I'd better try changing BUG() :-)

Even with an MMU, sometimes userspace maps page zero.  For example,
Wine on x86 does that.  (It might be possible to change Wine and
kernel vm86 to avoid it, but it has not happened).

Bug-free userspace behaviour should not stop kernel's BUG() from doing
it's basic job of trapping in the kernel!

It would be quite messy if userspace maps page zero, and then a kernel
BUG() ploughs ahead into __builtin_unreachable() and completely
undefined behaviour, possibly leading to worse behaviour than omitting
the check which called BUG().

Under those circumstances I'd rather see it use __builtin_trap() if
that really does trap :-)

The point of the exercise with __builtin_unreachable() is to reduce
the kernel size by removing the for(;;) loop.  *(int *)0 = 0 isn't the
smallest trapping sequence.  (When it works :-)

So the most compact _and_ reliable sequence for the kernel, on all
architectures, is probably:

    __asm__ volatile("smallest undefined or always-trapping instruction")

followed by __builtin_unreachable(), because GCC documentation _does_
say that asm followed by that will execute the asm and assume the asm
doesn't return.

-- Jamie

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Richard Henderson]

On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote:
> How is "size-optimal trap" defined?

E.g. Sparc and MIPS have "tcc" instructions that trap based on the
condition codes, and so we eliminate the branch.  That's the only
optimization we apply with __builtin_trap.

> Let me put it another way: I want this function to terminate with an
> explicit NULL pointer dereference in every case.

Then just use that.


r~

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Russell King - ARM Linux]

On Mon, Dec 21, 2009 at 11:30:43AM -0800, Richard Henderson wrote:
> On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote:
>> How is "size-optimal trap" defined?
>
> E.g. Sparc and MIPS have "tcc" instructions that trap based on the
> condition codes, and so we eliminate the branch.  That's the only
> optimization we apply with __builtin_trap.
>
>> Let me put it another way: I want this function to terminate with an
>> explicit NULL pointer dereference in every case.
>
> Then just use that.

That's precisely what we have been using for many years.

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Dave Korn]

Russell King - ARM Linux wrote:
> On Mon, Dec 21, 2009 at 11:30:43AM -0800, Richard Henderson wrote:
>> On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote:
>>> How is "size-optimal trap" defined?
>> E.g. Sparc and MIPS have "tcc" instructions that trap based on the
>> condition codes, and so we eliminate the branch.  That's the only
>> optimization we apply with __builtin_trap.
>>
>>> Let me put it another way: I want this function to terminate with an
>>> explicit NULL pointer dereference in every case.
>> Then just use that.
> 
> That's precisely what we have been using for many years.

  I don't understand.  It should still work just fine; the original version
posted appears to simply lack 'volatile' on the (int *) cast.

    cheers,
      DaveK

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Russell King - ARM Linux]

On Tue, Dec 22, 2009 at 02:09:02PM +0000, Dave Korn wrote:
> Russell King - ARM Linux wrote:
> > On Mon, Dec 21, 2009 at 11:30:43AM -0800, Richard Henderson wrote:
> >> On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote:
> >>> How is "size-optimal trap" defined?
> >> E.g. Sparc and MIPS have "tcc" instructions that trap based on the
> >> condition codes, and so we eliminate the branch.  That's the only
> >> optimization we apply with __builtin_trap.
> >>
> >>> Let me put it another way: I want this function to terminate with an
> >>> explicit NULL pointer dereference in every case.
> >> Then just use that.
> > 
> > That's precisely what we have been using for many years.
> 
>   I don't understand.  It should still work just fine; the original version
> posted appears to simply lack 'volatile' on the (int *) cast.

Neither do I - AFAIK the existing code works fine.

I think this is just a noisy thread about people wanting to use the
latest and greated compiler "features" whether they make sense to or
not, and this thread should probably die until some problem has actually
been identified.

If it ain't broke, don't fix.

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Dave Korn]

Russell King - ARM Linux wrote:

> [ ... ] this thread should probably die until some problem has actually
> been identified.
> 
> If it ain't broke, don't fix.

  <g>  Couldn't agree more.  Happy Christmas!

    cheers,
      DaveK

Re: [PATCH] ARM: Convert BUG() to use unreachable()

[Paolo Bonzini]

On 12/17/2009 06:17 PM, Richard Guenther wrote:
> It shouldn't as *(int *)0 = 0; might trap.  But if you want to be sure
> use
>     __builtin_trap ();
> instead for the whole sequence (the unreachable is implied then).
> GCC choses a size-optimal trap representation for your target then.

Agree that it shouldn't but just to be sure I'd use

   *(volatile int *)0 = 0;
   unreachable ();

Paolo