[PATCH] D20913: [asan] add primitives that allow coroutine implementations
Filipe Cabecinhas via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 8 07:16:23 PDT 2016
filcab added a subscriber: filcab.
filcab added a comment.
Please split the additions to the `swapcontext` test to a new, "just fiber-related" test case.
That will also make it easier to port that specific test to several fiber platforms, in the absence of `swapcontext` (or to other mechanisms for the same functionality).
More general comments:
Several things need to be updated in `AsanThread` to keep track of fibers. Some of them involve saving old state:
- Stack bounds: Mostly been dealt with, which is good. I think there might be some other things we can do to avoid adding too much complexity.
- `FakeStack`: Missing. It basically needs a place where we can dump a `FakeStack`.
For stack bounds:
How about if `__sanitizer_start_enter_fiber` takes a `void**` where we can stash the current `FakeStack` pointer, and then `__sanitizer_finish_enter_fiber` can overwrite it with a new one if needed?
On `__sanitizer_finish_exit_fiber` we could do the opposite (runtime would need to pass a `void*`, of course), and recover the old `FakeStack` (disposing of the fiber one (there's no documentation, but I'm assuming the `exit_fiber` function is to be called when actually exiting (for the last time) a fiber, not simply when returning to the main stack)).
Without the fake stack handling, we'll lose `use-after-return` errors.
My understanding is that we're calling `__sanitizer_*_enter_fiber` on an anything->fiber transition (be it the first one into that fiber or any other), and calling `__sanitizer_*_exit_fiber` on the *last* (for a given fiber) fiber->anything transition.
Let me know if that's not the case. And if it's not, why it's not generalized like that.
As for the stack bounds: Would it be a problem to figure them out dinamycally in `__sanitizer_finish_enter_fiber`? Possibly with a callback from the fiber library, since we need to know if a fiber is running (Our system's fiber library has a function that queries TLS and tells you if you're running in a fiber. I have no idea about other implementations).
================
Comment at: include/sanitizer/common_interface_defs.h:143
@@ +142,3 @@
+
+ void __sanitizer_start_enter_fiber(const void *bottom, size_t size);
+ void __sanitizer_finish_enter_fiber();
----------------
Needs documentation.
================
Comment at: lib/asan/asan_thread.cc:14
@@ -13,1 +13,3 @@
//===----------------------------------------------------------------------===//
+#include <stddef.h>
+
----------------
Isn't this one of the files that shouldn't include system headers?
================
Comment at: lib/asan/asan_thread.cc:127
@@ +126,3 @@
+ if (fiber_switching_)
+ Report("WARNING: starting fiber switch twice\n");
+ if (!fiber_stack_bottom_) {
----------------
I'd prefer something like "starting fiber switch while in fiber switch".
I'd also make it an assert unless we have actual uses cases of this.
================
Comment at: lib/asan/asan_thread.cc:130
@@ +129,3 @@
+ fiber_stack_bottom_ = bottom;
+ fiber_stack_top_ = bottom + size;
+ } else {
----------------
Why don't you change `stack_{top,bottom}` and, when exiting the fiber, get the bounds from the system?
At the very least (if the proposal above can't be done), we can probably avoid a bunch of code changes by simply having `old_stack_{top,bottom}` members to save the old stuff, and then set `stack_{top,bottom}` to the fiber's bounds. It would also avoid queries to `fiber_stack_*` to see if we should use those or the regular `stack_*`.
================
Comment at: lib/asan/asan_thread.cc:140
@@ +139,3 @@
+ if (!fiber_switching_)
+ Report("WARNING: finishing a fiber enter that has not started\n");
+ if (next_fiber_stack_bottom_) {
----------------
Assert?
================
Comment at: lib/asan/asan_thread.cc:151
@@ +150,3 @@
+ if (fiber_switching_)
+ Report("WARNING: starting fiber switch twice\n");
+ fiber_switching_ = true;
----------------
Assert?
================
Comment at: lib/asan/asan_thread.cc:157
@@ +156,3 @@
+ if (!fiber_switching_)
+ Report("WARNING: finishing a fiber exit that has not started\n");
+ fiber_stack_top_ = fiber_stack_bottom_ = 0;
----------------
Assert?
================
Comment at: lib/asan/asan_thread.cc:162
@@ +161,3 @@
+
+inline AsanThread::StackBounds AsanThread::GetStackBounds() const {
+ if (!fiber_switching_) {
----------------
The struct definition should be in an anonymous namespace defined here in the *.cc file.
This function should be a static function or also put in the anonymous namespace.
================
Comment at: lib/asan/asan_thread.cc:165
@@ +164,3 @@
+ if (fiber_stack_bottom_)
+ return StackBounds{fiber_stack_bottom_, fiber_stack_top_}; // NOLINT
+ else
----------------
ugh, the linter warns here?
================
Comment at: lib/asan/asan_thread.cc:176
@@ +175,3 @@
+ return StackBounds{next_fiber_stack_bottom_, // NOLINT
+ next_fiber_stack_top_}; // NOLINT
+ else
----------------
Line up the `//NOLINT` unless clang-format changes it to be unaligned.
But given the different amount of spaces in these two lines, it seems you should at least run clang-format ;-)
================
Comment at: lib/asan/asan_thread.cc:276
@@ -201,1 +275,3 @@
+ next_fiber_stack_top_ = next_fiber_stack_bottom_ = 0;
+ fiber_switching_ = false;
tls_end_ = tls_begin_ + tls_size;
----------------
Initializing these vars should be done in the `Init()` function, not here.
================
Comment at: lib/asan/asan_thread.cc:450
@@ +449,3 @@
+ if (!t) {
+ Report("WARNING: __asan_enter_fiber called from unknown thread\n");
+ return;
----------------
If it's not a "big" problem to call these functions if we don't know about the thread (hence the warning vs. assert, I guess), then we should probably use `VReport(1, ...)` so we don't warn when verbosity is off, no?
================
Comment at: lib/asan/asan_thread.h:137
@@ +136,3 @@
+ };
+ StackBounds GetStackBounds() const;
+
----------------
Remove this. `GetStackBounds` (and the struct) are only used in the .cc file. You can make them have internal linkage there.
================
Comment at: lib/asan/asan_thread.h:149
@@ +148,3 @@
+ // when the thread is switching from an alternate stack to another alternate
+ // stack, we need to keep both of them during the switch
+ uptr next_fiber_stack_top_;
----------------
Do we need this or can we use the regular `stack_{top,bottom}`?
Do we need to keep track of the old stack bounds?
================
Comment at: lib/asan/asan_thread.h:153
@@ +152,3 @@
+ // true if switching is in progress
+ bool fiber_switching_;
+
----------------
What about fiber vs non-fiber? How do we know when to pick each of `{fiber_,}stack_{top,bottom}`?
http://reviews.llvm.org/D20913
More information about the llvm-commits
mailing list