[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:1224
+  (void)Mod->getOrInsertGlobal(
+      "__dfsan_shadow_width_bits", PrimitiveShadowTy, [&] {
+        auto *GV = new GlobalVariable(
----------------
morehouse wrote:
> 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`.
Changed to i32. Updated tests to use SBITS instead.

Regarding the usefulness of the  `__dfsan_shadow_width_bytes`, it is in fact used in tests. Right now it's hardcoded to 2 in several places, like alignments of some alloca/load/store instructions that operate on a shadow. These would be change to [[#SBYTES]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97409