Fix gc crash

[roberth]

I've figured out another cause of gc crashes, related to coroutines.

This now solves

• a crash that occurs whenever a gc-registered thread is in a coroutine (the boehmgc patch)
• an inappropriate "stack overflow" when gc runs while a coroutine exists (the BoehmGCStackAllocator commit)

I'll run nixUnstable with this PR and #4895 to see if any other crashes occur.

[jonringer]

should the boehm-gc patch be upstreamed? looks like it addresses some of the FIXME comments above

[roberth]

looks like it addresses some of the FIXME comments above

The existing fixmes in boehm are for an unrelated feature on very rare architectures where the stack grows in the other direction. I did not implement those fixmes because we don't need it. Otoh, I couldn't resist writing the code to support these rare archs in my own part. I'll remove it, so the next person won't think it matters to Nix.

[roberth]

should the boehm-gc patch be upstreamed?

As it is, it is part of a very specific solution that works for Nix, which isn't ideal because it scans garbage stack sections sometimes. That's ok because it's a conservative garbage collector and in the case of Nix those garbage sections will not be scanned anymore when the coroutine has ended. This is the main reason I have doubts about upstreaming.
A more accurate implementation involves more changes as outlined in the inline comments in the patch. These affect both boehmgc and boost_coroutine2.
So the PR is a local optimum. With significant effort, it can be made accurate and upstream-worthy, but it improves Nix only marginally, so it's not something I would enjoy doing in my free time.

[manveru]

Can confirm that this fixes a segfault we experience when building github:input-output-hk/daedalus/4.1.0#daedalus with GC enabled.

[domenkozar]

should the boehm-gc patch be upstreamed?

As it is, it is part of a very specific solution that works for Nix, which isn't ideal because it scans garbage stack sections sometimes. That's ok because it's a conservative garbage collector and in the case of Nix those garbage sections will not be scanned anymore when the coroutine has ended. This is the main reason I have doubts about upstreaming.
A more accurate implementation involves more changes as outlined in the inline comments in the patch. These affect both boehmgc and boost_coroutine2.
So the PR is a local optimum. With significant effort, it can be made accurate and upstream-worthy, but it improves Nix only marginally, so it's not something I would enjoy doing in my free time.

That's fair. Could we open an issue and explain what we did, so they're aware of our use case?

[edolstra]

Really great work, thanks!

Yeah it would be good to get upstream's ideas about this.

[roberth]

While formulating my question I've found issues I hadn't seen before, with some recommendations. I'll summarize my findings here.
ivmai/bdwgc#150 is an interaction with someone who seems to have found a similar solution. Since then, functions have been added to manipulate the recorded stack base per thread. Maintaining these stack base pointers requires a gc lock around the coroutine context switch. While the current approach to scanning the coroutine stacks works, it'd be better to provide our own stack "pushing" routine as mentioned here, which runs as part of gc and does not need to worry about synchronization as much. It also may help with potential mark stack overflows, although I don't think we've experienced those.
So most of this will be helpful when writing a more accurate solution.
For now our solution is similar to ivmai/bdwgc#150 and we err on the side of overly conservative gc when it comes to stacks during coroutine execution.

[lilyball]

The boehmgc patch only applies to Linux. Boehm GC has a separate "stop the world and push thread stacks" implementation for Darwin (see darwin_stop_world.c) and this still crashes when GC is invoked from a coroutine.

[roberth]

The boehmgc patch only applies to Linux.

This was an oversight. See #4919

crashes when GC is invoked from a coroutine.

It's more frequent than that. The main cause is that an OS thread has its stack pointer in a coroutine, which is not supported by an unpatched GC. Which thread (or stack) invokes the GC doesn't really matter beyond that.