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

[George Balatsouras via Phabricator via llvm-commits]

gbalats added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1219
+  (void)Mod->getOrInsertGlobal(
+      "__dfsan_shadow_width_bits", PrimitiveShadowTy, [&] {
+        auto *gv = new GlobalVariable(
----------------
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).


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1229
+  (void)Mod->getOrInsertGlobal(
+      "__dfsan_shadow_width_bytes", PrimitiveShadowTy, [&] {
+        auto *gv = new GlobalVariable(
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97409