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

Wed Nov 23 14:23:21 PST 2022

fhahn marked an inline comment as done.
fhahn 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:
> 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).
Great point, thanks for checking and sorry for the long delay.

Finally had some time to look at this again. I updated the patch to move the logic back to DSE and just use it for the sizes used to determine whether an object is overwritten. That should avoid any issues with AA queries.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115167