[asan] Instrument comdat globals on COFF targets

[rnk]

Summary

If we can use comdats, then we can make it so that the global metadata
is thrown away if the prevailing definition of the global was
uninstrumented. I have only tested this on COFF targets, but in theory,
there is no reason that we cannot also do this for ELF.

This will allow us to re-enable string merging with ASan on Windows,
reducing the binary size cost of ASan on Windows.

[pcc]

there is no reason that we cannot also do this for ELF.

I thought we were already using !associated (which translates into SHF_LINK_ORDER) on ELF?

[eugenis]

Yes, and we put all globals in comdats, too. See AddressSanitizerModule::InstrumentGlobalsELF.

[rnk]

In D47841#1123978, @pcc wrote:

there is no reason that we cannot also do this for ELF.

I thought we were already using !associated (which translates into SHF_LINK_ORDER) on ELF?

We use that in addition to setting the comdat group. I think !associated is only really needed to make --gc-sections work. The comdat group is what allows us to mix instrumented and uninstrumented globals and not worry about which one prevails.

Anyway, it's just a matter of changing isOSBinFormatCOFF to supportsCOMDAT() to enable it for ELF. I've gotten in trouble in the past for claiming to know more about ELF than I really do, so I'd rather commit that as a separate, easily revertable one line change.

[rnk]

Think we should go for it?

[eugenis]

