[PATCH] D102462: LLVM Detailed IR tests for introduction of flag -fsanitize-address-detect-stack-use-after-return-mode.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:266-268
+               clEnumValN(AsanDetectStackUseAfterReturnMode::Never,
+                          "0", // only needed to keep unit tests passing
+                          "Redundant with 'never'."),
----------------
kda wrote:
> I added this one and the one below ('1') to avoid updating tests.
> Should I just update the tests and remove these?
then maybe '2' as well?


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:271
+                          "Detect stack use after return if "
+                          "binary flag 'asan-use-after-return' is true."),
+               clEnumValN(AsanDetectStackUseAfterReturnMode::Runtime,
----------------
it should be ASAN_OPTIONS=detect_stack_use_after_return=1


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:3322-3325
     // void *FakeStack = __asan_option_detect_stack_use_after_return
     //     ? __asan_stack_malloc_N(LocalStackSize)
     //     : nullptr;
     // void *LocalStackBase = (FakeStack) ? FakeStack : alloca(LocalStackSize);
----------------
this comment applies to inside of the "then" branch only

Please insert corresponsing spseudo code for else branch:
// void *FakeStack = __asan_stack_malloc_N(LocalStackSize);
// void *LocalStackBase = (FakeStack) ? FakeStack : alloca(LocalStackSize);


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:3344-3356
+      Value *NoFakeStack =
+          IRB.CreateICmpEQ(FakeStack, Constant::getNullValue(IntptrTy));
+      Term = SplitBlockAndInsertIfThen(NoFakeStack, InsBefore, false);
+      IRBIf.SetInsertPoint(Term);
+      Value *AllocaValue = DoDynamicAlloca
+                               ? createAllocaForLayout(IRBIf, L, true)
+                               : StaticAlloca;
----------------
we porbably need entire thing in the else branch
AsanStackMallocFunc[StackMallocIdx] may return NULL in runtime
so we need NoFakeStack branch as well

This is future oportunity to improve: make compiler-rt never return NULL so we can remove CreateICmpEQ and one branch


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:3358
+    } else {
+      // assert(ClUseAfterReturn == AsanDetectStackUseAfterReturnMode:Always)
+      StackMallocIdx = StackMallocSizeClass(LocalStackSize);
----------------
do you want to uncomment and keep it?


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:3366
+          DoDynamicAlloca ? createAllocaForLayout(IRB, L, true) : StaticAlloca;
+      LocalStackBase = (FakeStack) ? FakeStack : AllocValue;
+    }
----------------
FakeStack is LLVM object which represent result of the function call
(FakeStack) is never NULL, however in runtime it can contain NULL, so we need to compile IRB.CreateICmpEQ(FakeStack, Constant::getNullValue(IntptrTy)); like above



================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:261-263
 static cl::opt<bool> ClUseAfterReturn("asan-use-after-return",
                                       cl::desc("Check stack-use-after-return"),
                                       cl::Hidden, cl::init(true));
----------------
vitalybuka wrote:
> we are going to have two conflicting flags.
> Could you please just change this bool flag into Enum, probably without renaming?
> 
> static cl::opt<AsanDetectStackUseAfterReturnMode> ClUseAfterReturn("asan-use-after-return"...
> 0,1 should match current behavior
0,1 is fine
we can remove them later in a separate patch.


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/stack-poisoning.ll:21
+; CHECK-UAR-RUNTIME: label
+; CHECK-UAR-NORUNTIME-NOT: load i32, i32* @__asan_option_detect_stack_use_after_return
+; CHECK-UAR-NORUNTIME-NOT: label
----------------
this statements checks that there is no this line in after previous check
more reliable is to add --implicit-check-not=asan_option_detect_stack_use_after_return into FileCheck arg to make sure it's not there at all


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/stack-poisoning.ll:23
+; CHECK-UAR-NORUNTIME-NOT: label
 ; CHECK-UAR: call i64 @__asan_stack_malloc_4
+; CHECK-UAR-RUNTIME: label
----------------
It would be nice to check for CreateICmpEQ
But probably D102867 is enough


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/stack_dynamic_alloca.ll:22
+; CHECK-RUNTIME: load i32, i32* @__asan_option_detect_stack_use_after_return
+; CHECK-NORUNTIME-NOT: load i32, i32* @__asan_option_detect_stack_use_after_return
 
----------------
same


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102462/new/

https://reviews.llvm.org/D102462