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

[George Balatsouras via Phabricator via llvm-commits]

gbalats 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,
----------------
I think it would be cleaner if we retained the older function (with its older return value) and introduced a new one:
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,
----------------
Add assertion before returning to check that !ShouldTrackOrigins => Shadows.empty() && Origins.empty()


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1947-1948
+  return {IRB.CreateTrunc(CombinedWideShadow, DFS.PrimitiveShadowTy),
+          combineOrigins(Shadows, Origins, Pos,
+                         ConstantInt::getSigned(IRB.getInt64Ty(), 0))};
 }
----------------
Shouldn't this be guarded by ShouldTrackOrigins flag? Even if it turns out to be a noop (when Shadows, Origins is empty), it's a little confusing for the reader.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2035-2037
+      return {ShadowLI, OI != AllocaOriginMap.end()
+                            ? IRB.CreateLoad(DFS.OriginTy, OI->second)
+                            : nullptr};
----------------
Could instead use the flag, based on the assertion above.


================
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) =
----------------
Is the initialization required here given the next line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97570