[SanitizerBinaryMetadata] Make module_[cd]tor external

[MaskRay]

Summary

If a COMDAT key has a local linkage, it behaves as comdat nodeduplicate and
llvm/lib/Linker/LinkModules.cpp does not deduplicate its members.
This is not intended. Switch to an external linkage to allow deduplication.

See also https://maskray.me/blog/2021-07-25-comdat-and-section-group#grp_comdat

[MaskRay]

Add a source comment

[melver]

Thanks!

[melver]

llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp

227

Is there any usecase where having the COMDAT key internal linkage is valid? Should we add an assert() to appendToGlobalCtors()?

I'm guessing that will uncover a whole bunch of latent issues in other sanitizers as well.

(Unfortunately we can't make the ctor/dtor created in createSanitizerCtor always ExternalLinkage because e.g. ASan does actually need per-TU ctor/dtor for globals in some cases.)

[MaskRay]

llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp

227

comdat nodeduplicate can use a local linkage key. For non-LTO use cases, a local linkage key works since all linkers use the symbol name for deduplication, ignoring the symbol binding. If we make the comdat key hidden, after linking it's similar to a local linkage. I don't have a strong opinion whether IR verifier or another place should reject some cases.

llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp

Function *Dtor =
         createSanitizerCtorAndInitFunctions(
             Mod, (MI->FunctionPrefix + ".module_dtor").str(),
             (MI->FunctionPrefix + "_del").str(), InitTypes, InitArgs,
             /*VersionCheckName=*/StringRef(), /*Weak=*/ClWeakCallbacks)
             .first;
     Constant *CtorData = nullptr;
     Constant *DtorData = nullptr;
     if (TargetTriple.supportsCOMDAT()) {
-      // Use COMDAT to deduplicate constructor/destructor function.
+      // Use COMDAT to deduplicate constructor/destructor function. The COMDAT
+      // key needs to be a non-local linkage.
       Ctor->setComdat(Mod.getOrInsertComdat(Ctor->getName()));
       Dtor->setComdat(Mod.getOrInsertComdat(Dtor->getName()));
+      Ctor->setLinkage(GlobalValue::ExternalLinkage);
+      Dtor->setLinkage(GlobalValue::ExternalLinkage);
       CtorData = Ctor;
       DtorData = Dtor;
     }
     appendToGlobalCtors(Mod, Ctor, kCtorDtorPriority, CtorData);
     appendToGlobalDtors(Mod, Dtor, kCtorDtorPriority, DtorData);
   }
 
   return true;
 }
 
 void SanitizerBinaryMetadata::runOn(Function &F, MetadataInfoSet &MIS) {
   if (F.empty())

llvm/test/Instrumentation/SanitizerBinaryMetadata/ctor.ll

+; RUN: opt < %s -passes='module(sanmd-module)' -sanitizer-metadata-atomics -S | FileCheck %s
+
+; CHECK: $__sanitizer_metadata_atomics.module_ctor = comdat any
+; CHECK: $__sanitizer_metadata_atomics.module_dtor = comdat any
+; CHECK: $__sanitizer_metadata_covered.module_ctor = comdat any
+; CHECK: $__sanitizer_metadata_covered.module_dtor = comdat any
+
+; CHECK: @llvm.used = appending global [4 x ptr] [ptr @__sanitizer_metadata_atomics.module_ctor, ptr @__sanitizer_metadata_atomics.module_dtor, ptr @__sanitizer_metadata_covered.module_ctor, ptr @__sanitizer_metadata_covered.module_dtor], section "llvm.metadata"
+; CHECK: @llvm.global_ctors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 2, ptr @__sanitizer_metadata_atomics.module_ctor, ptr @__sanitizer_metadata_atomics.module_ctor }, { i32, ptr, ptr } { i32 2, ptr @__sanitizer_metadata_covered.module_ctor, ptr @__sanitizer_metadata_covered.module_ctor }]
+; CHECK: @llvm.global_dtors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 2, ptr @__sanitizer_metadata_atomics.module_dtor, ptr @__sanitizer_metadata_atomics.module_dtor }, { i32, ptr, ptr } { i32 2, ptr @__sanitizer_metadata_covered.module_dtor, ptr @__sanitizer_metadata_covered.module_dtor }]
+
+; CHECK: define dso_local void @__sanitizer_metadata_covered.module_ctor() #1 comdat {
+; CHECK: define dso_local void @__sanitizer_metadata_covered.module_dtor() #1 comdat {
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i8 @foo(ptr %a) nounwind uwtable {
+entry:
+  %0 = load atomic i8, ptr %a unordered, align 1
+  ret i8 %0
+}