[SanitizerCoverage] Create comdat for global arrays.

[morehouse]

Summary

Place global arrays in comdat sections with their associated functions.
This makes sure they are stripped along with the functions they
reference, even on the BFD linker.

[eugenis]

LGTM
FTR, http://llvm.org/docs/LangRef.html#associated-metadata mentions that comdat is "recommended".
It would be nice to factor this out into a utility function (create associated global for a global) and use it in asan, too.

[eugenis]

Btw what if getUniqueModuleId returns ""? It may happens for files without external symbols.
I think we need to fall back to non-GCed counters.

[morehouse]

• Fallback to counter suffix if getUniqueModuleId fails.

[morehouse]

• Add refactoring TODO.

[morehouse]

• Only set comdats for ELF.
• Fallback to using module ID when getUniqueModuleId fails.

[morehouse]

• Rebase

[morehouse]

• Rebase properly.

[eugenis]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

582 ↗

(On Diff #164958)

I don't think this is correct - getModuleIdentifier() is even less unique then getUniqueModuleId, and comdat names for local linkage functions really have to be unique.

Imagine two files with the same relative path, no exported symbols, defining local functions with the same name. We don't want them to share a comdat.

I think this case needs to fallback to non-comdat non-associated instrumentation (the one that suppressed linker GC).

[morehouse]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

582 ↗

(On Diff #164958)

Ah, I see. Will revert.

Hopefully suppressing GC for that case won't cause any regressions (e.g., https://github.com/google/sanitizers/issues/971).

[morehouse]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

582 ↗

(On Diff #164958)

Hmm... That approach gives me link errors again:

/usr/bin/ld: __sancov_pcs has both ordered [`__sancov_pcs[SSL_COMP_get_name]' in BUILD/libssl.a(ssl_ciph.o)] and unordered [`__sancov_pcs' in BUILD/libssl.a(ssl_ciph.o)] sections

I guess the linker doesn't like having some arrays in comdats and some not in comdats within the same section.

[eugenis]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

582 ↗

(On Diff #164958)

Never seen this error before. Does the linker treat names with a dot as ordered by the part of the name that follows the dot? What if you replace the dot with something else?

[morehouse]

llvm/trunk/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

582 ↗

(On Diff #164958)

I don't think there's any dots in the comdat name or the section name... In the case above the section name is __sancov_pcs and the comdat name is SSL_COMP_get_name.

[morehouse]

• Do not set comdat for local-linkage functions without a unique module ID

[morehouse]

The error was actually caused by setting associated metadata for some arrays and not others. By always setting the metadata, we can avoid the issue.

@eugenis: PTAL

[eugenis]

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

574

Don't call getUniqueModuleId for every function in a module, it's not exactly O(1).

[morehouse]

• Call getUniqueModuleId once per module.

[eugenis]

LGTM, I'm almost 100% sure this is safe, but I don't remember all the linker quirks around associated sections now.

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

private:
   std::pair<GlobalVariable *, GlobalVariable *>
   CreateSecStartEnd(Module &M, const char *Section, Type *Ty);
 
   void SetNoSanitizeMetadata(Instruction *I) {
     I->setMetadata(I->getModule()->getMDKindID("nosanitize"),
                    MDNode::get(*C, None));
   }
 
-  std::string UniqueSuffix() {
-    static size_t Count = 0;
-    return "_sancov" + std::to_string(Count++);
-  }
-
   std::string getSectionName(const std::string &Section) const;
   std::string getSectionStart(const std::string &Section) const;
   std::string getSectionEnd(const std::string &Section) const;
   Function *SanCovTracePCIndir;
   Function *SanCovTracePC, *SanCovTracePCGuard;
   Function *SanCovTraceCmpFunction[4];
   Function *SanCovTraceConstCmpFunction[4];
   Function *SanCovTraceDivFunction[2];

 }
 
 GlobalVariable *SanitizerCoverageModule::CreateFunctionLocalArrayInSection(
     size_t NumElements, Function &F, Type *Ty, const char *Section) {
   ArrayType *ArrayTy = ArrayType::get(Ty, NumElements);
   auto Array = new GlobalVariable(
       *CurModule, ArrayTy, false, GlobalVariable::PrivateLinkage,
       Constant::getNullValue(ArrayTy), "__sancov_gen_");
   if (auto Comdat = F.getComdat()) {
     Array->setComdat(Comdat);
-  } else {
+  } else if (TargetTriple.isOSBinFormatELF()) {
     // TODO: Refactor into a helper function and use it in ASan.
     assert(F.hasName());
     std::string Name = F.getName();
     if (F.hasLocalLinkage()) {
       std::string ModuleId = getUniqueModuleId(CurModule);
-      Name += ModuleId.empty() ? UniqueSuffix() : ModuleId;
+      Name += ModuleId.empty() ? CurModule->getModuleIdentifier() : ModuleId;
     }
     Comdat = CurModule->getOrInsertComdat(Name);
     // Make this IMAGE_COMDAT_SELECT_NODUPLICATES on COFF. Also upgrade private
     // linkage to internal linkage so that a symbol table entry is emitted. This
     // is necessary in order to create the comdat group.
     if (TargetTriple.isOSBinFormatCOFF()) {
       Comdat->setSelectionKind(Comdat::NoDuplicates);
       if (F.hasPrivateLinkage())