[dfsan] Record dfsan metadata in globals

[gbalats]

Summary

This will allow identifying exactly how many shadow bytes were used
during compilation, for when fast8 mode is introduced.

Also, it will provide a consistent matching point for instrumentation
tests so that the exact llvm type used (i8 or i16) for the shadow can
be replaced by a pattern substitution. This is handy for tests with
multiple prefixes.

[stephan.yichao.zhao]

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

1273

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)?

1283

Do we need both dfsan_shadow_width_bits and dfsan_shadow_width_bytes? __dfsan_shadow_width_bits seems sufficient.

[gbalats]

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

1273

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).

1283

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.

[gbalats]

Fixed variable naming

[stephan.yichao.zhao]

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

1273

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.

1283

This option -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.

[gbalats]

Switch to weak_odr linkage

[gbalats]

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

1273

Changed to weak ODR linkage. For the rest, see comment below.

1283

After offline discussion, switched to weak odr linkage which should remove the overhead of the two global flags to avoid specifying -D options to each RUN line.

[stephan.yichao.zhao]

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

1273

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.

[gbalats]

Add comment

[morehouse]

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

1278

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?

[gbalats]

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

1278

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).

[morehouse]

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

1278

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.

[gbalats]

Use int32 instead of shadow type for dfsan metadata.

[gbalats]

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

1278

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.

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp

class DataFlowSanitizer {
   WrapperKind getWrapperKind(Function *F);
   void addGlobalNamePrefix(GlobalValue *GV);
   Function *buildWrapperFunction(Function *F, StringRef NewFName,
                                  GlobalValue::LinkageTypes NewFLink,
                                  FunctionType *NewFT);
   Constant *getOrBuildTrampolineFunction(FunctionType *FT, StringRef FName);
   void initializeCallbackFunctions(Module &M);
   void initializeRuntimeFunctions(Module &M);
+  void injectMetadataGlobals(Module &M);
 
   bool init(Module &M);
 
   /// Returns whether the pass tracks origins. Support only fast16 mode in TLS
   /// ABI mode.
   bool shouldTrackOrigins();
 
