[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:
> > 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.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1229
+  (void)Mod->getOrInsertGlobal(
+      "__dfsan_shadow_width_bytes", PrimitiveShadowTy, [&] {
+        auto *gv = new GlobalVariable(
----------------
gbalats wrote:
> stephan.yichao.zhao wrote:
> > Do we need both __dfsan_shadow_width_bits and __dfsan_shadow_width_bytes? __dfsan_shadow_width_bits seems sufficient.
> We don't need both. But, again, it will make testing easier as there are some patterns that could use expressions that either depend on one or the other, so having both would make the testing code simpler.
This option [[ https://llvm.org/docs/CommandGuide/FileCheck.html#cmdoption-filecheck-d-var | -D ]] may help.
See examples from llvm/test/FileCheck/numeric-expression.txt

If this works, we could keep only one weak global flag to indicate what runtime can do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97409