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

[Dmitry Vyukov via llvm-commits]

dvyukov added a comment.

> 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.


================
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
----------------
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.

================
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
----------------
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
  }


================
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
----------------
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?


================
Comment at: test/asan/TestCases/Linux/swapcontext_annotation.cc:67
@@ +66,3 @@
+    perror("swapcontext");
+    _exit(0);
+  }
----------------
s/0/1/
otherwise test can silently break and continue passing on bots

================
Comment at: test/asan/TestCases/Linux/swapcontext_annotation.cc:89
@@ +88,3 @@
+      perror("swapcontext");
+      _exit(0);
+    }
----------------
s/0/1/

================
Comment at: test/asan/TestCases/Linux/swapcontext_annotation.cc:101
@@ +100,3 @@
+      perror("swapcontext");
+      _exit(0);
+    }
----------------
s/0/1/

================
Comment at: test/asan/TestCases/Linux/swapcontext_annotation.cc:136
@@ +135,3 @@
+void handler(int sig)
+{
+  ThrowAndCatch();
----------------
please reformat tests with clang-format
{ should be on the previous line

================
Comment at: test/asan/TestCases/Linux/swapcontext_annotation.cc:155
@@ +154,3 @@
+
+  if (argv[1][0] == 's')
+  {
----------------
Is it useful to run the test without the signal ('x' above)?
I would expect that the signal variant is a strict superset of no-signal. I can't imagine how signals can prevent some failure from happening.
If you agree, then remove the 'x' runs.

================
Comment at: test/asan/TestCases/Linux/swapcontext_annotation.cc:175
@@ +174,3 @@
+
+  char *heap = new char[kStackSize + 1];
+  next_child_stack = new char[kStackSize + 1];
----------------
Run all the following 10000 times (or whatever makes it run for 100ms or so). That will allow to stress interaction with signals better.


http://reviews.llvm.org/D20913