   /// Returns whether the pass tracks labels for struct fields and array
void DataFlowSanitizer::initializeCallbackFunctions(Module &M) {
   DFSanStoreCallbackFn = Mod->getOrInsertFunction("__dfsan_store_callback",
                                                   DFSanLoadStoreCallbackFnTy);
   DFSanMemTransferCallbackFn = Mod->getOrInsertFunction(
       "__dfsan_mem_transfer_callback", DFSanMemTransferCallbackFnTy);
   DFSanCmpCallbackFn =
       Mod->getOrInsertFunction("__dfsan_cmp_callback", DFSanCmpCallbackFnTy);
 }
 
+void DataFlowSanitizer::injectMetadataGlobals(Module &M) {
+  // These variables can be used:
+  // - by the runtime (to discover what the shadow width was, during
+  //   compilation)
+  // - in testing (to avoid hardcoding the shadow width and type but instead
+  //   extract them by pattern matching)
+  Type *IntTy = Type::getInt32Ty(*Ctx);
+  (void)Mod->getOrInsertGlobal("__dfsan_shadow_width_bits", IntTy, [&] {
+    return new GlobalVariable(
+        M, IntTy, /*isConstant=*/true, GlobalValue::WeakODRLinkage,
+        ConstantInt::get(IntTy, ShadowWidthBits), "__dfsan_shadow_width_bits");
+  });
+  (void)Mod->getOrInsertGlobal("__dfsan_shadow_width_bytes", IntTy, [&] {
+    return new GlobalVariable(M, IntTy, /*isConstant=*/true,
+                              GlobalValue::WeakODRLinkage,
+                              ConstantInt::get(IntTy, ShadowWidthBytes),
+                              "__dfsan_shadow_width_bytes");
+  });
+}
+
 bool DataFlowSanitizer::runImpl(Module &M) {
   init(M);
 
   if (ABIList.isIn(M, "skip"))
     return false;
 
   const unsigned InitialGlobalSize = M.global_size();
   const unsigned InitialModuleSize = M.size();
bool DataFlowSanitizer::runImpl(Module &M) {
   (void)Mod->getOrInsertGlobal("__dfsan_track_origins", OriginTy, [&] {
     Changed = true;
     return new GlobalVariable(
         M, OriginTy, true, GlobalValue::WeakODRLinkage,
         ConstantInt::getSigned(OriginTy, shouldTrackOrigins()),
         "__dfsan_track_origins");
   });
 
+  injectMetadataGlobals(M);
+
   ExternalShadowMask =
       Mod->getOrInsertGlobal(kDFSanExternShadowPtrMask, IntptrTy);
 
   initializeCallbackFunctions(M);
   initializeRuntimeFunctions(M);
 
   std::vector<Function *> FnsToInstrument;
   SmallPtrSet<Function *, 2> FnsWithNativeABI;

llvm/test/Instrumentation/DataFlowSanitizer/basic.ll

 ; RUN: opt < %s -dfsan -S | FileCheck %s --check-prefixes=CHECK,CHECK_NO_ORIGIN
 ; RUN: opt < %s -dfsan -dfsan-track-origins=1 -dfsan-fast-16-labels=true -S | FileCheck %s --check-prefixes=CHECK,CHECK_ORIGIN
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; CHECK: @__dfsan_arg_tls = external thread_local(initialexec) global [100 x i64]
 ; CHECK: @__dfsan_retval_tls = external thread_local(initialexec) global [100 x i64]
 ; CHECK: @__dfsan_arg_origin_tls = external thread_local(initialexec) global [200 x i32]
 ; CHECK: @__dfsan_retval_origin_tls = external thread_local(initialexec) global i32
 ; CHECK_NO_ORIGIN: @__dfsan_track_origins = weak_odr constant i32 0
 ; CHECK_ORIGIN: @__dfsan_track_origins = weak_odr constant i32 1
+; CHECK: @__dfsan_shadow_width_bits = weak_odr constant i32 [[#SBITS:]]
+; CHECK: @__dfsan_shadow_width_bytes = weak_odr constant i32 [[#SBYTES:]]
 ; CHECK: @__dfsan_shadow_ptr_mask = external global i64
-; CHECK: declare void @__dfsan_load_callback(i16, i8*)
-; CHECK: declare void @__dfsan_store_callback(i16, i8*)
-; CHECK: declare void @__dfsan_mem_transfer_callback(i16*, i64)
-; CHECK: declare void @__dfsan_cmp_callback(i16)
+; CHECK: declare void @__dfsan_load_callback(i[[#SBITS]], i8*)
+; CHECK: declare void @__dfsan_store_callback(i[[#SBITS]], i8*)
+; CHECK: declare void @__dfsan_mem_transfer_callback(i[[#SBITS]]*, i64)
+; CHECK: declare void @__dfsan_cmp_callback(i[[#SBITS]])
 
 ; CHECK: ; Function Attrs: nounwind readnone
-; CHECK-NEXT: declare zeroext i16 @__dfsan_union(i16 zeroext, i16 zeroext) #0
+; CHECK-NEXT: declare zeroext i[[#SBITS]] @__dfsan_union(i[[#SBITS]] zeroext, i[[#SBITS]] zeroext) #0
 
 ; CHECK: ; Function Attrs: nounwind readnone
-; CHECK-NEXT: declare zeroext i16 @dfsan_union(i16 zeroext, i16 zeroext) #0
+; CHECK-NEXT: declare zeroext i[[#SBITS]] @dfsan_union(i[[#SBITS]] zeroext, i[[#SBITS]] zeroext) #0
 
 ; CHECK: ; Function Attrs: nounwind readonly
-; CHECK-NEXT: declare zeroext i16 @__dfsan_union_load(i16*, i64) #1
+; CHECK-NEXT: declare zeroext i[[#SBITS]] @__dfsan_union_load(i[[#SBITS]]*, i64) #1
 
 ; CHECK: ; Function Attrs: nounwind readonly
-; CHECK-NEXT: declare zeroext i16 @__dfsan_union_load_fast16labels(i16*, i64) #1
+; CHECK-NEXT: declare zeroext i[[#SBITS]] @__dfsan_union_load_fast16labels(i[[#SBITS]]*, i64) #1
 
 ; CHECK: ; Function Attrs: nounwind readonly
 ; CHECK-NEXT: declare zeroext i64 @__dfsan_load_label_and_origin(i8*, i64) #1
 
 ; CHECK: declare void @__dfsan_unimplemented(i8*)
-; CHECK: declare void @__dfsan_set_label(i16 zeroext, i32 zeroext, i8*, i64)
+; CHECK: declare void @__dfsan_set_label(i[[#SBITS]] zeroext, i32 zeroext, i8*, i64)
 ; CHECK: declare void @__dfsan_nonzero_label()
 ; CHECK: declare void @__dfsan_vararg_wrapper(i8*)
 ; CHECK: declare zeroext i32 @__dfsan_chain_origin(i32 zeroext)
 ; CHECK: declare void @__dfsan_mem_origin_transfer(i8*, i8*, i64)
-; CHECK: declare void @__dfsan_maybe_store_origin(i16 zeroext, i8*, i64, i32 zeroext)
+; CHECK: declare void @__dfsan_maybe_store_origin(i[[#SBITS]] zeroext, i8*, i64, i32 zeroext)
 
 define void @foo() {
   ret void
 }