Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defer to druntime for stack scanning when druntime is in charge of the thread. #417

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

schveiguy
Copy link
Contributor

Going to see how this works with druntime and fibers.

scanned. We can't scan druntime stacks with the sdc mechanism.
@schveiguy schveiguy marked this pull request as ready for review January 21, 2025 15:03
@schveiguy
Copy link
Contributor Author

OK, let's open this up for review. I tried to make this as straightforward as possible. I renamed some functions because they no longer reflect the purpose.

Hard to make a test for this, as it's fibers from druntime that cause the issue.

@deadalnix
Copy link
Contributor

I'm not a fan of this approach. Most of the changes should be bounded to the dmd specific part of the code, or we need some kind of justification that this approach is better regardless of what dmd needs.

@schveiguy
Copy link
Contributor Author

schveiguy commented Jan 29, 2025

What about at least a separate PR where the stack is only scanned if the stackTop is not null? Would that be a reasonable first step? This is the first commit here.

The decision to scan or not scan the stack is based on the thread state being suspended. We need a different mechanism here, and I thought setting the stackTop to null was a decent approach. If the thread isn't suspended, then that stackTop is null, and we can artificially set it to null when we don't want the stack scanned.

The second problem is how to solve the case where the calling thread's stack can't be defined by the stackTop .. stackBottom range. This one is way trickier. I don't love the way it's done here either. Open to ideas. DMD absolutely needs this (and any fiber-based runtime). The right answer is to start registering stacks like druntime does and scanning all those. Is it worth complicating SDC with an equivalent mechanism for this?

What about just explicitly scanning the registers eagerly? You can do this by saving the current stack top, pushing the registers with a shell, and then processing them immediately.

@deadalnix
Copy link
Contributor

I'm a bit confused as to why we want that first step.

If we let druntime scan the stack and TLS, why do we want to call the method that does so for SDC and then customize that method so that it doesn't do it in some specific situations, when we just don't want to call this method at all, no?

@schveiguy
Copy link
Contributor Author

So the main reason is that we track all threads that are created, not just D threads. And in each thread, we initialize a threadcache, populated with allocations for tracking TLS and also freeing the ThreadRunner object.

If we do not scan these threads, at least the TLS, we will collect those allocations, and encounter a double free on thread exit when the cache is flushed.

So we need to scan these threads via SDC's mechanisms (to avoid double free problems), and D threads via druntime's mechanisms (to avoid issues with fibers).

If we did not hook all thread creation, and store the threadcache in TLS for every thread, then we could forego that scanning.

I'm also OK to move in the direction of removing the pthread hook and/or using malloc for all these things, but this is a much bigger undertaking. I was planning to save that until after we have ported the SDC GC to normal D and made it an independent project.

@deadalnix
Copy link
Contributor

Ha, why is it always more complicated than it initially seems? Anyways, ok for not scanning the stack if the bottom is some sentinel value. Let's not make it null because it is the default initialisation, but we can make it all ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants