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

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1224
+  (void)Mod->getOrInsertGlobal(
+      "__dfsan_shadow_width_bits", PrimitiveShadowTy, [&] {
+        auto *GV = new GlobalVariable(
----------------
gbalats wrote:
> morehouse wrote:
> > If these globals have `PrimitiveShadowTy` type, will we have to declare multiple weak globals in the runtime (i.e. one that's int8 and one thats int16)?
> > 
> > Why not make it an `int` or `size_t` to simplify the runtime side?
> I'm not yet familiar with how the runtime integration works. Is there a reason why the runtime has to mirror these using exactly the same type instead of e.g. int32?
> 
> The only reason to use `PrimitiveShadowTy` here, instead of `i8`, is to make testing easier so that by matching just one variable one can extract both the `i16` and `16` parts to use later in the test:
> 
> ```
> ; CHECK: @__dfsan_shadow_width_bits = weak_odr constant [[ST:i[0-9]+]] [[#SBITS:]], align {{[1-2]}}
> ```
>  
> Of course, you could use `i[[SBITS]]` instead of `[[ST]]` but it would be cumbersome. Alternatively, we could use a different global to extract the shadow type, though I'm not sure which one (it would have to be generated in all option combinations; perhaps one of the callback functions).
If the linker does not give an explicit error, I believe it is still undefined behavior to link two symbols of the same name with different types.

I would prefer using a single standard type across runtime/instrumentation and extracting the `SBITS` value in tests.

Also, I don't see much use for `__dfsan_shadow_width_bytes` since it isn't used in tests and its value can be easily obtained in the runtime from `__dfsan_shadow_width_bits`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97409