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

Philippe Daouadi via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 13 04:29:40 PDT 2016

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.


More information about the llvm-commits mailing list