[PATCH] D97570: [dfsan] Propagate origin tracking at load

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

stephan.yichao.zhao added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:579
   Value *combineOperandShadows(Instruction *Inst);
-  Value *loadShadow(Value *ShadowAddr, uint64_t Size, uint64_t Align,
-                    Instruction *Pos);
+  std::pair<Value *, Value *> loadShadowOrigin(Value *ShadowAddr, uint64_t Size,
+                                               Align InstAlignment,
----------------
gbalats wrote:
> I think it would be cleaner if we retained the older function (with its older return value) and introduced a new one:
> loadShadowAndOrigin
That would be great. I had also considered this. 
At some points, loading origins depends on loading shadows. 
One is loadFast16ShadowFast, using both shadows and origins to create some select instructions.
The other is  by DFSanLoadLabelAndOriginFn.
So it is not straightforward to call loadShadow from loadShadowAndOrigin. 


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1946
   }
-  return IRB.CreateTrunc(CombinedWideShadow, DFS.PrimitiveShadowTy);
+  return {IRB.CreateTrunc(CombinedWideShadow, DFS.PrimitiveShadowTy),
+          combineOrigins(Shadows, Origins, Pos,
----------------
gbalats wrote:
> Add assertion before returning to check that !ShouldTrackOrigins => Shadows.empty() && Origins.empty()
combineOrigins has a similar assert. Added a "if (ShouldTrackOrigins) {" here to improve readability.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2027
+
+  // Non-scaped load.
   if (AllocaInst *AI = dyn_cast<AllocaInst>(Addr)) {
----------------
morehouse wrote:
> What does "non-scaped" mean?
replaced by non-escaped, for alloca pointers are not used outside the function.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2078
+  // Other cases that support loading shadows or origins in a fast way.
+  Value *ShadowAddr = nullptr, *OriginAddr = nullptr;
+  std::tie(ShadowAddr, OriginAddr) =
----------------
gbalats wrote:
> Is the initialization required here given the next line?
Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97570