[PATCH] D97789: [dfsan] Propagate origin tracking at store

[stephan.yichao.zhao via Phabricator via llvm-commits]

stephan.yichao.zhao marked 11 inline comments as done.
stephan.yichao.zhao added a comment.

In D97789#2600830 <https://reviews.llvm.org/D97789#2600830>, @morehouse wrote:

> How many patches are left for origin tracking?
>
> When can we start adding end-to-end tests in compiler-rt?

We are able to add end-to-end tests after this one because ld/st are common.

After this one, the left patches are

1. adding origin tracking report and the set of end-to-end tests the current instrumentation can support
2. support phi and memory transfer instructions
3. those custom function extensions to support origin. This could be split into 2-3.



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:611
 
+  void incNumOfOriginStores();
+
----------------
morehouse wrote:
> Do we need this function at all?  Maybe just inline the increment at the one place we do it.
removed the function.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2167
+  if (IntptrSize == OriginSize)
+    return Origin;
+  assert(IntptrSize == OriginSize * 2);
----------------
morehouse wrote:
> Do we still need to do an intptr cast here?
Just checked DataLayout::getIntPtrType from llvm/lib/IR/DataLayout.cpp. It returns an int type.
So the CreateIntCast is more like adjusting bitwidth by ext or trunc. It is fine if the code returns Origin directly.




================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2313
 
-  const Align Alignment = ClPreserveAlignment ? SI.getAlign() : Align(1);
 
----------------
morehouse wrote:
> We no longer honor `ClPreserveAlignment` here.  TBH I don't know what the original purpose of the flag is, but why are we OK to remove this?
We moved this into getShadowAlign. So both load and store, and other related code can share them.


================
Comment at: llvm/test/Instrumentation/DataFlowSanitizer/origin_ldst.ll:294
+  store i16 1, i16* %p
+  ret i16* %p
+}
----------------
morehouse wrote:
> This is a dangling pointer to `p`.  In real code this would be a bug.
> 
> I'm not sure if its undefined behavior or not;  if it is, the compiler could optimize the "escape" away.
Thank you. Replaced ret %p by foo(%p).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97789