[ASan] Process functions in Asan module pass

[kstoimenov]

Summary

This came up as recommendation while reviewing D112098.

[kstoimenov]

Removed unrelated files.

[kstoimenov]

Reverted pass builder.

[kstoimenov]

Added AddressSanitizerOptions back.

[kstoimenov]

Tests are not passing. Hold off the review.

[vitalybuka]

In D112732#3094158, @kstoimenov wrote:

Tests are not passing. Hold off the review.

Yep, removed FUNCTION_PASS_WITH_PARAMS("asan", must affect some tests

llvm/include/llvm/Transforms/Instrumentation/AddressSanitizer.h

107

please run check-all
it was used in as well llvm-project/llvm/tools/opt/NewPMDriver.cpp

llvm/lib/Passes/PassRegistry.def

396

you should also rename MODULE_PASS_WITH_PARAMS("asan-module", to just MODULE_PASS_WITH_PARAMS("asan",

[kstoimenov]

Fixed test failures and crashes.

[kstoimenov]

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

1318

At first I had module sanitizer run first, but some tests failed. Is it possible that the function pass has some side effects, which are used by the module pass?

2860

This was a pain to figure out. Maybe we should apply the same check in HWAsan?

[kstoimenov]

Fixed remaing 2 tests.

[vitalybuka]

llvm/include/llvm/Transforms/Instrumentation/AddressSanitizer.h

92

Why do we need to remove AddressSanitizerOptions?

llvm/lib/Passes/PassRegistry.def

139

And I guess now we need to use parseASanPassOptions here?

396

I still see asan-module here

[kstoimenov]

Update after discussing with vitalybuka@.

[kstoimenov]

llvm/include/llvm/Transforms/Instrumentation/AddressSanitizer.h

92

Adding it back in as discussed offline.

llvm/lib/Passes/PassRegistry.def

139

I must have reverted by an accident. Should be done now.

146

I think parseModuleAddressSanitizerPassOptions does exactly the same thing as parseASanPassOptions, which is to look for the "kernel" param.

[vitalybuka]

rebase

[vitalybuka]

Lets keep AddressSanitizerPass for a while, we don't have to remove it in the same patch

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

1318

That's OK.

NewPMDriver.cpp was wrong, BackendUtil.cpp was wrong

if we put module in-front of function the following with disable globals instrumentation:

if (G && (!ClInitializers || GlobalIsLinkerInitialized(G)) &&
       isSafeAccess(ObjSizeVis, Addr, O.TypeSize)) {

2860

I looks like should be the first "if" in the function

[kstoimenov]

Moved module instumentation back ahead of function instrumentation.

[kstoimenov]

Moved empty check to the top of the function.

[kstoimenov]

Moved module after function.

[kstoimenov]

After rebase.

[rupprecht]

clang/lib/CodeGen/BackendUtil.cpp

1189

I'm seeing build errors here, and I'm not sure what this is supposed to be?

/home/rupprecht/src/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1189:37: error: use of undeclared identifier 'ModuleUseAfterScope'; did you mean 'UseAfterScope'?
            CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
                                    ^~~~~~~~~~~~~~~~~~~
                                    UseAfterScope

https://buildkite.com/llvm-project/upstream-bazel-rbe/builds/11193#bbebbf99-a2c1-4959-b6f7-f7fb816591c0

[rupprecht]

clang/lib/CodeGen/BackendUtil.cpp

1189

I see it was reverted already, sorry!

[kstoimenov]

clang/lib/CodeGen/BackendUtil.cpp

1189

Sorry about breaking the build. I can't understand how my local tests passed. Functions with a long list of boolean params are so error prone. The downside of not using the builder pattern :-(

[vitalybuka]

rebase

[kstoimenov]

After merging D113072.

[aeubanks]

can we rename "asan-module" to "asan" after this change? and remove the extra "asan-pipeline"/"asan-function-pipeline" parsing callbacks in NewPMDriver.cpp? and previously we had -passes=require<asan-globals-md> because a function pass couldn't invoke a module analysis, but that's no longer relevant, so cleaning those up from RUN lines would also be good

[vitalybuka]

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

1318

Fixing typo in the comment.

NewPMDriver.cpp was wrong, BackendUtil.cpp was wrong

NewPMDriver.cpp was correct, BackendUtil.cpp was wrong

[vitalybuka]

In D112732#3107387, @aeubanks wrote:

can we rename "asan-module" to "asan" after this change? and remove the extra "asan-pipeline"/"asan-function-pipeline" parsing callbacks in NewPMDriver.cpp? and previously we had -passes=require<asan-globals-md> because a function pass couldn't invoke a module analysis, but that's no longer relevant, so cleaning those up from RUN lines would also be good

That was idea, but I changed my mind and recommended to do that in a separate patch.

clang/lib/CodeGen/BackendUtil.cpp

auto ASanPass = [&](SanitizerMask Mask, bool CompileKernel) {
         bool UseAfterScope = CodeGenOpts.SanitizeAddressUseAfterScope;
         bool UseGlobalGC = asanUseGlobalsGC(TargetTriple, CodeGenOpts);
         bool UseOdrIndicator = CodeGenOpts.SanitizeAddressUseOdrIndicator;
         llvm::AsanDtorKind DestructorKind =
             CodeGenOpts.getSanitizeAddressDtor();
         llvm::AsanDetectStackUseAfterReturnMode UseAfterReturn =
             CodeGenOpts.getSanitizeAddressUseAfterReturn();
         MPM.addPass(RequireAnalysisPass<ASanGlobalsMetadataAnalysis, Module>());
-        MPM.addPass(ModuleAddressSanitizerPass(CompileKernel, Recover,
-                                               UseGlobalGC, UseOdrIndicator,
-                                               DestructorKind));
-        MPM.addPass(createModuleToFunctionPassAdaptor(AddressSanitizerPass(
-            {CompileKernel, Recover, UseAfterScope, UseAfterReturn})));
+        MPM.addPass(ModuleAddressSanitizerPass(
+            CompileKernel, Recover, ModuleUseAfterScope, UseOdrIndicator,
+            DestructorKind, UseAfterScope, UseAfterReturn));
       }
     };
     ASanPass(SanitizerKind::Address, false);
     ASanPass(SanitizerKind::KernelAddress, true);
 
     auto HWASanPass = [&](SanitizerMask Mask, bool CompileKernel) {
       if (LangOpts.Sanitize.has(Mask)) {
         bool Recover = CodeGenOpts.SanitizeRecover.has(Mask);

llvm/include/llvm/Transforms/Instrumentation/AddressSanitizer.h

public:
 
   Result run(Module &, ModuleAnalysisManager &);
 
 private:
   friend AnalysisInfoMixin<ASanGlobalsMetadataAnalysis>;
   static AnalysisKey Key;
 };
 
 struct AddressSanitizerOptions {
   bool CompileKernel = false;
   bool Recover = false;
   bool UseAfterScope = false;
   AsanDetectStackUseAfterReturnMode UseAfterReturn =
       AsanDetectStackUseAfterReturnMode::Runtime;
 };
 
 /// Public interface to the address sanitizer pass for instrumenting code to
 /// check for various memory errors at runtime.
 ///
 /// The sanitizer itself is a function pass that works by inserting various
 /// calls to the ASan runtime library functions. The runtime library essentially
 /// replaces malloc() and free() with custom implementations that allow regions
 /// surrounding requested memory to be checked for invalid accesses.
 class AddressSanitizerPass : public PassInfoMixin<AddressSanitizerPass> {
 public:
   explicit AddressSanitizerPass(AddressSanitizerOptions Options)
       : Options(Options){};
   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
   void printPipeline(raw_ostream &OS,
                      function_ref<StringRef(StringRef)> MapClassName2PassName);
   static bool isRequired() { return true; }
 
 private:
   AddressSanitizerOptions Options;
 };
 
 /// Public interface to the address sanitizer module pass for instrumenting code
 /// to check for various memory errors.
 ///
 /// This adds 'asan.module_ctor' to 'llvm.global_ctors'. This pass may also
 /// run intependently of the function address sanitizer.
 class ModuleAddressSanitizerPass
     : public PassInfoMixin<ModuleAddressSanitizerPass> {
 public:
   explicit ModuleAddressSanitizerPass(
       bool CompileKernel = false, bool Recover = false, bool UseGlobalGC = true,
       bool UseOdrIndicator = false,
-      AsanDtorKind DestructorKind = AsanDtorKind::Global);
+      AsanDtorKind DestructorKind = AsanDtorKind::Global,
+      bool UseAfterScope = false,
+      AsanDetectStackUseAfterReturnMode UseAfterReturn =
+          AsanDetectStackUseAfterReturnMode::Runtime);
   PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
   void printPipeline(raw_ostream &OS,
                      function_ref<StringRef(StringRef)> MapClassName2PassName);
   static bool isRequired() { return true; }
 
 private:
   bool CompileKernel;
   bool Recover;
   bool UseGlobalGC;
   bool UseOdrIndicator;
   AsanDtorKind DestructorKind;
+  bool UseAfterScope;
+  AsanDetectStackUseAfterReturnMode UseAfterReturn;
 };
 
 // Insert AddressSanitizer (address sanity checking) instrumentation
 FunctionPass *createAddressSanitizerFunctionPass(
     bool CompileKernel = false, bool Recover = false,
     bool UseAfterScope = false,
     AsanDetectStackUseAfterReturnMode UseAfterReturn =
         AsanDetectStackUseAfterReturnMode::Runtime);

llvm/lib/Passes/PassRegistry.def

MODULE_PASS_WITH_PARAMS("loop-extract",
                         "single")
 MODULE_PASS_WITH_PARAMS("hwasan",
                         "HWAddressSanitizerPass",
                         [](HWAddressSanitizerOptions Opts) {
                           return HWAddressSanitizerPass(Opts);
                         },
                         parseHWASanPassOptions,
                         "kernel;recover")
-MODULE_PASS_WITH_PARAMS("asan-module",
+MODULE_PASS_WITH_PARAMS("asan",
                         "ModuleAddressSanitizerPass",
                         [](bool CompileKernel) {
                           return ModuleAddressSanitizerPass(CompileKernel,
                                                             false, true,
                                                             false);
                         },
                         parseModuleAddressSanitizerPassOptions,
                         "kernel")
 #undef MODULE_PASS_WITH_PARAMS
 
 #ifndef CGSCC_ANALYSIS
 #define CGSCC_ANALYSIS(NAME, CREATE_PASS)
 #endif
 CGSCC_ANALYSIS("no-op-cgscc", NoOpCGSCCAnalysis())
 CGSCC_ANALYSIS("fam-proxy", FunctionAnalysisManagerCGSCCProxy())
FUNCTION_PASS_WITH_PARAMS("loop-unroll",
                            },
                           parseLoopUnrollOptions,
                           "O0;O1;O2;O3;full-unroll-max=N;"
                           "no-partial;partial;"
                           "no-peeling;peeling;"
                           "no-profile-peeling;profile-peeling;"
                           "no-runtime;runtime;"
                           "no-upperbound;upperbound")
-FUNCTION_PASS_WITH_PARAMS("asan",
-                          "AddressSanitizerPass",
-                           [](AddressSanitizerOptions Opts) {
-                             return AddressSanitizerPass(Opts);
-                           },
-                          parseASanPassOptions,
-                          "kernel")
 FUNCTION_PASS_WITH_PARAMS("msan",
                           "MemorySanitizerPass",
                            [](MemorySanitizerOptions Opts) {
                              return MemorySanitizerPass(Opts);
                            },
                           parseMSanPassOptions,
                           "recover;kernel;track-origins=N")
 FUNCTION_PASS_WITH_PARAMS("simplifycfg",

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

void ModuleAddressSanitizerPass::printPipeline(
   OS << "<";
   if (CompileKernel)
     OS << "kernel";
   OS << ">";
 }
 
 ModuleAddressSanitizerPass::ModuleAddressSanitizerPass(
     bool CompileKernel, bool Recover, bool UseGlobalGC, bool UseOdrIndicator,
-    AsanDtorKind DestructorKind)
+    AsanDtorKind DestructorKind, bool UseAfterScope,
+    AsanDetectStackUseAfterReturnMode UseAfterReturn)
     : CompileKernel(CompileKernel), Recover(Recover), UseGlobalGC(UseGlobalGC),
-      UseOdrIndicator(UseOdrIndicator), DestructorKind(DestructorKind) {}
+      UseOdrIndicator(UseOdrIndicator), DestructorKind(DestructorKind),
+      UseAfterScope(UseAfterScope), UseAfterReturn(UseAfterReturn) {}
 
 PreservedAnalyses ModuleAddressSanitizerPass::run(Module &M,
-                                                  AnalysisManager<Module> &AM) {
-  GlobalsMetadata &GlobalsMD = AM.getResult<ASanGlobalsMetadataAnalysis>(M);
-  ModuleAddressSanitizer Sanitizer(M, &GlobalsMD, CompileKernel, Recover,
+                                                  ModuleAnalysisManager &MAM) {
+  GlobalsMetadata &GlobalsMD = MAM.getResult<ASanGlobalsMetadataAnalysis>(M);
+  ModuleAddressSanitizer ModuleSanitizer(M, &GlobalsMD, CompileKernel, Recover,
                                          UseGlobalGC, UseOdrIndicator,
                                          DestructorKind);
-  if (Sanitizer.instrumentModule(M))
+  bool Modified = false;
+  auto &FAM = MAM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
+  for (Function &F : M) {
+    AddressSanitizer FunctionSanitizer(M, &GlobalsMD, CompileKernel, Recover,
+                                       UseAfterScope, UseAfterReturn);
+    const TargetLibraryInfo &TLI = FAM.getResult<TargetLibraryAnalysis>(F);
+    Modified |= FunctionSanitizer.instrumentFunction(F, &TLI);
+  }
+  Modified |= ModuleSanitizer.instrumentModule(M);
+  if (Modified)
     return PreservedAnalyses::none();
   return PreservedAnalyses::all();
 }
 
 INITIALIZE_PASS(ASanGlobalsMetadataWrapperPass, "asan-globals-md",
                 "Read metadata to mark which globals should be instrumented "
                 "when running ASan.",
                 false, true)
bool ShouldInstrument =
       ClDebugMin < 0 || ClDebugMax < 0 ||
       (Instrumented >= ClDebugMin && Instrumented <= ClDebugMax);
   Instrumented++;
   return !ShouldInstrument;
 }
 
 bool AddressSanitizer::instrumentFunction(Function &F,
                                           const TargetLibraryInfo *TLI) {
+  if (F.empty())
+    return false;
   if (F.getLinkage() == GlobalValue::AvailableExternallyLinkage) return false;
   if (!ClDebugFunc.empty() && ClDebugFunc == F.getName()) return false;
   if (F.getName().startswith("__asan_")) return false;
 
   bool FunctionModified = false;
 
   // If needed, insert __asan_init before checking for SanitizeAddress attr.
   // This function needs to be called even if the function body is not
   // instrumented.
   if (maybeInsertAsanInitAtFunctionEntry(F))
     FunctionModified = true;
 

llvm/test/Instrumentation/SanitizerCoverage/tracing-comdat.ll

 ; Test that the coverage guards have proper comdat
 ; Make sure asan does not instrument __sancov_gen_
 
 ; RUN: opt < %s -passes='module(sancov-module)' -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard  -S  | FileCheck %s
-; RUN: opt < %s -passes='module(require<asan-globals-md>,sancov-module,asan-module),function(asan)' -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard  -S  | FileCheck %s
+; RUN: opt < %s -passes='module(require<asan-globals-md>,sancov-module,asan)' -sanitizer-coverage-level=3 -sanitizer-coverage-trace-pc-guard  -S  | FileCheck %s
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 $Foo = comdat any
 ; Function Attrs: uwtable
 define linkonce_odr void @Foo() comdat {
 entry:
   ret void
 }
 
 define linkonce_odr void @Bar() {
 entry:
   ret void
 }
 
 ; CHECK: @__sancov_gen_ = private global [1 x i32] zeroinitializer, section "__sancov_guards", comdat($Foo)
 ; CHECK: @__sancov_gen_.1 = private global [1 x i32] zeroinitializer, section "__sancov_guards"

llvm/test/Other/new-pm-print-pipeline.ll

 ; CHECK-12: function(early-cse<>,early-cse<memssa>)
 
 ; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='msan-module,function(msan,msan<>,msan<recover;kernel;track-origins=5>)' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-13
 ; CHECK-13: msan-module,function(msan<track-origins=0>,msan<track-origins=0>,msan<recover;kernel;track-origins=5>)
 
 ; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='module(hwasan<>,hwasan<kernel;recover>)' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-14
 ; CHECK-14: hwasan<>,hwasan<kernel;recover>
 
-; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='function(asan<>,asan<kernel>)' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-15
-; CHECK-15: function(asan<>,asan<kernel>)
+; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='module(asan<>,asan<kernel>)' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-15
+; CHECK-15: asan<>,asan<kernel>
 
 ; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='module(loop-extract<>,loop-extract<single>)' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-16
 ; CHECK-16: loop-extract<>,loop-extract<single>
 
 ; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='function(print<stack-lifetime><may>,print<stack-lifetime><must>)' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-17
 ; CHECK-17: function(print<stack-lifetime><may>,print<stack-lifetime><must>)
 
 ; RUN: opt -disable-output -disable-verify -print-pipeline-passes -passes='function(simplifycfg<bonus-inst-threshold=5;forward-switch-cond;switch-to-lookup;keep-loops;hoist-common-insts;sink-common-insts>,simplifycfg<bonus-inst-threshold=7;no-forward-switch-cond;no-switch-to-lookup;no-keep-loops;no-hoist-common-insts;no-sink-common-insts>)' < %s | FileCheck %s --match-full-lines --check-prefixes=CHECK-18

llvm/tools/opt/NewPMDriver.cpp

PB.registerPipelineParsingCallback(
           MPM.addPass(NewPMCheckDebugifyPass());
           return true;
         }
         return false;
       });
   PB.registerPipelineParsingCallback(
       [](StringRef Name, ModulePassManager &MPM,
          ArrayRef<PassBuilder::PipelineElement>) {
-        AddressSanitizerOptions Opts;
         if (Name == "asan-pipeline") {
           MPM.addPass(
               RequireAnalysisPass<ASanGlobalsMetadataAnalysis, Module>());
-          MPM.addPass(
-              createModuleToFunctionPassAdaptor(AddressSanitizerPass(Opts)));
           MPM.addPass(ModuleAddressSanitizerPass());
           return true;
         } else if (Name == "asan-function-pipeline") {
+          // FIXME: now this is the same as asan-pipeline and can me removed.
           MPM.addPass(
               RequireAnalysisPass<ASanGlobalsMetadataAnalysis, Module>());
-          MPM.addPass(
-              createModuleToFunctionPassAdaptor(AddressSanitizerPass(Opts)));
+          MPM.addPass(ModuleAddressSanitizerPass());
           return true;
         }
         return false;
       });
 
 #define HANDLE_EXTENSION(Ext)                                                  \
   get##Ext##PluginInfo().RegisterPassBuilderCallbacks(PB);
 #include "llvm/Support/Extension.def"