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

Johannes Doerfert via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 8 14:40:01 PDT 2021


jdoerfert added a comment.

In D105338#2865463 <https://reviews.llvm.org/D105338#2865463>, @spatel wrote:

> In D105338#2865297 <https://reviews.llvm.org/D105338#2865297>, @jdoerfert wrote:
>
>> In D105338#2864865 <https://reviews.llvm.org/D105338#2864865>, @spatel wrote:
>>
>>> 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?
>>
>> We pointed them to a fix (`__builtin_trap()`) a while ago, didn't we?
>
> Yep, and now it's in place with (cc @hctim ):
> D105654 <https://reviews.llvm.org/D105654> / d458f379324967 <https://reviews.llvm.org/rGd458f379324967c3c408be06e21aad9bc92c54cb>
> ...but there's a code comment and comment in PR47480 that says LLVM has a bug. I assume that means the LangRef has a bug, but nobody wants to change it?

It depends on how you look at it. LangRef and blog post disagree, that's for sure. What is "correct" depends now. I would modify the blog post with a big note, add a clang warning, and start to delete the stores.

> As for Clang, I don't know how we'd solve that problem (without changing LLVM) other than to say (loudly) that the blog post no longer applies, and we want code to update. Now we can at least point people to the GWP-asan patch as a reference, but as noted, there is very likely going to be fallout...

Thinking out load here: What about a warning in clang if we store to a 0 literal?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105338



More information about the llvm-commits mailing list