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

[Philippe Daouadi via llvm-commits]

blastrock marked 17 inline comments as done.
blastrock added a comment.

In http://reviews.llvm.org/D20913#456044, @filcab wrote:

> In http://reviews.llvm.org/D20913#455665, @blastrock wrote:
>
> > 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?


The test is in this diff, the file named swapcontext_use_after_return.cc. I didn't write any implementation as you can see and the test passes on my machine. I don't know how the fakestack mechanism works, so I'm not sure what problem I should be trying to solve by adding the void** argument to __asan_start_switch_fiber().

Just to explain what the test does, it is very similar to the previous test and calls UseAfterReturn() at some tricky points. Moreover, it stores the addresses of function-local dead variables in global pointers so that they can be accessed from outside the fiber. All of these cases correctly trigger a use-after-return error.

I also tried accessing a still-alive function-local variable from outside the fiber which still ran correctly. I didn't commit that part because I think I would need to write a separate file to test for that automatically, since this file tests for the error-case.


================
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);
----------------
filcab wrote:
> 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".
The ucontext struct is described in man getcontext and the uc_stack field is of type stack_t which is described in man sigaltstack. Both say they are part of POSIX and I see no note about architecture-dependency, so I think this is ok.


http://reviews.llvm.org/D20913