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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D105338#2865215 <https://reviews.llvm.org/D105338#2865215>, @chandlerc wrote:

> In D105338#2864865 <https://reviews.llvm.org/D105338#2864865>, @spatel wrote:
>
>> In D105338#2863990 <https://reviews.llvm.org/D105338#2863990>, @chandlerc wrote:
>>
>>> But just reverting this is pretty hostile unless Clang has been updated to make C and C++ volatile stores continue to behave as the language expects first.
>>>
>>> Just regressing Clang because no one has written the LangRef change doesn't make sense to me. I think you should fix Clang to not be buggy first if you want to pursue this (or work with the Clang devs to get such a patch), just like we would fix a buggy LLVM pass that broke real users before enabling a valid optimization that uncovered the big.
>>
>> I agree. I don't know exactly what fixes are needed for Clang, but we do know that code like this:
>> https://github.com/llvm/llvm-project/blob/8cf60e61e7b0e3213b5b81039878dcf1ef18c00f/compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp#L254
>> ( the root cause of https://llvm.org/PR47480 ) is going to break again with this change.
>>
>> If the goal is to get source like that to change, then we might as well fix that first, so we can point to it as a reference example of how to fix code?
>
> I really don't know how to change that code in a meaningful way. It is already marking the store as volatile (in the C sense).

For example by using `__builtin_trap()`.

> I would expect this to require extensive changes to kernel code for example, and I'm not sure there would be any appetite for those changes.

Either that, or someone would have to come up with an RFC to carve out an exception specifically for volatile store to null when null pointer is defined.

> Even LLVM's own blog contains advice to use the code pattern that GWP-ASan uses: 'If you're using an LLVM-based compiler, you can dereference a "volatile" null pointer to get a crash if that's what you're looking for [...]' (http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html), and while it is talking about null pointers, the prior section seems to make it very clear this is also how to get a crash from something like a guard page which is the specific use case in question.

There has been a very lengthy disscussion originating from this very message starting at https://reviews.llvm.org/D87149#2264099


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338