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

[Philippe Daouadi via llvm-commits]

blastrock added a comment.

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

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


Oh, that what they are used for! I think what you said could work, I must give it more thought.

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


No, the second part is not exact (or I'm misunderstanding you). __sanitizer_*_exit_fiber are used when returning from the fiber to the thread's stack. The fiber may be re-entered later.
To be clear:

- you call enter_fiber when you are about to switch to a fiber
- you call exit_fiber when you are about to switch back to the thread's stack

These are not related to the fiber's lifetime.

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


I'm not sure I understand. You want to avoid giving the stack bounds as arguments to __sanitizer_start_enter_fiber? We actually need to know the stack bounds we are switching to beforehand, in case a signal handler runs on the fiber, before `__sanitizer_finish_enter_fiber` is called.

@dvyokov I tried the signal triggering stuff with setitimer, but the resolution (on my machine at least) seems to low. The signal triggers 0 to 1 time, never close to critical parts. I even tried letting it run for 5min with a while (true) of the test, it didn't trigger any bad behavior.
Instead, I tried adding calls to ThrowAndCatch() in between the lines of StartEnterFiber() and the others functions directly in the library (of course I can't commit that), and I found a few bugs with that, not fixed yet.
My point is that using setitimer to trigger these bugs seems useless, do you still want me to do it? I have no other solution to unit-test that though...


================
Comment at: lib/asan/asan_thread.cc:14
@@ -13,1 +13,3 @@
 //===----------------------------------------------------------------------===//
+#include <stddef.h>
+
----------------
filcab wrote:
> Isn't this one of the files that shouldn't include system headers?
I don't know about that. I use it for size_t to implement the functions exposed in common_interface_defs.h. I can move these implementations in another file if needed, or use uptr if that makes sense, but I'm not sure I can use it in common_interface_defs.

================
Comment at: lib/asan/asan_thread.cc:127
@@ +126,3 @@
+  if (fiber_switching_)
+    Report("WARNING: starting fiber switch twice\n");
+  if (!fiber_stack_bottom_) {
----------------
filcab wrote:
> 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.
No use case comes to my mind, I'm ok with making this an assert.

================
Comment at: lib/asan/asan_thread.cc:130
@@ +129,3 @@
+    fiber_stack_bottom_ = bottom;
+    fiber_stack_top_ = bottom + size;
+  } else {
----------------
filcab wrote:
> 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_*`.
> 
see my response to your comment on asan_thread.hh:149

================
Comment at: lib/asan/asan_thread.cc:165
@@ +164,3 @@
+    if (fiber_stack_bottom_)
+      return StackBounds{fiber_stack_bottom_, fiber_stack_top_}; // NOLINT
+    else
----------------
filcab wrote:
> ugh, the linter warns here?
It says that a semicolon after a closing brace is useless, it probably thinks of this as a block...

================
Comment at: lib/asan/asan_thread.cc:176
@@ +175,3 @@
+      return StackBounds{next_fiber_stack_bottom_, // NOLINT
+                         next_fiber_stack_top_};  // NOLINT
+    else
----------------
filcab wrote:
> 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 ;-)
I think I did run clang-format, I will do it again. I just need to use the default llvm style, right?

================
Comment at: lib/asan/asan_thread.cc:450
@@ +449,3 @@
+  if (!t) {
+    Report("WARNING: __asan_enter_fiber called from unknown thread\n");
+    return;
----------------
filcab wrote:
> 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?
I don't know if this can actually happen, so I don't know what would be the consequences. I guess that it would only trigger the warning in __asan_handle_no_return later, so this one can be made a verbose log, since nothing really bad has happened yet.

================
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_;
----------------
filcab wrote:
> Do we need this or can we use the regular `stack_{top,bottom}`?
> Do we need to keep track of the old stack bounds?
I think we do. This is there to handle the case where the user does:

```
__start_enter_fiber();
// a signal triggers here and the handler throws which triggers __asan_handle_no_return
swapcontext();

// in the fiber we just switched to
// a signal may trigger here too
__finish_enter_fiber();
```

On each call of `__asan_handle_no_return` between the `__start` and `__finish_enter_fiber`, we need to check on which stack we actually are running on, so we need both of them. This is what is done in GetStackBounds()

================
Comment at: lib/asan/asan_thread.h:153
@@ +152,3 @@
+  // true if switching is in progress
+  bool fiber_switching_;
+
----------------
filcab wrote:
> What about fiber vs non-fiber? How do we know when to pick each of `{fiber_,}stack_{top,bottom}`?
If fiber_stack_* are not null then we are on the fiber, unless fiber_switching_ is true which means that we don't know if we are on fiber stack or thread stack.


http://reviews.llvm.org/D20913