On Tue, 20 Jun 2023, Jeff Law wrote:
>
>
> On 6/20/23 00:59, Richard Biener via Gcc-patches wrote:
> > DSE isn't good at identifying program points that end lifetime
> > of variables that are not associated with virtual operands. But
> > at least for those that end basic-blocks we can handle the simple
> > case where this ending is in the same basic-block as the definition
> > we want to elide. That should catch quite some common cases already.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >
> > As you can see from the testcase I had to adjust this possibly can
> > lead to more severe issues when one forgets a return (the C++ frontend
> > places builtin_unreachable () there). I'm still planning to push
> > this improvement unless I hear objections.
> >
> > Thanks,
> > Richard.
> >
> > * tree-ssa-dse.cc (dse_classify_store): When we found
> > no defs and the basic-block with the original definition
> > ends in __builtin_unreachable[_trap] the store is dead.
> >
> > * gcc.dg/tree-ssa/ssa-dse-47.c: New testcase.
> > * c-c++-common/asan/pr106558.c: Avoid undefined behavior
> > due to missing return.
> 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.
We do have testsuite coverage that this happens (dump-scanning,
not runtime it seems).
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.
> Don't get me wrong, I'm all for removing the memory references if it's safe to
> do so.
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 ...
Thanks,
Richard.
