[PATCH] D24628: [ASAN] Pass previous stack information through __sanitizer_finish_switch_fiber

[Filipe Cabecinhas via cfe-commits]

filcab added a comment.

Please add some printf calls to the test, to show that you have the correct stack+size, too.

Thanks for working on this.
Filipe


================
Comment at: lib/asan/asan_thread.cc:141
@@ -140,3 +140,3 @@
   if (!fake_stack_save && current_fake_stack)
     current_fake_stack->Destroy(this->tid());
 }
----------------
Please make sure you're accounting for this. It seems the test is always passing nullptr to start_switch, which shouldn't be done unless you're finishing up a fiber execution, IIRC.

================
Comment at: lib/asan/asan_thread.cc:162
@@ -155,1 +161,3 @@
+    *size_old = stack_top_ - stack_bottom_;
+  }
   stack_bottom_ = next_stack_bottom_;
----------------
I think the usual style is: one statement => no braces.

================
Comment at: test/asan/TestCases/Linux/swapcontext_annotation.cc:9
@@ -8,3 +8,3 @@
 // This test is too subtle to try on non-x86 arch for now.
-// REQUIRES: x86_64-supported-target,i386-supported-target
+// REQUIRES: x86-target-arch
 
----------------
blastrock wrote:
> andriigrynenko wrote:
> > The test was actually broken in trunk (not updated for the fakestack argument). Apparently it was never run (considered unsupported) because of wrong targets here. 
> Indeed, my bad, I forgot to compile and run the tests after the last modification I made to that patch. Sorry for that.
> 
> I didn't need to change this line to run them on my computer though, but if you know what you are doing with this line, it's ok. And it's strange indeed that no buildfarm caught this...
They wouldn't catch tests not being run.
Test with LLVM_LIT_ARGS=-v (instead of -sv) on your cmake config and double-check that the tests are run.

================
Comment at: test/asan/TestCases/Linux/swapcontext_annotation.cc:47
@@ -46,3 +46,3 @@
   CallNoReturn();
-  __sanitizer_finish_switch_fiber();
+  __sanitizer_finish_switch_fiber(NULL, NULL, NULL);
 
----------------
dvyukov wrote:
> #include <stddef.h> for NULL
nit: nullptr?


https://reviews.llvm.org/D24628