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

[George Balatsouras via Phabricator via llvm-commits]

gbalats added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:746
     const std::vector<std::string> &ABIListFiles) {
+  if (ClFast8Labels && ClFast16Labels) {
+    report_fatal_error(
----------------
stephan.yichao.zhao wrote:
> When releasing fast8labels, we set ClFast8Labels = true by default.
> If users wanted to use 16bit mode by -dfsan-fast-16-labels=true, this assert happens. So they have to do -dfsan-fast-16-labels=true -dfsan-fast-8-labels=false.
> This message can suggest this if we found both are true. Something like:
> "cannot set both -dfsan-fast-8-labels and -dfsan-fast-16-labels. -dfsan-fast-8-labels is true by default. 
> 16mode will be deprecated. To use 16 mode, set -dfsan-fast-16-labels=true -dfsan-fast-8-labels=false."
> 
> Or when -dfsan-fast-16-labels is true, or we found -dfsan-fast-8-labels=false, this code can print out the deprecation warning.
I'm not sure I follow. Did you mean ClFast16Labels perhaps, since the ClFast8Labels has just been introduced by this path? Where is it set? The default value of both flags is false, so the statement "-dfsan-fast-*-labels is true by default" isn't accurate.

I think the deprecation warning should be introduced after fast8 is properly supported by the dfsan runtime.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1030
+    ShadowPtrMask = ClFast8Labels
+                        ? ConstantInt::getSigned(IntptrTy, ~0xE000000000LL)
+                        : ConstantInt::getSigned(IntptrTy, ~0xF000000000LL);
----------------
stephan.yichao.zhao wrote:
> Like aarch64 it may not be easy to test this. Can we make this as a TODO?
So what should the behavior be if one runs on MIPS? I think if we don't change the shadow ptr mask, it won't be correct either so I'm not sure just keeping the older value is preferable. Should we report a fatal error if ClFast8Labels is used on MIPS? I'm not sure how this would play out with continuous integration and if it will introduce failures.


================
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.
----------------
stephan.yichao.zhao wrote:
> 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.
Added comments and assertion.


================
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 =
----------------
stephan.yichao.zhao wrote:
> const
Done. I assume this referred to the ShadowSize variable. Moved to the top.


================
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());
----------------
stephan.yichao.zhao wrote:
> 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.
Added assertion and changed <= to == 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.
----------------
stephan.yichao.zhao wrote:
> This also needs to update about when 32bit is used.
Added comment.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2468
   if (LeftSize >= ShadowVecSize) {
+    assert(ShadowVecSize * DFS.ShadowWidthBits <= 128);
     auto *ShadowVecTy =
----------------
stephan.yichao.zhao wrote:
> 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.
I'm not sure why the suggestion makes this easier to follow. It only makes it easier if you know what the code was before, imo, but I think the original intention was that the vector type should fit in 128 bits regardless of the mode being used (which is what this assertion is trying to enforce).

Removed the assertion to support ShadowBytes > 2.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98734