[asan] Disable ASan global-GC depending on the target and compiler flags

[eugenis]

Summary

The linux part is a bit ahead of time - the instrumentation code where this matters have not landed yet. But when it does, this would be the right condition, and for now ELF instrumentation simply ignores this setting.

[eugenis]

From a quick look at the code, it seems like -fno-data-sections on COFF would suppress GC of globals the same as on ELF. Is that true?

[pcc]

I think it would be better to move this logic to the driver and have it pass in an -mllvm flag. The sanitizer passes should really be taking no arguments in the constructor like the other passes, so I don't want us to add another argument here.

[rnk]

In D32064#726861, @pcc wrote:

I think it would be better to move this logic to the driver and have it pass in an -mllvm flag. The sanitizer passes should really be taking no arguments in the constructor like the other passes, so I don't want us to add another argument here.

That seems like the opposite of the direction we've been moving, though. cl::opt flags can't appear twice, and this means one process can't do two asan compilations in two LLVMContexts in parallel with different settings.

lib/Driver/SanitizerArgs.cpp

636 ↗

(On Diff #95254)

We can return true for COFF here. By adding a comdat during asan instrumentation, we effectively implement -fdata-sections ourselves. If the user really wanted -fno-data-sections for some reason, they're out of luck right now.

[eugenis]

lib/Driver/SanitizerArgs.cpp

636 ↗

(On Diff #95254)

What do you mean by "out of luck", will it break compilation?
Because the point of this change is not to enable data-sections unless asked by the user. Data sections greatly inflate ELF object file size (not sure how big is the effect on COFF) and we should not do that by default.

[rnk]

lib/Driver/SanitizerArgs.cpp

636 ↗

(On Diff #95254)

By "out of luck", I was thinking about users who might do crazy things like take label differences between globals and assume that they are laid out consecutively in the original source file order. I wasn't thinking about object file size.

The size increase for COFF is probably comparable, so I guess I agree, this is reasonable behavior. I think Chromium explicitly passes /Gw, which is equivalent to -fdata-sections. We basically don't support non-integrated as on Windows at this point. We've extended the assembler a number of times already.

[pcc]

In D32064#728629, @rnk wrote:

In D32064#726861, @pcc wrote:

I think it would be better to move this logic to the driver and have it pass in an -mllvm flag. The sanitizer passes should really be taking no arguments in the constructor like the other passes, so I don't want us to add another argument here.

That seems like the opposite of the direction we've been moving, though. cl::opt flags can't appear twice, and this means one process can't do two asan compilations in two LLVMContexts in parallel with different settings.

Yes, but adding an argument is also the wrong direction. This information should really be passed either via the module (e.g. module flags or attributes) or the TargetMachine. If we aren't going to do that, we might as well pass it via -mllvm, as it is simpler.

[eugenis]

Ping.
I don't really have a preference.

[rnk]

In D32064#728683, @pcc wrote:

In D32064#728629, @rnk wrote:

In D32064#726861, @pcc wrote:

I think it would be better to move this logic to the driver and have it pass in an -mllvm flag. The sanitizer passes should really be taking no arguments in the constructor like the other passes, so I don't want us to add another argument here.

That seems like the opposite of the direction we've been moving, though. cl::opt flags can't appear twice, and this means one process can't do two asan compilations in two LLVMContexts in parallel with different settings.

Yes, but adding an argument is also the wrong direction. This information should really be passed either via the module (e.g. module flags or attributes) or the TargetMachine. If we aren't going to do that, we might as well pass it via -mllvm, as it is simpler.

I'm really just echoing what I thought was conventional wisdom. I think avoiding new cl::opts is something that @chandlerc cares more about. It's been said on llvm-dev that cl::opt should mostly be for debugging. We should be able to ship a production compiler that effectively compiles every cl::opt to its default value.

I don't see why constructor parameters are the wrong direction. It's already how ASan instrumentation takes every other global pass option. Only tsan uses -mllvm flags created by clang. If we aren't going to make ASan consistent, we should standardize on what we already have.

[eugenis]

The ultimate solution would be a function attribute - if we want this thing to work correctly with LTO. But it sounds a bit like overkill, plus none of the settings it depends on (integrated-as, data-sections) work with LTO anway: the former is ignored and the second does not make sense.

[pcc]

I won't stand in the way here if others feel strongly that the flag should be passed via a constructor. It will just mean more work to be done if/when this flag is ever changed to be passed via some other mechanism, but that's a relatively minor detail.

[eugenis]

Reverted back to using pass constructor argument.

[pcc]

Please add a test case.

[eugenis]

what kind of test?

[pcc]

I think the only functional change here is for COFF, so you can add a CodeGen test that checks that metadata globals are created only if -fdata-sections is passed.

[eugenis]

PTAL

[pcc]

LGTM

[eugenis]

r301225

lib/CodeGen/BackendUtil.cpp

public:
   void EmitAssemblyWithNewPassManager(BackendAction Action,
                                       std::unique_ptr<raw_pwrite_stream> OS);
 };
 
 // We need this wrapper to access LangOpts and CGOpts from extension functions
 // that we add to the PassManagerBuilder.
 class PassManagerBuilderWrapper : public PassManagerBuilder {
 public:
-  PassManagerBuilderWrapper(const CodeGenOptions &CGOpts,
+  PassManagerBuilderWrapper(const Triple &TargetTriple,
+                            const CodeGenOptions &CGOpts,
                             const LangOptions &LangOpts)
-      : PassManagerBuilder(), CGOpts(CGOpts), LangOpts(LangOpts) {}
+      : PassManagerBuilder(), TargetTriple(TargetTriple), CGOpts(CGOpts),
+        LangOpts(LangOpts) {}
+  const Triple &getTargetTriple() const { return TargetTriple; }
   const CodeGenOptions &getCGOpts() const { return CGOpts; }
   const LangOptions &getLangOpts() const { return LangOpts; }
+
 private:
+  const Triple &TargetTriple;
   const CodeGenOptions &CGOpts;
   const LangOptions &LangOpts;
 };
-
 }
 
 static void addObjCARCAPElimPass(const PassManagerBuilder &Builder, PassManagerBase &PM) {
   if (Builder.OptLevel > 0)
     PM.add(createObjCARCAPElimPass());
 }
 
 static void addObjCARCExpandPass(const PassManagerBuilder &Builder, PassManagerBase &PM) {
static void addSanitizerCoveragePass(const PassManagerBuilder &Builder,
   Opts.TraceDiv = CGOpts.SanitizeCoverageTraceDiv;
   Opts.TraceGep = CGOpts.SanitizeCoverageTraceGep;
   Opts.Use8bitCounters = CGOpts.SanitizeCoverage8bitCounters;
   Opts.TracePC = CGOpts.SanitizeCoverageTracePC;
   Opts.TracePCGuard = CGOpts.SanitizeCoverageTracePCGuard;
   PM.add(createSanitizerCoverageModulePass(Opts));
 }
 
+// Check if ASan should use GC-friendly instrumentation for globals.
+// First of all, there is no point if -fdata-sections is off (expect for MachO,
+// where this is not a factor). Also, on ELF this feature requires an assembler
+// extension that only works with -integrated-as at the moment.
+static bool asanUseGlobalsGC(const Triple &T, const CodeGenOptions &CGOpts) {
+  switch (T.getObjectFormat()) {
+  case Triple::MachO:
+    return true;
+  case Triple::COFF:
+    return CGOpts.DataSections;
+  case Triple::ELF:
+    return CGOpts.DataSections && !CGOpts.DisableIntegratedAS;
+  default:
+    return false;
+  }
+}
+
 static void addAddressSanitizerPasses(const PassManagerBuilder &Builder,
                                       legacy::PassManagerBase &PM) {
   const PassManagerBuilderWrapper &BuilderWrapper =
       static_cast<const PassManagerBuilderWrapper&>(Builder);
+  const Triple &T = BuilderWrapper.getTargetTriple();
   const CodeGenOptions &CGOpts = BuilderWrapper.getCGOpts();
   bool Recover = CGOpts.SanitizeRecover.has(SanitizerKind::Address);
   bool UseAfterScope = CGOpts.SanitizeAddressUseAfterScope;
+  bool UseGlobalsGC = asanUseGlobalsGC(T, CGOpts);
   PM.add(createAddressSanitizerFunctionPass(/*CompileKernel*/ false, Recover,
                                             UseAfterScope));
-  PM.add(createAddressSanitizerModulePass(/*CompileKernel*/false, Recover));
+  PM.add(createAddressSanitizerModulePass(/*CompileKernel*/ false, Recover,
+                                          UseGlobalsGC));
 }
 
 static void addKernelAddressSanitizerPasses(const PassManagerBuilder &Builder,
                                             legacy::PassManagerBase &PM) {
   PM.add(createAddressSanitizerFunctionPass(
       /*CompileKernel*/ true,
       /*Recover*/ true, /*UseAfterScope*/ false));
   PM.add(createAddressSanitizerModulePass(/*CompileKernel*/true,

 
 void EmitAssemblyHelper::CreatePasses(legacy::PassManager &MPM,
                                       legacy::FunctionPassManager &FPM) {
   // Handle disabling of all LLVM passes, where we want to preserve the
   // internal module before any optimization.
   if (CodeGenOpts.DisableLLVMPasses)
     return;
 
-  PassManagerBuilderWrapper PMBuilder(CodeGenOpts, LangOpts);
-
   // Figure out TargetLibraryInfo.  This needs to be added to MPM and FPM
   // manually (and not via PMBuilder), since some passes (eg. InstrProfiling)
   // are inserted before PMBuilder ones - they'd get the default-constructed
   // TLI with an unknown target otherwise.
   Triple TargetTriple(TheModule->getTargetTriple());
   std::unique_ptr<TargetLibraryInfoImpl> TLII(
       createTLII(TargetTriple, CodeGenOpts));
 
+  PassManagerBuilderWrapper PMBuilder(TargetTriple, CodeGenOpts, LangOpts);
+
   // At O0 and O1 we only run the always inliner which is more efficient. At
   // higher optimization levels we run the normal inliner.
   if (CodeGenOpts.OptimizationLevel <= 1) {
     bool InsertLifetimeIntrinsics = (CodeGenOpts.OptimizationLevel != 0 &&
                                      !CodeGenOpts.DisableLifetimeMarkers);
     PMBuilder.Inliner = createAlwaysInlinerLegacyPass(InsertLifetimeIntrinsics);
   } else {
     // We do not want to inline hot callsites for SamplePGO module-summary build

test/CodeGen/asan-globals-gc.cpp

+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-windows-msvc %s | FileCheck %s --check-prefix=WITHOUT-GC
+// RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-windows-msvc -fdata-sections %s | FileCheck %s --check-prefix=WITH-GC
+
+int global;
+
+// WITH-GC-NOT: call void @__asan_register_globals
+// WITHOUT-GC: call void @__asan_register_globals