[PATCH] D20913: [asan] add primitives that allow coroutine implementations
Filipe Cabecinhas via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 13 06:21:34 PDT 2016
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 @@
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 @@
+ child_context.uc_stack.ss_sp = child_stack;
+ child_context.uc_stack.ss_size = kStackSize / 2;
Same as swapcontext_annotation.cc (but with setters).
More information about the llvm-commits