Thanks for the reviews! I do not have commit access, so please merge this whenever you can.

Thank you for the answers.

Indeed, I'm closing this then, I haven't tested your patch yet but the fix seems to be the exact same. You seem to have a better idea of what you are doing and your patch reflects that ^^

No, D23765 seems to fix this issue.

I got what "shadows" means, this patch can handle a UsingDecl with multiple shadows.

I'm new to this and I'm not sure at all this is the correct way to fix it. Any input is welcome :)

Seems fine to me, but I think you forgot to update the tests :)

I don't have a windows machine at hand, I didn't test this commit.

I also don't understand why it is needed for these new functions and not for some other functions like __sanitizer_print_memory_profile.

I guess I should add INTERFACE_FUNCTION(__sanitizer_*_switch_fiber) to that file. Should I open a new review on phabricator?

Thank you for your time reviewing this! :)

I added some documentation in common_interface_defs.h.

In D20913#462251, @filcab wrote:

As I commented above, I think adding a way to kill a fake stack without being in the fiber (so: from outside the fiber that "has" that fake stack) would be nice. But I'm ok with doing this internally if there's no interest in it from open source.
In your fiber library, is a use case where the fiber returns to the thread and then re-enters possible? And is it possible to have the fiber go back to the thread and the thread decide to either switch back to the fiber or kill the fiber?

No, in my library, when a fiber returns, it is completely destroyed. And we never kill a fiber from outside before it is finished.
This use-case came to my mind but I thought it wasn't a real world use-case, I was wrong ^^

In D20913#462104, @dvyukov wrote:

One consequence is that the fake frame that should be allocated when entering the fiber is not, only the functions called from there have one.

That's a consequence of manual annotations. If a program uses swapcontext, and we intercept and annotate it, then it won't happen, right?

Correct. But it's not that bad anyway I think. If the fake stack is there to find use-after-return errors, when the fiber entry function returns, it means the fiber is dead, so there is no real need to track those anymore. It's like a thread dying, and in both cases (thread or fiber) the fake stack is destroyed and it won't detect use-after-returns as such.

I think it is fine. The guideline for manual annotations should then recommend to introduce an additional function that merely finishes the switch and calls real non-inlinable fiber body. Worth noting in __sanitizer_start_switch_fiber comment, and, yes, it needs some comment.

Yes, I was planning on writing a few lines of documentation when we all agree on the interface.

I don't see any new test files. Did you forget to add a file?

I didn't add it. The test proposed by @filcab is based on printfs inside the asan library to show when fake frames are collected. We couldn't write an automatic test for that.

I think I got how fake stacks work. I implemented your idea @filcab, and with your test code, the FakeFrame is not collected anymore.

Hm... shouldn't I fix @filcab's issue with FakeStack before?

Wow, thanks for taking the time to explain all this ^^
I have a few questions about it.

Here's what's new:

• replaced warnings by asserts when calling start/finish_switch_fiber in wrong order
• simplified code with only one atomic as suggested by @dvyukov
• clang-formatted the two test files
• removed the useless x/s argument from the test, the signal now always triggers
• replaced ThrowAndCatch by a call to a noreturn function that will longjmp back
• replaced some exit(0) by exit(1) in the test
• loop 30 times instead of doing a single run

setjmp/longjmp seems to work, the test is now correctly passing. The only thing is that I loop only 30 times and it already incurs a 1s total overhead (4 compilations at all optimization levels) on my machine, quite far from the 10000 you wished for. 100 iterations gives 5s overhead, ~60s for 1000 iterations.

