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

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

In http://reviews.llvm.org/D20913#455665, @blastrock wrote:

> I changed the interface to what @filcab suggested. 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.


Like @dvyukov said, having that in test code is fine. The test is already Linux-only anyway :-)
And this exports a simpler API, which is a plus for maintainability of a low-level library ;-) (fewer things to track)

> About the FakeStack stuff, I wrote a new test, planning to do the void** thing @filcab asked for, but the test is already passing on my machine. Could you help me write a failing test before I implement that feature?


I'm unsure what you mean here. Do you mean you wrote a test+implementation to swap the fakestack pointer? Or something else?
Can you share the test?


================
Comment at: lib/asan/asan_thread.cc:125
@@ +124,3 @@
+  if (stack_switching_)
+    Report("WARNING: starting fiber switch while in fiber switch\n");
+  stack_switching_ = true;
----------------
If we're going to error, I'd probably go with a Die() call, since it's a bad use of the library (two start switch without an end in the middle).


================
Comment at: lib/asan/asan_thread.h:135
@@ +134,3 @@
+  };
+  StackBounds GetStackBounds() const;
+
----------------
Indeed. Probably best to leave it as is, then. It's clean enough and we wouldn't hide that much complexity due to the additional friend declarations.

================
Comment at: test/asan/TestCases/Linux/swapcontext_annotation.cc:117
@@ +116,3 @@
+  ThrowAndCatch();
+  __sanitizer_start_switch_fiber(child_context.uc_stack.ss_sp,
+                                 child_context.uc_stack.ss_size);
----------------
Are the `ss_sp` and `ss_size` properties cross-arch? If not, please write getters for them (so we can `#ifdef` different platforms there) since most of this should be "portable enough".

================
Comment at: test/asan/TestCases/Linux/swapcontext_use_after_return.cc:95
@@ +94,3 @@
+  getcontext(&child_context);
+  child_context.uc_stack.ss_sp = child_stack;
+  child_context.uc_stack.ss_size = kStackSize / 2;
----------------
Same as swapcontext_annotation.cc (but with setters).


http://reviews.llvm.org/D20913