[PATCH] D20913: [asan] add primitives that allow coroutine implementations

[Philippe Daouadi via llvm-commits]

blastrock added a comment.

In http://reviews.llvm.org/D20913#455864, @dvyukov wrote:

> > Another downside is that getting the current thread's stack bounds is done through a non-portable pthread API, but it does simplify asan code.
>
>
> I only see the APIs in test code. Is it correct?
>  Tests are OK. swapcontext is also non-portable.


Yes, I didn't add non-portable code to the library itself. I said that because it forces users to write non-portable code to annotate their code, but it's acceptable.


================
Comment at: lib/asan/asan_thread.cc:133
@@ +132,3 @@
+  if (!stack_switching_)
+    Report("WARNING: finishing a fiber enter that has not started\n");
+  // order is important so that the range [bottom, top] is empty until we are
----------------
dvyukov wrote:
> If you want to tolerate some user errors here, then I think it's better to return here. next_stack_top/bottom are most likely 0, so we will end up with no stack at all otherwise.
@filcab suggested an assertion instead, I think it would indeed be better since it shows a mis-use of annotations.

================
Comment at: lib/asan/asan_thread.cc:147
@@ +146,3 @@
+  if (!stack_switching_) {
+    if (next_stack_top_ && next_stack_bottom_)
+      return StackBounds{next_stack_bottom_, next_stack_top_};  // NOLINT
----------------
dvyukov wrote:
> Why?
> If I am reading core correctly, this must never happen.
> 
> > I made my implementation more signal-proof by using volatile. I saw later that in some other parts of the code, you prefer using atomic access, I can change the volatiles to that if needed.
> 
> Yes, please.
> Current ordering of memory  accesses if very tricky, esp in FinishSwitchFiber and I see that that is actually important for GetStackBounds correctness.
> 
> I think it is enough to make only stack_switching_ atomic:
> 
>   atomic<int> stack_switching_;
>   
>   void AsanThread::StartSwitchFiber(uptr bottom, uptr size) {
>     next_stack_bottom_ = bottom;
>     next_stack_top_ = bottom + size;
>     atomic_store(&stack_switching_, 1, memory_order_release);
>   }
>     
>   void AsanThread::FinishSwitchFiber() {
>     stack_bottom_ = next_stack_bottom_;
>     stack_top_ = next_stack_top_;
>     atomic_store(&stack_switching_, 0, memory_order_release);
>     next_stack_top_ = 0;
>     next_stack_bottom_ = 0;
>   }
>   
>   inline AsanThread::StackBounds AsanThread::GetStackBounds() const {
>     if (!atomic_load(&stack_switching_, memory_order_acquire))
>       return StackBounds{stack_bottom_, stack_top_};  // NOLINT
>     char local;
>     const uptr cur_stack = (uptr)&local;
>     // Note: need to check next stack first, because FinishSwitchFiber
>     // may be in process of overwriting stack_top_/bottom_. But in such case
>     // we are already on the next stack.
>     if (cur_stack >= next_stack_bottom_ && cur_stack < next_stack_top_)
>       return StackBounds{next_stack_bottom_, next_stack_top_};  // NOLINT
>     return StackBounds{stack_bottom_, stack_top_};  // NOLINT
>   }
> 
Oh yes, you're right, this can't happen, I'll remove it.

As for your implementation, I think it would work, and it is indeed simpler.

================
Comment at: test/asan/TestCases/Linux/swapcontext_annotation.cc:5
@@ +4,3 @@
+// function
+// RUN: %clangxx_asan -lpthread -O0 %s -o %t
+// RUN: %run %t x 2>&1 | FileCheck %s
----------------
dvyukov wrote:
> I would expect that we do all that -O0/-O1/-O2 automatically on a higher level. At least we used to as far as I remember. Now all that cmake/lit in incomprehensible, so I am not sure... But I don't see any -O flags in other asan tests.
> 
> Does anybody know is we run asan lit tests with different optimization levels?
> 
I took this snippet from the preivous test swapcontext_test.cc which was already there and did all the optimization levels explicitly. I can check if the test framework already runs all optimization levels by itself.

================
Comment at: test/asan/TestCases/Linux/swapcontext_annotation.cc:67
@@ +66,3 @@
+    perror("swapcontext");
+    _exit(0);
+  }
----------------
dvyukov wrote:
> s/0/1/
> otherwise test can silently break and continue passing on bots
Indeed, though it would be caught anyway because the string "TestX passed" would not appear in output.


http://reviews.llvm.org/D20913