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

Tue Dec 7 08:43:57 PST 2021

fhahn added a comment.

In D115167#3174188 <https://reviews.llvm.org/D115167#3174188>, @efriedma wrote:

> A couple thoughts:

Thanks for taking a look!

> 1. We need to be careful using the exact memset size.  We could end up with an "impossible" memory location: for example, setting 100 bytes of a 50-byte buffer.  That's probably okay here?  But maybe not in other contexts; we need to be sure we don't treat memset_chk as UB.  Maybe worth a comment describing what, exactly, we're modeling.

I think using an 'impossible' memory location should be fine here, we use the location to remove stores based on the fact that the `memset_chk` is executed and impossible locations will cause an abort. It's a bit of a shame that after all the work of moving most location reasoning from DSE to MemoryLocation we still need special logic here that doesn't fit into MemoryLocation. But as you said, it is not safe in general.

> 2. In the case where the bounds check in memset_chk fails, it just aborts the process, and there isn't any synchronization point before it does that.  So I'm not sure why it matters if the pointer is visible to other threads.  Another thread can't read the contents before the process abort because it would be unsynchronized, and the memory doesn't exist after the process aborts.

It might have been overly conservative on my side. I was thinking about cases where a failing `memset_chk` aborts the current thread/process but not other threads or  processes that may access the same (shared) memory. But if that's not an issue the restriction can be removed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115167