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

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added a comment.

How many patches are left for origin tracking?

When can we start adding end-to-end tests in compiler-rt?



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:210-211
+// TODO: This default value follows MSan. DFSan may use a different value.
+static cl::opt<int> ClInstrumentationWithCallThreshold(
+    "dfsan-instrumentation-with-call-threshold",
+    cl::desc("If the function being instrumented requires more than "
----------------



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


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:662
+
+  bool shouldInstrumentationWithCall();
+
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:664
+
+  int NumOfOriginStores = 0;
 };
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2167
+  if (IntptrSize == OriginSize)
+    return Origin;
+  assert(IntptrSize == OriginSize * 2);
----------------
Do we still need to do an intptr cast here?


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2174
+void DFSanFunction::paintOrigin(IRBuilder<> &IRB, Value *Origin,
+                                Value *OriginAddr, uint64_t Size,
+                                Align Alignment) {
----------------
I think these names would be clearer.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2187
+    Value *IntptrOrigin = originToIntptr(IRB, Origin);
+    Value *IntptrOriginPtr =
+        IRB.CreatePointerCast(OriginAddr, PointerType::get(DFS.IntptrTy, 0));
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2313
 
-  const Align Alignment = ClPreserveAlignment ? SI.getAlign() : Align(1);
 
----------------
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?


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2322
+void DFSanFunction::storeOrigin(Instruction *Pos, Value *Addr, uint64_t Size,
+                                Value *Shadow, Value *Origin, Value *OriginAddr,
+                                Align InstAlignment) {
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2496-2498
+  if (ShouldTrackOrigins) {
+    Origin = DFSF.combineOrigins(Shadows, Origins, &SI);
+  }
----------------
Nit


================
Comment at: llvm/test/Instrumentation/DataFlowSanitizer/origin_ldst.ll:294
+  store i16 1, i16* %p
+  ret i16* %p
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97789