[PATCH] D98734: [dfsan] Add -dfsan-fast-8-labels flag

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

stephan.yichao.zhao added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:33
 /// |       origin       |
 /// +--------------------+ 0x200000008000 (kOriginAddr)
 /// |   shadow memory    |
----------------
Please update this diagram for 8-bit layout.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1030
+    ShadowPtrMask = ClFast8Labels
+                        ? ConstantInt::getSigned(IntptrTy, ~0xE000000000LL)
+                        : ConstantInt::getSigned(IntptrTy, ~0xF000000000LL);
----------------
Like aarch64 it may not be easy to test this. Can we make this as a TODO?


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1864
   IRBuilder<> IRB(Pos);
-  if (ClFast16Labels) {
+  if (ClFast8Labels || ClFast16Labels) {
     CCS.Block = Pos->getParent();
----------------
Since this expression is used multiple times, please create a function.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2013
     Align OriginAlign, Value *FirstOrigin, Instruction *Pos) {
-  // First OR all the WideShadows, then OR individual shadows within the
-  // combined WideShadow. This is fewer instructions than ORing shadows
-  // individually.
+  // First OR all the WideShadows (i.e., 64bit or 32bit shadow chunks) linearly;
+  // then OR individual shadows within the combined WideShadow by binary ORing.
----------------
32bit is a special case when Size is small. Please add some comments here to explain when 32bit is used.
We also ignore cases for loadFast16ShadowFast when ShadowSize is like 12, 20. Please also explain this
design choice in the comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2023
   IRBuilder<> IRB(Pos);
-  Value *WideAddr =
-      IRB.CreateBitCast(ShadowAddr, Type::getInt64PtrTy(*DFS.Ctx));
+  uint64_t ShadowSize = Size * DFS.ShadowWidthBytes;
+  Type *WideShadowTy =
----------------
const


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2025
+  Type *WideShadowTy =
+      ShadowSize <= 4 ? Type::getInt32Ty(*DFS.Ctx) : Type::getInt64Ty(*DFS.Ctx);
+  Value *WideAddr = IRB.CreateBitCast(ShadowAddr, WideShadowTy->getPointerTo());
----------------
We call loadFast16ShadowFast only when Size > 2 and Size % 8 = 0 or Size == 4. So Size is at least 4.
If we assert (ShadowSize >= 4), this avoids reasoning if the code still works when ShadowSize < 4.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2069
   // Fast path for the common case where each byte has identical shadow: load
   // shadow 64 bits at a time, fall out to a __dfsan_union_load call if any
   // shadow is non-equal.
----------------
This also needs to update about when 32bit is used.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2077
 
+  uint64_t ShadowSize = Size * DFS.ShadowWidthBytes;
+  Type *WideShadowTy =
----------------
const


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2211
 
+  // When the byte size is small enough, we optimize the generated instructions.
   switch (Size) {
----------------
something like "load shadow directly without optimizing instrumentation".


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2468
   if (LeftSize >= ShadowVecSize) {
+    assert(ShadowVecSize * DFS.ShadowWidthBits <= 128);
     auto *ShadowVecTy =
----------------
The assert prevents using ShadowBytes > 2.
But not every number less than 128 works.
Maybe calculating  ShadowVecSize = 8bitmode? 64/ShadowWidthBits: 128/ShadowWidthBits; makes this easy to follow.


================
Comment at: llvm/test/Instrumentation/DataFlowSanitizer/array.ll:9
 ; RUN: opt < %s -dfsan -dfsan-fast-16-labels=true -dfsan-debug-nonzero-labels -S | FileCheck %s --check-prefixes=CHECK,DEBUG_NONZERO_LABELS
+; RUN: opt < %s -dfsan -dfsan-fast-8-labels=true -S | FileCheck %s --check-prefixes=CHECK,FAST16
+; RUN: opt < %s -dfsan -dfsan-fast-8-labels=true -dfsan-combine-pointer-labels-on-load=false -S | FileCheck %s --check-prefixes=CHECK,NO_COMBINE_LOAD_PTR
----------------
renamed to FAST?


================
Comment at: llvm/test/Instrumentation/DataFlowSanitizer/struct.ll:9
 ; RUN: opt < %s -dfsan -dfsan-fast-16-labels=true -dfsan-debug-nonzero-labels -S | FileCheck %s --check-prefixes=CHECK,DEBUG_NONZERO_LABELS
+; RUN: opt < %s -dfsan -dfsan-fast-8-labels=true -S | FileCheck %s --check-prefixes=CHECK,FAST16
+; RUN: opt < %s -dfsan -dfsan-fast-8-labels=true -dfsan-combine-pointer-labels-on-load=false -S | FileCheck %s --check-prefixes=CHECK,NO_COMBINE_LOAD_PTR
----------------
FAST16->FAST?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98734