[PATCH] D92261: [dfsan] Track field/index-level shadow values in variables

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added a comment.

How does this change affect instrumented binary size?



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:422
+  ///
+  /// getZeroShadow({T1,T2,...}) = {getZeroShadow(T1),getZeroShadow(T1),...}
+  /// getZeroShadow([n x T]) = [n x getZeroShadow(T)]
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:426
+  ///
+  /// Note that in Args mode a zero shadow is always i16(0).
+  Constant *getZeroShadow(Type *OrigTy);
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:436
+  ///
+  /// getShadowTy({T1,T2,...}) = {getShadowTy(T1),getShadowTy(T1),...}
+  /// getShadowTy([n x T]) = [n x getShadowTy(T)]
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:440
+  ///
+  /// Note that in Args mode a shadow type is always i16.
   Type *getShadowTy(Type *OrigTy);
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:473
+  /// terms of domination tree.
+  DenseMap<Value *, CachedShadow> CachedShadow2PrimitiveShadow;
   DenseMap<Value *, std::set<Value *>> ShadowElements;
----------------
Nit:  This is only used for mapping expanded shadow to collapsed shadow.  Maybe `CachedCollapsedShadows` is more descriptive, while being consistent with the naming of `CachedShadows`?


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:506-507
                    Instruction *Pos);
+  /// Returns a shadow value with the original type T. All its primitive sub
+  /// values are assigne to PrimitiveShadow.
+  ///
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:514-515
+                                    Instruction *Pos);
+  /// Returns a primitive shadow value by combining all primitive values of
+  /// Shadow.
+  ///
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:520
+  /// CTP(other types, PS) = PS
+  Value *convertToPrimitiveShadow(Value *Shadow, Instruction *Pos);
 
----------------
Nit:  I think `expandFromPrimitiveShadow` and `collapseToPrimitiveShadow` are more intuitive names.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:529-531
+  /// Returns a primitive shadow value by combining all primitive values of a
+  /// Shadow value with type Struct. This is an auxilary method of
+  /// convertToPrimitiveShadow.
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:534-536
+  /// Returns a primitive shadow value by combining all primitive values of a
+  /// Shadow value with type Array. This is an auxilary method of
+  /// convertToPrimitiveShadow.
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:649-651
+  if (getInstrumentedABI() == DataFlowSanitizer::IA_Args) {
+    return ZeroPrimitiveShadow == V;
+  }
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:664-666
+  if (getInstrumentedABI() == DataFlowSanitizer::IA_Args) {
+    return ZeroPrimitiveShadow;
+  }
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:679
+
+Value *DFSanFunction::convertFromPrimitiveShadowByShadowType(
+    Value *Shadow, SmallVector<unsigned, 4> &Indices, Type *SubShadowTy,
----------------
Nit:  This is really just a recursive helper function.  Maybe a name like `expandFromPrimitiveShadowRecursive` is more descriptive?  Also since it doesn't access any member variables, it could just be a local static function.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:708
+Value *DFSanFunction::convertFromPrimitiveShadowByShadowType(
+    Type *ShadowTy, Value *PrimitiveShadow, Instruction *Pos) {
+  if (!isa<ArrayType>(ShadowTy) && !isa<StructType>(ShadowTy))
----------------
This function is small enough, I think we should just inline it into `convertFromPrimitiveShadow` below.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:746
+    Value *ShadowInner = convertToPrimitiveShadow(ShadowItem, IRB);
+    Aggregator = IRB.CreateOr(Aggregator, ShadowInner);
+  }
----------------
OR only works for fast16 mode.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:765
+  return Aggregator;
+}
+
----------------
`collapseStructShadow` and `collapseArrayShadow` are practically identical.  Can we use a templates or polymorphism to share their implementation?


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:768
+Value *DFSanFunction::convertToPrimitiveShadow(Value *Shadow,
+                                               IRBuilder<> &IRB) {
+  Type *ShadowTy = Shadow->getType();
----------------
Since this function is small, I think we should inline it into `convertToPrimitiveShadow` below.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:788
+  if (CS.Block && DT.dominates(CS.Block, Pos->getParent()) &&
+      DT.dominates(CS.Shadow, Pos))
+    return CS.Shadow;
----------------
This additional check isn't used for the CCS cache.  Why do we need it here?


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:816-817
+      Elements.push_back(getShadowTy(ST->getElementType(i)));
+    StructType *Res = StructType::get(*Ctx, Elements);
+    return Res;
+  }
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:982
+    // F is called by a wrapped custom function with primitive shadows. So
+    // its arguments and return value need convertion.
     DFSanFunction DFSF(*this, F, /*IsNativeABI=*/true);
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1432
+  return convertFromPrimitiveShadow(T, PrimitiveValue, Pos);
+}
+
----------------
Nit:  This function is tiny.  Maybe we should just inline it everywhere instead?


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1438
   if (DFS.isZeroShadow(V1))
-    return V2;
+    return convertToPrimitiveShadow(V2, Pos);
   if (DFS.isZeroShadow(V2))
----------------
Do you plan to make this work without converting everything to primitive shadow?


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1544
+// Addr has alignment Align, and take the union of each of those shadows. The
+// returned shadow is always with primitive types.
 Value *DFSanFunction::loadShadow(Value *Addr, uint64_t Size, uint64_t Align,
----------------



================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1786
+    Shadow = DFSF.combineShadowsThenConvert(SI.getValueOperand()->getType(),
+                                            Shadow, PtrShadow, &SI);
   }
----------------
What's the reason for expanding the shadow when we're about to collapse it again in `storeShadow`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92261