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.
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.
jeff