Sounds like a good idea.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

 bool AddressSanitizerModule::ShouldInstrumentGlobal(GlobalVariable *G) {
   Type *Ty = G->getValueType();
   LLVM_DEBUG(dbgs() << "GLOBAL: " << *G << "\n");
 
   if (GlobalsMD.get(G).IsBlacklisted) return false;
   if (!Ty->isSized()) return false;
   if (!G->hasInitializer()) return false;
   if (GlobalWasGeneratedByCompiler(G)) return false; // Our own globals.
-  // Touch only those globals that will not be defined in other modules.
-  // Don't handle ODR linkage types and COMDATs since other modules may be built
-  // without ASan.
-  if (G->getLinkage() != GlobalVariable::ExternalLinkage &&
-      G->getLinkage() != GlobalVariable::PrivateLinkage &&
-      G->getLinkage() != GlobalVariable::InternalLinkage)
-    return false;
-  if (G->hasComdat()) return false;
   // Two problems with thread-locals:
   //   - The address of the main thread's copy can't be computed at link-time.
   //   - Need to poison all copies, not just the main thread's one.
   if (G->isThreadLocal()) return false;
   // For now, just ignore this Global if the alignment is large.
   if (G->getAlignment() > MinRedzoneSizeForGlobal()) return false;
 
+  // If we know this global is defined only in this module (external or static),
+  // we can instrument it.
+  if (G->getLinkage() != GlobalVariable::ExternalLinkage &&
+      G->getLinkage() != GlobalVariable::PrivateLinkage &&
+      G->getLinkage() != GlobalVariable::InternalLinkage) {
+    // For targets that support comdats, we can instrument ODR globals in
+    // comdats. We simply add our global metadata to the group and hope that an
+    // instrumented global is selected.
+    // FIXME: This should work on ELF, enable it there too.
+    if (!TargetTriple.isOSBinFormatCOFF())
+      return false;
+    if (G->getLinkage() != GlobalVariable::WeakODRLinkage &&
+        G->getLinkage() != GlobalVariable::LinkOnceODRLinkage)
+      return false;
+    if (!G->hasComdat())
+      return false;
+  }
+
+  // If a comdat is present, it must have a selection kind that implies ODR
+  // semantics: no duplicates, any, or exact match.
+  if (Comdat *C = G->getComdat()) {
+    switch (C->getSelectionKind()) {
+    case Comdat::Any:
+    case Comdat::ExactMatch:
+    case Comdat::NoDuplicates:
+      break;
+    case Comdat::Largest:
+    case Comdat::SameSize:
+      return false;
+    }
+  }
+
   if (G->hasSection()) {
     StringRef Section = G->getSection();
 
     // Globals from llvm.metadata aren't emitted, do not instrument them.
     if (Section == "llvm.metadata") return false;
     // Do not instrument globals from special LLVM sections.
     if (Section.find("__llvm") != StringRef::npos || Section.find("__LLVM") != StringRef::npos) return false;
 
for (size_t i = 0; i < n; i++) {
     // Create a new global variable with enough space for a redzone.
     GlobalValue::LinkageTypes Linkage = G->getLinkage();
     if (G->isConstant() && Linkage == GlobalValue::PrivateLinkage)
       Linkage = GlobalValue::InternalLinkage;
     GlobalVariable *NewGlobal =
         new GlobalVariable(M, NewTy, G->isConstant(), Linkage, NewInitializer,
                            "", G, G->getThreadLocalMode());
     NewGlobal->copyAttributesFrom(G);
+    NewGlobal->setComdat(G->getComdat());
     NewGlobal->setAlignment(MinRZ);
 
     // Move null-terminated C strings to "__asan_cstring" section on Darwin.
     if (TargetTriple.isOSBinFormatMachO() && !G->hasSection() &&
         G->isConstant()) {
       auto Seq = dyn_cast<ConstantDataSequential>(G->getInitializer());
       if (Seq && Seq->isCString())
         NewGlobal->setSection("__TEXT,__asan_cstring,regular");

llvm/test/Instrumentation/AddressSanitizer/win-string-literal.ll

+; RUN: opt < %s -asan -asan-module -S | FileCheck %s
+
+; Generated like so:
+; $ clang -S -emit-llvm -Xclang -disable-llvm-passes -fsanitize=address -O1 t.cpp -o t.ll
+; const char *getstr() { return "asdf"; }
+
+; CHECK: $"??_C@_04JIHMPGLA@asdf?$AA@" = comdat any
+
+; CHECK: @"??_C@_04JIHMPGLA@asdf?$AA@" =
+; CHECK-SAME: linkonce_odr dso_local unnamed_addr constant { [5 x i8], [59 x i8] }
+; CHECK-SAME: { [5 x i8] c"asdf\00", [59 x i8] zeroinitializer }, comdat, align 32
+
+; CHECK: @"__asan_global_??_C@_04JIHMPGLA@asdf?$AA@" =
+; CHECK-SAME: private global { i64, i64, i64, i64, i64, i64, i64, i64 }
+; CHECK-SAME: { i64 ptrtoint ({ [5 x i8], [59 x i8] }* @"??_C@_04JIHMPGLA@asdf?$AA@" to i64),
+; CHECK-SAME:   i64 5, i64 64, i64 ptrtoint ([17 x i8]* @__asan_gen_.1 to i64),
+; CHECK-SAME:   i64 ptrtoint ([8 x i8]* @__asan_gen_ to i64), i64 0,
+; CHECK-SAME:   i64 ptrtoint ({ [6 x i8]*, i32, i32 }* @__asan_gen_.3 to i64), i64 0 },
+; CHECK-SAME:   section ".ASAN$GL", comdat($"??_C@_04JIHMPGLA@asdf?$AA@"), align 64
+
+; ModuleID = 't.cpp'
+source_filename = "t.cpp"
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc19.13.26131"
+
+$"??_C@_04JIHMPGLA@asdf?$AA@" = comdat any
+
+@"??_C@_04JIHMPGLA@asdf?$AA@" = linkonce_odr dso_local unnamed_addr constant [5 x i8] c"asdf\00", comdat, align 1
+
+; Function Attrs: nounwind sanitize_address uwtable
+define dso_local i8* @"?getstr@@YAPEBDXZ"() #0 {
+entry:
+  ret i8* getelementptr inbounds ([5 x i8], [5 x i8]* @"??_C@_04JIHMPGLA@asdf?$AA@", i32 0, i32 0)
+}
+
+attributes #0 = { nounwind sanitize_address uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
+!llvm.asan.globals = !{!0}
+!llvm.module.flags = !{!2, !3}
+!llvm.ident = !{!4}
+
+!0 = !{[5 x i8]* @"??_C@_04JIHMPGLA@asdf?$AA@", !1, !"<string literal>", i1 false, i1 false}
+!1 = !{!"t.cpp", i32 1, i32 31}
+!2 = !{i32 1, !"wchar_size", i32 2}
+!3 = !{i32 7, !"PIC Level", i32 2}
+!4 = !{!"clang version 7.0.0 "}