Cleanup: hide undefined behavior from the compiler better.

Cleanup: hide undefined behavior from the compiler better.

Submitting this on behalf of Paul Pluzhnikov.

R=mark@chromium.org

Committed: https://code.google.com/p/google-breakpad/source/detail?r=1342

[Mark Mentovai]

Can we just use __builtin_trap()?

[ppluzhnikov_google.com]

 +// observes that UB statement is reached, it can assume that all statemnts

s/statemnts/statements/

Sorry about the typo in my original.


On Mon, Jun 30, 2014 at 2:22 PM,  <mark@chromium.org> wrote:
> Can we just use __builtin_trap()?

I believe __builtin_trap is the opposite of what we want.

The trouble is this:

   if (condition) {
     statement1;
     statement_with_UB;
   }

GCC assumes that you will never execute statement_with_UB.

If that's true, then you will never execute statement1 either;
therefore it can be completely removed.

Replacing statement_with_UB with __builtin_trap ... doesn't change the
reasoning about statement1. And if statement1 is the one that sets up
crash handler ... you are going to not have a crash handler :-)

I've verified that that is exactly what happens when I replace
"*reinterpret_cast<volatile int*>(NULL) = 0;" with __builtin_trap. In
fact, it makes the test fail under gcc-4.8 as well.

What we want then is to replace statement_with_UB with
statement_that_we_know_will_crash_but_GCC_cant_tell ;-)


-- 
Paul Pluzhnikov

[ivanpe]

On 2014/06/30 22:28:22, ppluzhnikov_google.com wrote:
>  +// observes that UB statement is reached, it can assume that all statemnts
> 
> s/statemnts/statements/
> 
> Sorry about the typo in my original.
> 
> 
> On Mon, Jun 30, 2014 at 2:22 PM,  <mailto:mark@chromium.org> wrote:
> > Can we just use __builtin_trap()?
> 
> I believe __builtin_trap is the opposite of what we want.
> 
> The trouble is this:
> 
>    if (condition) {
>      statement1;
>      statement_with_UB;
>    }
> 
> GCC assumes that you will never execute statement_with_UB.
> 
> If that's true, then you will never execute statement1 either;
> therefore it can be completely removed.
> 
> Replacing statement_with_UB with __builtin_trap ... doesn't change the
> reasoning about statement1. And if statement1 is the one that sets up
> crash handler ... you are going to not have a crash handler :-)
> 
> I've verified that that is exactly what happens when I replace
> "*reinterpret_cast<volatile int*>(NULL) = 0;" with __builtin_trap. In
> fact, it makes the test fail under gcc-4.8 as well.
> 
> What we want then is to replace statement_with_UB with
> statement_that_we_know_will_crash_but_GCC_cant_tell ;-)
> 
> 
> -- 
> Paul Pluzhnikov

Typo is fixed.  Thanks.

[Mark Mentovai]

OK, so if I understand correctly, what’s happening with __builtin_trap() isn’t
so much that GCC is assuming that it won’t execute, it’s that GCC knows that
this statement is fatal, and thus statement1 is irrelevant and optimized out
because the program is going to crash regardless of whether statement1 executes.

Back to the code we’re replacing:

    if (condition) {
      statement1;
      statement_with_UB;
    }

your comments indicate that GCC optimizes away this entire conditional. Is that
true, with the post-fork() child continuing on incorrectly into the codepath
designated for the parent?

[ppluzhnikov_google.com]

-cc google-breakpad-dev to which I can't send.

On Tue, Jul 1, 2014 at 9:13 AM,  <mark@chromium.org> wrote:
> OK, so if I understand correctly, what's happening with __builtin_trap()
> isn't so much that GCC is assuming that it won't execute, it's that GCC
> knows that this statement is fatal,

That is not how GCC behaves, though that looks like a reasonable
interpretation of __builtin_trap documentation.

AFAICT, how GCC actually behaves is:
1. it assumes that __builtin_trap will *not* be executed (i.e. that
the path in control flow graph leading to it will not be taken), and
2. emits a UD2 instruction if the __builtin_trap is ever reached.


> and thus statement1 is irrelevant

The statement1 is irrelevant because GCC assumes that it will never be
reached (there are no branches between statement1 and __builtin_trap;
if we assume that the latter is unreachable, then so is the former). I
believe this is subtly different from what you are saying.

> and optimized out because the program is going to crash regardless of
> whether statement1 executes.
>
> Back to the code we're replacing:
>
>     if (condition) {
>       statement1;
>       statement_with_UB;
>     }
>
> your comments indicate that GCC optimizes away this entire conditional.

I don't see where in my comment I said that.

Only the (entire) body of the if statement is replaced with UD2. The
condition is still evaluated, and the UD2 is jumped over when the
condition is false.

> Is that true, with the post-fork() child continuing on incorrectly into
> the codepath designated for the parent?
>
> https://breakpad.appspot.com/6674002/



-- 
Paul Pluzhnikov

[Mark Mentovai]

Thanks for the explanation. LGTM. I am concerned that this will rear its head
again under LTO, though.

[ppluzhnikov_google.com]

-cc google-breakpad-dev

On Tue, Jul 1, 2014 at 10:01 AM,  <mark@chromium.org> wrote:
> Thanks for the explanation. LGTM. I am concerned that this will rear its
> head again under LTO, though.

Yes, if LTO can deduce that p_null is never assigned to, and if it
chooses to believe that "volatile" can be ignored, then we'll be back
to square one.

I think this possibility is pretty remote (famous last words :-)

Thanks,
-- 
Paul Pluzhnikov

[ivanpe]

Committed patchset #3 manually as r1342 (presubmit successful).

[Ted Mielczarek]

Fancy compilers getting too smart for their own good! If we replaced this with a
block of inline assembly, would that be enough to work around GCC's optimizer?

[ppluzhnikov_google.com]

-cc google-breakpad-dev

On Mon, Jul 7, 2014 at 4:13 AM,  <ted.mielczarek@gmail.com> wrote:
> Fancy compilers getting too smart for their own good! If we replaced
> this with a block of inline assembly, would that be enough to work
> around GCC's optimizer?

Inline assembly would work, but then you would have to implement it
separately for every supported architecture.


-- 
Paul Pluzhnikov