[PATCH] D12704: [ASan] Don't instrument promotable dynamic allocas.

[Alexey Samsonov via llvm-commits]

samsonov added inline comments.

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:433
@@ -432,3 +432,3 @@
   bool isDynamicAlloca(AllocaInst &AI) const {
     return AI.isArrayAllocation() || !AI.isStaticAlloca();
   }
----------------
m.ostepenko wrote:
> I wonder if we should remove isArrayAllocation() from this filter.
Let's handle this separately.

================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1800
@@ -1792,1 +1799,3 @@
+  }
+
   if (ClInstrumentAllocas && DynamicAllocaVec.size() > 0) {
----------------
m.ostepenko wrote:
> > 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?
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.


http://reviews.llvm.org/D12704