Handle previously ASAN-instrumented IR gracefully when ASAN re-invoked

[tejohnson]

Summary

Fixes the case when invoking -fsanitize=address twice due to serializing
through bitcode or LLVM assembly and then compiling that down to native
code. Without this there are assertion failures from the instrumentation
passes.

Fixes PR32700.

[vsk]

Is it possible to reduce the test case s.t the test file doesn't contain instrumented IR? ASan instrumentation is prone to change, so I worry that the test could start passing for invalid reasons. This might happen if 1) __asan_before_dynamic_init is renamed, 2) GlobalsMetadata::doInit is written more defensively, and 3) a non-idempotent ASan transform is introduced later.

I envision something like:

RUN: opt -asan -asan-module < %s -S -o %t.ll
RUN: opt -asan -asan-module < %t.ll -S -o %t.2.ll
RUN: diff %t.ll %t.2.ll

I spent a few minutes trying to come up with a test like this but did not succeed, because my test file didn't trip the assert you hit.

[tejohnson]

In D41222#954847, @vsk wrote:

Is it possible to reduce the test case s.t the test file doesn't contain instrumented IR? ASan instrumentation is prone to change, so I worry that the test could start passing for invalid reasons. This might happen if 1) __asan_before_dynamic_init is renamed, 2) GlobalsMetadata::doInit is written more defensively, and 3) a non-idempotent ASan transform is introduced later.

I envision something like:

RUN: opt -asan -asan-module < %s -S -o %t.ll
RUN: opt -asan -asan-module < %t.ll -S -o %t.2.ll
RUN: diff %t.ll %t.2.ll

I spent a few minutes trying to come up with a test like this but did not succeed, because my test file didn't trip the assert you hit.

That's a great idea. The reason you weren't able to trigger it is because what is being tripped over the second time is the llvm.asan.globals metadata, which is inserted by clang via SanitizerMetadata::reportGlobalToASan. I was able to get this in the .ll by building the .c file with "-fsanitize=address -emit-llvm -Xclang -disable-llvm-passes". Then running it through opt twice in the manner you describe above hits the assert. I was then able to simplify the initial .ll file even more and still hit the original assert.

Updated test case being uploaded momentarily.

[tejohnson]

Simplify test case

[vitalybuka]

Could you please clang-format the patch for consistency?

[vsk]

Really nice, lgtm!

[tejohnson]

In D41222#955521, @vitalybuka wrote:

Could you please clang-format the patch for consistency?

clang-format didn't find anything. Is there something specific that doesn't look right?

[vitalybuka]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

2422

Simple if statements here and below. However it looks like that existing ifs do not follow LLVM style. So please keep the patch as is.

[pcc]

If this doesn't even fix the ThinLTO bug I don't see why we should land it. The other scenarios mentioned on the bug are "no warranty" scenarios because they use flags like -emit-llvm which aren't really user facing.

[tejohnson]

In D41222#955655, @pcc wrote:

If this doesn't even fix the ThinLTO bug I don't see why we should land it. The other scenarios mentioned on the bug are "no warranty" scenarios because they use flags like -emit-llvm which aren't really user facing.

The fix is small though, and seems better than asserting (or a runtime error as in the release build case)?

[pcc]

The asan pass is more like something like codegenprepare in that we shouldn't really expect it to be run twice. If it does run twice, it most likely indicates a logic error in the pass pipeline, and it would be best to diagnose that with an assertion failure rather than silently ignoring it (which could also hide other unnecessary duplication of passes in the pipeline).

Besides which, this fix is not complete. If a module does not contain dynamically-initialized globals and the globaldce pass happens to run between the two invocations of the asan passes then the reference to the __asan_before_dynamic_init function will be dropped and the passes will still run twice.

If we really wanted to do something about this I think we should be inserting module-level metadata and testing for presence of that metadata (and, ideally, asserting if we see the metadata).

lib/Transforms/Instrumentation/AddressSanitizer.cpp

int AddressSanitizerModule::GetAsanVersion(const Module &M) const {
   bool isAndroid = Triple(M.getTargetTriple()).isAndroid();
   int Version = 8;
   // 32-bit Android is one version ahead because of the switch to dynamic
   // shadow.
   Version += (LongSize == 32 && isAndroid);
   return Version;
 }
 
+static bool isAlreadyAsanInstrumented(const Module &M) {
+  // Looks for a function inserted by the second (module) pass of
+  // ASAN, to detect modules fully ASAN instrumented.
+  return M.getFunction(kAsanPoisonGlobalsName) != nullptr;
+}
+
 bool AddressSanitizerModule::runOnModule(Module &M) {
+  // Skip modules which already have ASan instrumentation.
+  if (isAlreadyAsanInstrumented(M))
+    return false;
   C = &(M.getContext());
   int LongSize = M.getDataLayout().getPointerSizeInBits();
   IntptrTy = Type::getIntNTy(*C, LongSize);
   TargetTriple = Triple(M.getTargetTriple());
   Mapping = getShadowMapping(TargetTriple, LongSize, CompileKernel);
   initializeCallbacks(M);
 
   if (CompileKernel)
EmptyAsm = InlineAsm::get(FunctionType::get(IRB.getVoidTy(), false),
                             /*hasSideEffects=*/true);
   if (Mapping.InGlobal)
     AsanShadowGlobal = M.getOrInsertGlobal("__asan_shadow",
                                            ArrayType::get(IRB.getInt8Ty(), 0));
 }
 
 // virtual
 bool AddressSanitizer::doInitialization(Module &M) {
+  // Skip modules which already have ASan instrumentation.
+  if (isAlreadyAsanInstrumented(M))
+    return false;
+
   // Initialize the private fields. No one has accessed them before.
   GlobalsMD.init(M);
 
   C = &(M.getContext());
   LongSize = M.getDataLayout().getPointerSizeInBits();
   IntptrTy = Type::getIntNTy(*C, LongSize);
   TargetTriple = Triple(M.getTargetTriple());
 
if (II && II->getIntrinsicID() == Intrinsic::localescape) {
         ProcessedAllocas[AI] = false;
       }
       break;
     }
   }
 }
 
 bool AddressSanitizer::runOnFunction(Function &F) {
+  // Skip modules which already have ASan instrumentation.
+  if (isAlreadyAsanInstrumented(*F.getParent()))
+    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

test/Instrumentation/AddressSanitizer/skip-previously-instrumented.ll

+; Test to ensure previously-instrumented IR is handled gracefully
+; (ASAN skipped and not aborted).
+; RUN: opt < %s -asan -asan-module -S -o %t.ll
+; RUN: opt < %t.ll -asan -asan-module -S -o %t2.ll
+; RUN: diff %t.ll %t2.ll
+source_filename = "hello.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@.str = private unnamed_addr constant [13 x i8] c"hello world\0A\00", align 1
+
+!llvm.asan.globals = !{!0}
+
+!0 = !{[13 x i8]* @.str, !1, !"<string literal>", i1 false, i1 false}
+!1 = !{!"hello.c", i32 5, i32 12}