Re: [PATCH] Improve DSE to handle stores before __builtin_unreachable ()

[Richard Biener <rguenther@suse.de>]

On Wed, 21 Jun 2023, Jeff Law wrote:

> 
> 
> On 6/21/23 00:41, Richard Biener wrote:
> >> I thought during the introduction of erroneous path isolation that we
> >> concluded stores, calls and such had observable side effects that must be
> >> preserved, even when we hit a block that leads to __builtin_unreachable.
> > 
> > Indeed, I remember we repeatedly hit this in the past.  But
> > double-checking I see that we instrument
> > 
> >    if (x)
> >      *(int *)0 = 0;
> > 
> > as
> > 
> >    <bb 2> [local count: 1073741824]:
> >    if (x_2(D) != 0)
> >      goto <bb 3>; [50.00%]
> >    else
> >      goto <bb 4>; [50.00%]
> > 
> >    <bb 3> [local count: 536870913]:
> >    MEM[(int *)0B] ={v} 0;
> >    __builtin_trap ();
> > 
> > path isolation doesn't seem to use __builtin_unreachable ().  I did
> > not add __builtin_trap () as possible sink (but I did want to treat
> > __builtin_unreachable () and __builtin_unreachable_trap () the same
> > way).  The pass also marks the offending store as volatile.
> Nuts.  I mixed up trap vs unreachable in my own head.  Though I think for the
> purposes of this issue they should be treated the same.  The only difference
> is one actively halts the code, the other silently lets it keep going.

I think there's a difference in that __builtin_trap () is observable
while __builtin_unreachable () is not and reaching __builtin_unreachable 
() invokes undefined behavior while reaching __builtin_trap () does not.

So the isolation code marking the trapping code volatile should be
enough and the trap () is just there to end the basic block
(and maybe be on the safe side to really trap).

Richard.

> > So yes, I think preserving the original trap kind (if there is any)
> > is important and it still seems to work.  I don't remember whether
> > we have any test coverage for that though.  I'll also note that
> > __builtin_trap () has virtual operands (def and use) while
> > __builtin_unreachable[_trap] () are 'const'.  Honza correctly
> > says they should probably be novops instead of 'const' preserving
> > the fact that they have side-effects.
> If we have test coverage it's probably minimal -- a few things to validate
> proper behavior around builtin_trap plus whatever regressions came up.
> 
> 
> > I think it's desirable for assertions.  Since we elide plain
> > __builtin_unreachable () and fall thru whereever it leads that
> > shouldn't be an issue.
> > 
> > If I manually add a __builtin_unreachable () to the above case
> > I see the *(int *)0 = 0; store DSEd.  Maybe we should avoid
> > removing stores that might trap here?  POSIX wise such a trap
> > could be a way to jump out of the path leading to unreachable ()
> > via siglongjmp ...
> Yea, removing the store seemswrong .  As you note, someone could have a
> handler to catch the store, then longjump elsewhere to continue some kind of
> sensible execution.
> 
> The erroneous path isolation bits have some code to try and clean up the bogus
> path a bit.  Ideally leaving a single statement with undefined beahvior and
> the trap.  I wonder if there's any code you could re-use from that effort.
> 
> Essentially a mini pass that cleans up paths post-dominated by a
> builtin_unreachable or builtin_trap.