@dvyukov I wrote that loop to run the tests longer while the signal triggers, and I have found... a deadlock :(

In D20913#456044, @filcab wrote:

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

In D20913#455864, @dvyukov wrote:

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.

I only see the APIs in test code. Is it correct?
Tests are OK. swapcontext is also non-portable.

Yes, I didn't add non-portable code to the library itself. I said that because it forces users to write non-portable code to annotate their code, but it's acceptable.

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.

In D20913#453386, @filcab wrote:

The way I see it, you could do something like (I'm giving vars/functions/members totally different names to avoid ambiguity with the current names. Feel free to keep something closer to your current ones):
__sanitizer_fiber_switch_start(void *ctx, uptr size);
This function is passed the fiber's or the thread's stack bounds, and sets next_stack_{top,bottom}.

__sanitizer_fiber_switch_finish();
This function is run in the new stack that you switched to and removes the current stack_{top,bottom}, replacing with what was in next_stack_{top,bottom} (setting the latter ones to 0, after).

That way, you only have one primitive to deal with (This follows from our simple fiber implementation. Yours might have functionality that requires the current implementation. Please let me know).

My implementation does not know about the thread's stack bounds. It could ask them through pthread and store them in some thread_local storage to avoid asking them each time.

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

I found one of these tests you are talking about in tsan/signal_sync.cc. It seems to trigger a signal every 10 microseconds, but even with that, isn't the probability that the signal triggers right in between __sanitizer_enter() and swapcontext() very low? I think it would be useful to add such a test, but not replace the current one by it, as the current one is deterministic and tests all tricky cases (except the signals).

Because then I will not be able to write something like

I added support for switching from one fiber to another and added that to the unit test. I also added a few ThrowAndCatch calls there to try to trigger the warning that this patch fixes in __asan_handle_no_return.

Ok, I think I did everything you asked for, except for the interception of the swapcontext calls, I will come back at it later.

You generally can do that by taking an address of any local variable.

If we are looking for the stack of the current thread, yes. But there are functions that need to know the stack of other threads, like GetThreadRangesLocked. It is the only such function I have found but I don't know if there are others.

If we express the annotations as start_switch/fnish_switch, then we have more flexibility to handle it. First, we can handle signal masks internally. Second, we can remember both old and new stacks for the duration of the switch, and then check both in __asan_handle_no_return. This will eliminate the need to mess with signal masks.

Oh, I see!

In D20913#448009, @dvyukov wrote:

I see that folly always uses main thread stack as scheduler stack: always switches main->fiber/fiber->main, but not fiber1->fiber2. Some programs avoid the excessive switch by doing fiber1->fiber2. We need to make sure that we support that pattern as well.

I think this is easy to support, we can remove the warning about entering a fiber twice, then, for example, you could go main -> enter_fiber -> fiber 1 -> enter_fiber -> fiber 2 -> exit_fiber -> main.

I wonder if it can interfere with signals (without altstack). Namely, a signal arrives when asan has setup fiber stack, but the actual switch did not happen (SP is still pointing to the old stack). Then signal handler executes something that causes __asan_handle_no_return. Seems that we can get the same crash. Need tests.

I didn't think about that. I think you are right, it will cause the warning in __asan_handle_no_return and the false positives afterwards. However, I don't know how we could avoid that... The user doing the annotation could take care of that by deferring signals between the enter_fiber and the switch to the new stack. Do you have any other idea?

In D20913#447864, @dvyukov wrote:

Philippe, what coroutine implementation do you use?

I use boost::context as you seem to have guessed

We should intercept and annotate at least ucontext API, so that applications using ucontext work out of the box. It will also simplify testing and make it more realistic, as we will just need to write a real program using ucontext.

So you mean redefining makecontext and swapcontext in compiler-rt, annotate them and call libc's implementation after that? Is there an example of such a code for another api?

Unfortunately, boost::context uses own assembly to implement coroutines, so we still need to expose annotations. We can talk to boost maintainers to add annotations directly to boost::context.

I can try to make them a patch for that when I'm done with this. For the moment I added the annotations around boost::context calls.

Thank you!

Oh I see! Indeed, I don't have commit rights, so I would help me if you could do it :)

Sorry, I'm not sure I got what is the workflow on this platform. This patch is now "accepted", but do I need to do something else here? The only relevant action I can do seems to be "close revision", should I do that?

Do not include pthread.h and sched.h when threads are disabled