[PATCH] D28589: [asan] Refactor instrumentation of globals.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

Thanks for working on this!



================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:605-614
+      const SmallVectorImpl<GlobalVariable *> &ExtendedGlobals,
+      const SmallVectorImpl<Constant *> &MetadataInitializers);
+  void InstrumentGlobalsMachO(
+      IRBuilder<> &IRB, Module &M,
+      const SmallVectorImpl<GlobalVariable *> &ExtendedGlobals,
+      const SmallVectorImpl<Constant *> &MetadataInitializers);
+  void InstrumentGlobalsWithMetadataArray(
----------------
These could all take ArrayRefs.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1621-1628
+  unsigned SizeOfGlobalStruct = DL.getTypeAllocSize(Initializer->getType());
+  assert(isPowerOf2_32(SizeOfGlobalStruct) &&
+         "global metadata will not be padded appropriately");
+
+  // The MSVC linker always inserts padding when linking incrementally. We
+  // cope with that by aligning each struct to its size, which must be a power
+  // of two.
----------------
I guess we can move this part into `InstrumentGlobalsCOFF` as a separate functional change.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1654
+  }
+}
+
----------------
.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1899-1900
 
-  // Create calls for poisoning before initializers run and unpoisoning after.
-  if (HasDynamicallyInitializedGlobals)
-    createInitializerPoisonCalls(M, ModuleName);
-
-  // Platforms with a dedicated metadata section don't need to emit any more
-  // code.
-  if (UseComdatMetadata)
-    return true;
+  bool UseCOFFComdatMetadata = TargetTriple.isOSBinFormatCOFF();
+  bool UseMachOGlobalsSection = ShouldUseMachOGlobalsSection();
+  bool UseMetadataArray = !(UseCOFFComdatMetadata || UseMachOGlobalsSection);
----------------
Fold these into their only use.


================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1907-1910
   } else if (UseMetadataArray) {
-    // On platforms that don't have a custom metadata section, we emit an array
-    // of global metadata structures.
-    ArrayType *ArrayOfGlobalStructTy = ArrayType::get(GlobalStructTy, n);
-    AllGlobals = new GlobalVariable(
-        M, ArrayOfGlobalStructTy, false, GlobalVariable::InternalLinkage,
-        ConstantArray::get(ArrayOfGlobalStructTy, Initializers), "");
-  }
-
-  // Create a call to register the globals with the runtime.
-  if (UseMachOGlobalsSection) {
-    IRB.CreateCall(AsanRegisterImageGlobals,
-                   {IRB.CreatePointerCast(RegisteredFlag, IntptrTy)});
+    InstrumentGlobalsWithMetadataArray(IRB, M, NewGlobals, Initializers);
   } else {
+    llvm_unreachable("");
----------------
Why not just
```
} else {
  InstrumentGlobalsWithMetadataArray(IRB, M, NewGlobals, Initializers);
}
```
?


Repository:
  rL LLVM

https://reviews.llvm.org/D28589