[PATCH] D115167: [DSE] Use precise loc for memset_chk writing to local objects.

Wed Dec 15 14:25:58 PST 2021

nikic added inline comments.

================
Comment at: llvm/lib/Analysis/MemoryLocation.cpp:256
       if (const auto *Len = dyn_cast<ConstantInt>(Call->getArgOperand(2))) {
         // memset_chk writes at most Len bytes. It may write less, if Len
         // exceeds the specified max size and aborts.
----------------
nikic wrote:
> fhahn wrote:
> > I now moved the code, but I'm not sure how to adjust/frame the comment here. 
> memset_chk will either write exactly Len bytes or abort the process. If memset_chk aborts, then the partial write cannot be observed. For analysis purposes we can assume that exactly Len bytes are written.
Okay, I'm having second thoughts here, maybe we should go back to your previous version.

The main danger is that something decides that because the precise location is larger than the underlying object, the access must be UB. And indeed, this is exactly what we're doing in https://github.com/llvm/llvm-project/blob/15c8b8ad85c196df25aa3c22bca3d8458f86525a/llvm/lib/Analysis/BasicAliasAnalysis.cpp#L1604-L1613. My earlier statement that this would be fine for alias analysis was wrong.

Though I think that this may be problematic for DSE for the same reason. We pass the location to AA and it could determine that it thus doesn't alias. The saving grace here is probably that https://github.com/llvm/llvm-project/blob/3d595eccc7d5b20d9f202492bf48394ac7c078c6/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp#L926-L935 only uses the NoAlias result if the underlying objects differ, but one could probably still get an incorrect NoAlias result here if the underlying object is hidden in a way that BasicAA understands but DSE does not (e.g. through a phi or select).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115167