[PATCH] D92261: [dfsan] Track field/index-level shadow values in variables
Matt Morehouse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 7 12:29:37 PST 2020
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
More information about the llvm-commits
mailing list