[PATCH] D101584: [dfsan] Fix origin tracking for fast8

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

stephan.yichao.zhao added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:505
   void injectMetadataGlobals(Module &M);
+  Value *loadNextOrigin(Instruction *Pos, Align OriginAlign,
+                        Value **OriginAddr);
----------------
-> loadNextOriginAndIncAddr or a similar name? So a reader can follow the main code w/o looking into the method.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2152
+          IRB.CreateShl(CombinedWideShadow,
+                        ConstantInt::get(DFS.IntptrTy, WideShadowBitWidth / 2));
+      Value *SecondOrigin = DFS.loadNextOrigin(Pos, OriginAlign, &OriginAddr);
----------------
The [[ https://releases.llvm.org/2.6/docs/LangRef.html#i_shl | LangRef ]] needs both operands have the same type. 
I guess on a 64bit system, this is fine. But making this one have type WideShadowTy would make them always consistent.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2191
+        Shadows.push_back(NextWideShadow);
+        Origins.push_back(DFS.loadNextOrigin(Pos, OriginAlign, &OriginAddr));
+      }
----------------
If we lift this origin load out of the if-else, this if-else could be shared with the if-else before the for-loop, so the comments and the assertion assert(BytesPerWideShadow == 8); will also be shared.

```
FirstOrigin = DFS.loadNextOrigin(Pos, OriginAlign, &OriginAddr);
if () {
   ...
   Origins.push_back(FirstOrigin)
} else {
   ...
   Origins.push_back(FirstOrigin)
}

```


================
Comment at: llvm/test/Instrumentation/DataFlowSanitizer/origin_load.ll:258
   ; CHECK-NEXT:            %[[#WIDE_SHADOW:]] = load i64, i64* %[[#WIDE_SHADOW_PTR]], align [[#SBYTES]]
+  ; CHECK8-NEXT:           %[[#WIDE_SHADOW_LO:]] = shl i64 %[[#WIDE_SHADOW]], 32
+  ; CHECK8-NEXT:           %[[#ORIGIN_PTR2:]] = getelementptr i32, i32* %[[#ORIGIN_PTR]], i64 1
----------------
If WIDE_SHADOW_LO is named to be WIDE_SHADOW_1; and WIDE_SHADOW is named WIDE_SHADOW_12; and ORIGIN is named ORIGIN1; 
 and we do not reuse ORIGIN, but named new assignment to ORIGIN as ORIGIN23 if it is a select from ORIGIN2 and ORIGIN3, it helps read the code. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101584