[PATCH] D109816: [hwasan] also omit safe mem[cpy|mov|set].

[Florian Mayer via Phabricator via llvm-commits]

fmayer added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:789
       return true;
-  } else if (SSI && SSI->accessIsSafe(*Inst)) {
+  } else if (SSI && SSI->accessIsSafe(*Inst) && findAllocaForValue(Ptr)) {
     return true;
----------------
eugenis wrote:
> Is this a separate bugfix?
> 
> Am I right that this is not needed for regular load/store because the argument is required to be 100% traceable to a single alloca, but 2-args memintrinsics are safe if one arg is 100%, and the other is 100% not stack? That does not sound right.
> 
> The comment on accessIsSafe does not even try to cover such cases. Also, I do not see any tests under Analysis/StackSafety for the mixed memintrinsic case.
Before this change, we only checked this for instructions that took a single pointer variable. There were two cases in `accessIsSafe`:

* the pointer is non stack: we never reached this instruction in the stack safety pass, and we return false because we do not find it in the mapping.
* the pointer is potentially stack: we reach this instruction in the pass, and SECV will take care of checking whether we can *only* reach it from the alloca.

Now, for memcpy / memmove there is the extra case that we can have both cases for two arguments. Now the problem becomes if one argument is in range of the alloca, but the other one isn't reachable from any alloca, accessIsSafe will return true.

What I did in this change is clarify the semantics (as they already were) of accessIsSafe in the comment, and defer responsibility to check whether *all* arguments are potentially stack to the caller. As such, we only hit the second case above.


================
Comment at: llvm/test/Instrumentation/HWAddressSanitizer/stack-safety-analysis.ll:132
+  %y = bitcast [10 x i8]* %buf.sroa.1 to i8*
+  call void @llvm.lifetime.start.p0i8(i64 10, i8* nonnull %x)
+  call void @llvm.lifetime.start.p0i8(i64 10, i8* nonnull %y)
----------------
eugenis wrote:
> If lifetimes are irrelevant to a test case, you can just remove them altogether.
> 
The stack safety does look at lifetimes, so to make it more representative I put them there. Either way is fine, WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109816