[PATCH] D105338: [InstCombine] Revert "Temporarily do not drop volatile stores before unreachable"

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

CC @jfb, does C and C++ actually provide the guarantee we are talking about here?

In D105338#2868543 <https://reviews.llvm.org/D105338#2868543>, @rsmith wrote:

> In D105338#2868257 <https://reviews.llvm.org/D105338#2868257>, @lebedev.ri wrote:
>
>> I think we should separate two patterns:
>>
>> 1. `*(volatile int)0 = 0`, aka let's trap
>> 2. `volatile int* ptr; <...> *ptr = x;`
>>
>> Indeed, we can't do anything reasonable with the latter.
>> Langref states that they do not trap, D53184 <https://reviews.llvm.org/D53184>, so the clang is already miscompiling the source C code that expects them to trap.

Actually, i failed to convey the full detail of the problem.
This patch only matters when said volatile store and all the instructions after it must transfer execution to the `unreachable`.
So

  store volatile
  throw 1 ; execution of the normal, non-exception codeflow stops here, unreachable is not reached, execution continues in exception handler
  unreachable

is fine and the store can **not** be dropped, while in

  store volatile
  printf("how did we get here?"); ; nothrow willreturn
  unreachable

both the call and the store will be consumed by the unreachable.

Also, there aren't two patterns, there are three:

1. `*(volatile int)0 = 0`, aka let's trap
2. `volatile int* ptr; <...> *ptr = x; // that's it, we've successfully trapped`, i.e. what we've had in GWP-ASAN in https://github.com/llvm/llvm-project/blob/8cf60e61e7b0e3213b5b81039878dcf1ef18c00f/compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp#L254
3. `volatile int* ptr; <...> *ptr = x; /* did we trap? */ *ptr = y; /* what about now ? */`

Point 1. and point 2. are the same, they are specifically written with the intention of trapping, the only difference is the address.
Point 3 is the problematic one. Does anyone actually expects that *any* volatile store could potentially "arbitrairly" trap? CC @jfb

Point 1. is "addressed" by D105728 <https://reviews.llvm.org/D105728>.
Point 2. potentially needs salvaging in the user code, iff it ends with `unreachable`, by either making it the first snippet, or ensuring that it does not end in unreachable (by e.g. ending with an explicit trap, or with an endless loop that isn't mustprogress)
Point 3 is rather hopeless.

> In the absence of the recent semantics change, are there actually end-to-end miscompilations,

It is easy to construct: https://godbolt.org/z/fKch73G4M
While we have to compute `333 / (42 * width)` before the first volatile store,
we also compute the `666 / (84 * width)` before said first volatile store,
so if the `84 * width` would overflow (resulting in poison as per https://llvm.org/docs/LangRef.html#id108),
then the `666 / (84 * width)` would be UB (as per https://llvm.org/docs/LangRef.html#poison-values)
but if the first volatile store would have trapped, then we would't have experienced an UB,
yet we did. Not sure if i'm mixing up C++ and IR semantics..

> or do you instead mean that Clang is producing IR that the IR semantics don't guarantee to behave as expected (but do in practice)?

Mainly, yes.

> Those are two quite different outcomes, especially given that there's code in our own compiler-rt (and presumably in lots of other places) that assumes that a volatile null dereference gives an actual null dereference even if the null pointer is not "immediate".



>> Unless someone comes forward with an RFC to change the semantics, LLVM will continue to believe that they will not trap.
>
> My question stands: is there a way for a frontend to generate IR that gives the semantics that Clang expects here? Or is the outcome that, after this change to the de facto semantics (and the earlier change to the de jure semantics in D53184 <https://reviews.llvm.org/D53184>), LLVM IR can no longer represent C programs with the semantics that Clang expects?
>
> If there's nothing today, we could presumably add something like an intrinsic that is not guaranteed to return and that reads inaccessible memory only, that gets lowered to a no-op, and Clang could insert a call to that after every volatile load or store. That would seem more robust to me than an ad-hoc syntactic check for only pattern 1, given we know that pattern 2 also exists in the wild, and Clang has promised that both patterns work for many years.

Hm, load too? :] We already happily drop those: https://godbolt.org/z/8Kdo7z3jd


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105338/new/

https://reviews.llvm.org/D105338