[ASan] Don't instrument promotable dynamic allocas.

[samsonov]

Summary

We already have a test that checks that promotable dynamic allocas are
ignored, as well as static promotable allocas. Make sure this test will
still pass if/when we enable dynamic alloca instrumentation by default.

[samsonov]

• [ASan] Handle lifetime intrinsics before handling dynamic allocas.

[m.ostapenko]

Thanks for fixing this! Just one question below.

lib/Transforms/Instrumentation/AddressSanitizer.cpp

1800 ↗

(On Diff #34370)

lifetime intrinsics may refer to dynamic allocas, so we need to emit instrumentation before these dynamic allocas would be replaced.

Well, if we instrument dynamic alloca related to some lifetime intrinsic first time and instrument it in handleDynamicAllocaCall() second time, we may end up with redzones would be mixed, right? Anyway, looking to poisonAlloca(...) function:

void FunctionStackPoisoner::poisonAlloca(Value *V, uint64_t Size,
                                         IRBuilder<> &IRB, bool DoPoison) {
  // For now just insert the call to ASan runtime.
  Value *AddrArg = IRB.CreatePointerCast(V, IntptrTy);
  Value *SizeArg = ConstantInt::get(IntptrTy, Size);
  IRB.CreateCall(
      DoPoison ? AsanPoisonStackMemoryFunc : AsanUnpoisonStackMemoryFunc,
      {AddrArg, SizeArg});
}

Size parameter is uint64_t, so it seems we can poison static allocas only.

So, I'm wondering, is that the case?

[m.ostapenko]

Ah, right, I see that we indeed can mess with dynamic allocas and lifetime intrinsics here (in such cases, when alloca's size is known in compile time, but it's treated as dynamic one for others reasons).

Alexey, could you please land this?

Thanks!

lib/Transforms/Instrumentation/AddressSanitizer.cpp

433 ↗

(On Diff #34370)

I wonder if we should remove isArrayAllocation() from this filter.

[m.ostapenko]

Alexey, could you commit this please? It would be very nice to try enable dynamic alloca instrumentation in Clang.

[samsonov]

Looking into this now. Sorry for the delay!

[samsonov]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

433 ↗

(On Diff #34370)

Let's handle this separately.

1800 ↗

(On Diff #34370)

Yes. Alloca will be treated as "dynamic" if it has constant size, but it's not located in the function entry block, for instance.

AsanPoisonStackMemoryFunc is severely undertested (and half-dead), specially in presence of dynamic allocas, so for now I think it's fine to keep "correctness" in the sense of "keep tests we have working". I'm not sure that poisoning left and right redzoes in __asan_alloca_poison() would conflict with poisoning the alloca itself on lifetime intrinsics.

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

if (PreviouslySeenAllocaInfo != ProcessedAllocas.end())
     return PreviouslySeenAllocaInfo->getSecond();
 
   bool IsInteresting =
       (AI.getAllocatedType()->isSized() &&
        // alloca() may be called with 0 size, ignore it.
        getAllocaSizeInBytes(&AI) > 0 &&
        // We are only interested in allocas not promotable to registers.
        // Promotable allocas are common under -O0.
-       (!ClSkipPromotableAllocas || !isAllocaPromotable(&AI) ||
-        isDynamicAlloca(AI)));
+       (!ClSkipPromotableAllocas || !isAllocaPromotable(&AI)));
 
   ProcessedAllocas[&AI] = IsInteresting;
   return IsInteresting;
 }
 
 /// If I is an interesting memory access, return the PointerOperand
 /// and set IsWrite/Alignment. Otherwise return nullptr.
 Value *AddressSanitizer::isInterestingMemoryAccess(Instruction *I,
void FunctionStackPoisoner::createDynamicAllocasInitStorage() {
   DynamicAllocaLayout = IRB.CreateAlloca(IntptrTy, nullptr);
   IRB.CreateStore(Constant::getNullValue(IntptrTy), DynamicAllocaLayout);
   DynamicAllocaLayout->setAlignment(32);
 }
 
 void FunctionStackPoisoner::poisonStack() {
   assert(AllocaVec.size() > 0 || DynamicAllocaVec.size() > 0);
 
+  // Insert poison calls for lifetime intrinsics for alloca.
+  bool HavePoisonedAllocas = false;
+  for (const auto &APC : AllocaPoisonCallVec) {
+    assert(APC.InsBefore);
+    assert(APC.AI);
+    IRBuilder<> IRB(APC.InsBefore);
+    poisonAlloca(APC.AI, APC.Size, IRB, APC.DoPoison);
+    HavePoisonedAllocas |= APC.DoPoison;
+  }
+
   if (ClInstrumentAllocas && DynamicAllocaVec.size() > 0) {
     // Handle dynamic allocas.
     createDynamicAllocasInitStorage();
     for (auto &AI : DynamicAllocaVec) handleDynamicAllocaCall(AI);
 
     unpoisonDynamicAllocas();
   }
 
void FunctionStackPoisoner::poisonStack() {
   } else {
     // void *FakeStack = nullptr;
     // void *LocalStackBase = alloca(LocalStackSize);
     FakeStack = ConstantInt::get(IntptrTy, 0);
     LocalStackBase =
         DoDynamicAlloca ? createAllocaForLayout(IRB, L, true) : StaticAlloca;
   }
 
-  // Insert poison calls for lifetime intrinsics for alloca.
-  bool HavePoisonedAllocas = false;
-  for (const auto &APC : AllocaPoisonCallVec) {
-    assert(APC.InsBefore);
-    assert(APC.AI);
-    IRBuilder<> IRB(APC.InsBefore);
-    poisonAlloca(APC.AI, APC.Size, IRB, APC.DoPoison);
-    HavePoisonedAllocas |= APC.DoPoison;
-  }
-
   // Replace Alloca instructions with base+offset.
   for (const auto &Desc : SVD) {
     AllocaInst *AI = Desc.AI;
     Value *NewAllocaPtr = IRB.CreateIntToPtr(
         IRB.CreateAdd(LocalStackBase, ConstantInt::get(IntptrTy, Desc.Offset)),
         AI->getType());
     replaceDbgDeclareForAlloca(AI, NewAllocaPtr, DIB, /*Deref=*/true);
     AI->replaceAllUsesWith(NewAllocaPtr);

llvm/trunk/test/Instrumentation/AddressSanitizer/debug_info_noninstrumented_alloca.ll

 ; This test checks that non-instrumented allocas stay in the first basic block.
 ; Only first-basic-block allocas are considered stack slots, and moving them
 ; breaks debug info.
 
 ; RUN: opt < %s -asan -asan-module -S | FileCheck %s
+; RUN: opt < %s -asan -asan-module -asan-instrument-allocas=1 -S | FileCheck %s
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.10.0"
 
 define i32 @foo() sanitize_address {
 entry:
   ; Won't be instrumented because of asan-skip-promotable-allocas.
   %non_instrumented1 = alloca i32, align 4