Page MenuHomePhabricator

blastrock (Philippe Daouadi)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 25 2014, 6:35 AM (264 w, 6 d)

Recent Activity

Nov 26 2016

blastrock abandoned D27116: Fix crash when using __has_nothrow_copy with inherited constructors.

No, D23765 seems to fix this issue.

Nov 26 2016, 4:04 AM

Nov 25 2016

blastrock updated the diff for D27116: Fix crash when using __has_nothrow_copy with inherited constructors.

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

Nov 25 2016, 12:56 PM

Nov 24 2016

blastrock added a comment to D27116: Fix crash when using __has_nothrow_copy with inherited constructors.

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

Nov 24 2016, 3:02 PM
blastrock retitled D27116: Fix crash when using __has_nothrow_copy with inherited constructors from to Fix crash when using __has_nothrow_copy with inherited constructors.
Nov 24 2016, 2:59 PM

Sep 16 2016

blastrock added inline comments to D24628: [ASAN] Pass previous stack information through __sanitizer_finish_switch_fiber.
Sep 16 2016, 12:42 PM

Sep 15 2016

blastrock added a comment to D24628: [ASAN] Pass previous stack information through __sanitizer_finish_switch_fiber.

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

Sep 15 2016, 2:15 PM

Jun 21 2016

blastrock added a comment to D21557: Fix asan_win_dll_thunk.cc test.

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

Jun 21 2016, 9:10 AM
blastrock retitled D21557: Fix asan_win_dll_thunk.cc test from to Fix asan_win_dll_thunk.cc test.
Jun 21 2016, 9:07 AM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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

Jun 21 2016, 7:26 AM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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

Jun 21 2016, 7:04 AM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

Thank you for your time reviewing this! :)

Jun 21 2016, 6:15 AM
blastrock updated the diff for D20913: [asan] add primitives that allow coroutine implementations.

I added some documentation in common_interface_defs.h.

Jun 21 2016, 12:57 AM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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

Jun 21 2016, 12:46 AM

Jun 20 2016

blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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.

Jun 20 2016, 6:58 AM

Jun 19 2016

blastrock updated the diff for D20913: [asan] add primitives that allow coroutine implementations.

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

Jun 19 2016, 10:11 AM

Jun 14 2016

blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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

Jun 14 2016, 10:58 AM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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

Jun 14 2016, 8:02 AM

Jun 13 2016

blastrock updated the diff for D20913: [asan] add primitives that allow coroutine implementations.

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
Jun 13 2016, 1:24 PM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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.

Jun 13 2016, 12:55 PM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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

Jun 13 2016, 12:01 PM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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

Jun 13 2016, 10:54 AM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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.

Jun 13 2016, 4:29 AM

Jun 12 2016

blastrock updated the diff for D20913: [asan] add primitives that allow coroutine implementations.

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.

Jun 12 2016, 12:08 PM

Jun 11 2016

blastrock added inline comments to D20913: [asan] add primitives that allow coroutine implementations.
Jun 11 2016, 3:12 AM

Jun 10 2016

blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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.

Jun 10 2016, 12:32 PM

Jun 9 2016

blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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.

Jun 9 2016, 2:48 AM

Jun 7 2016

blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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

Jun 7 2016, 2:53 AM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

Because then I will not be able to write something like

Jun 7 2016, 2:02 AM

Jun 6 2016

blastrock updated the diff for D20913: [asan] add primitives that allow coroutine implementations.

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.

Jun 6 2016, 12:32 PM
blastrock added inline comments to D20913: [asan] add primitives that allow coroutine implementations.
Jun 6 2016, 8:14 AM

Jun 5 2016

blastrock updated the diff for D20913: [asan] add primitives that allow coroutine implementations.

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

Jun 5 2016, 1:53 PM

Jun 3 2016

blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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.

Jun 3 2016, 6:23 AM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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!

Jun 3 2016, 5:57 AM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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?

Jun 3 2016, 4:27 AM
blastrock added a comment to D20913: [asan] add primitives that allow coroutine implementations.

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.

Jun 3 2016, 2:46 AM

Jun 2 2016

blastrock retitled D20913: [asan] add primitives that allow coroutine implementations from to [asan] add primitives that allow coroutine implementations.
Jun 2 2016, 8:30 AM

Aug 27 2015

blastrock added a comment to D9639: Do not include pthread.h and sched.h when threads are disabled.

Thank you!

Aug 27 2015, 11:40 AM
blastrock added a comment to D9639: Do not include pthread.h and sched.h when threads are disabled.

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

Aug 27 2015, 10:44 AM
blastrock added a comment to D9639: Do not include pthread.h and sched.h when threads are disabled.

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?

Aug 27 2015, 10:29 AM

Jul 14 2015

blastrock added reviewers for D9639: Do not include pthread.h and sched.h when threads are disabled: danalbert, jroelofs.
Jul 14 2015, 1:21 AM

May 9 2015

blastrock updated D9639: Do not include pthread.h and sched.h when threads are disabled.
May 9 2015, 9:47 AM
blastrock updated the diff for D9639: Do not include pthread.h and sched.h when threads are disabled.

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

May 9 2015, 9:45 AM
blastrock retitled D9639: Do not include pthread.h and sched.h when threads are disabled from to Do not include pthread.h and sched.h when threads are disabled.
May 9 2015, 9:37 AM