[PATCH] D143530: [SanitizerBinaryMetadata] Make module_[cd]tor external

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp:227
     }
     appendToGlobalCtors(Mod, Ctor, kCtorDtorPriority, CtorData);
     appendToGlobalDtors(Mod, Dtor, kCtorDtorPriority, DtorData);
----------------
melver wrote:
> 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.)
`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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143530