[PATCH] D97409: [dfsan] Record dfsan metadata in globals

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

stephan.yichao.zhao added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1219
+  (void)Mod->getOrInsertGlobal(
+      "__dfsan_shadow_width_bits", PrimitiveShadowTy, [&] {
+        auto *gv = new GlobalVariable(
----------------
gbalats wrote:
> stephan.yichao.zhao wrote:
> > gbalats wrote:
> > > stephan.yichao.zhao wrote:
> > > > Are these two variables shared by dfsan runtime and each compilation unit?
> > > > Weak leakage is better, it allows all of them 'share' a global: each compilation unit inserts one if users select fast8, and the dfsan runtime reads it and decides what to do.
> > > > See __dfsan_track_origins as an example.
> > > > 
> > > > PrimitiveShadowTy is used for shadow data, while __dfsan_shadow_width_bits is only a flag.
> > > > Can we use IntegerType::get(*Ctx, 8)?
> > > > 
> > > I thought the sharing between compilation units part was satisfied by the use of getOrInsertGlobal and that internal vs external linkage was only relevant as to what compilation units can actually reference that variable (and since we don't need other compilation units to reference it at all, internal linkage was sufficient). Does this mean that this getOrInsertGlobal would actually create one definition per compilation unit with the current code?
> > > 
> > > As for the use of PrimitiveShadowTy, you're right that you could use i8. I opted for the actual shadow type to make pattern matching in tests easier (so that you could extract the type directly from the global).
> > Yeah. Each compilation unit will create one because the instrumentation pass runs per compilation unit.
> > 
> > See the comments below. -D could help test bitwidth at different modes.
> > So here we can have a different type name. I feel i8 may be strange at C level since dfsan.cpp also needs to define it. Maybe i32 is better, in dfsan.cpp, that will be int.
> > 
> > I just noticed __dfsan_track_origins I added also uses OriginTy... This also needs to be fixed.
> Changed to weak ODR linkage. For the rest, see comment below.
Please add some comments for injectMetadataGlobals about how the two variables would be used. For example, one is used by runtime, and both can help testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97409