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

fix issue 4071: improve support for shared runtime #15974

Merged
merged 7 commits into from
Jan 15, 2024
Merged

Conversation

rainers
Copy link
Member

@rainers rainers commented Jan 1, 2024

  • register module info of clients and run module ctors/dtors
  • register .data section of clients with the GC

The link/lib test case for shared runtime works now but for exceptions on Win64, although support for proper scanning of TLS is still missing.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @rainers!

Bugzilla references

Auto-close Bugzilla Severity Description
4071 normal Missing support to share memory and objects between DLLs and executable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#15974"

druntime/src/core/sys/windows/dll.d Outdated Show resolved Hide resolved
druntime/src/core/sys/windows/dll.d Outdated Show resolved Hide resolved
druntime/src/core/sys/windows/dll.d Outdated Show resolved Hide resolved
druntime/src/core/sys/windows/dll.d Outdated Show resolved Hide resolved
druntime/src/core/sys/windows/dll.d Outdated Show resolved Hide resolved
void finiTLSRanges(void** teb) nothrow @nogc
{
}
void scanTLSRanges(void** teb, scope void delegate(void* pbeg, void* pend) nothrow dg) nothrow
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now added scanning of TLS of multiple DLLs.

The static version of this section could be replaced with the generic one used for version (Shared) at the cost of a couple of indirections. That would allow removing bracketing symbols generated here: https://github.com/dlang/dmd/blob/master/compiler/src/dmd/backend/mscoffobj.d#L1201

@rainers rainers force-pushed the rainer_dll2 branch 2 times, most recently from 6d63698 to f44d51f Compare January 2, 2024 11:49
- run checks in release builds, too
- side effects in assert() never executed in release builds
- respect different termination order on windows
@rainers
Copy link
Member Author

rainers commented Jan 5, 2024

@kinke I could avoid adding a _d_dso_register call by hooking rt_initSharedModule into dll_process_attach and d_run_main2. Would you consider adopting that for LDC or should I make these DMD-specific?

@kinke
Copy link
Contributor

kinke commented Jan 5, 2024

I probably wouldn't adopt it; I'm using https://github.com/ldc-developers/ldc/blob/master/runtime/druntime/src/rt/dso.d now, and that seems to work fine, no need for any dllmain etc.

@kinke
Copy link
Contributor

kinke commented Jan 5, 2024

That said, the whole rt.sections* stuff could use a bigger cleanup/refactoring, it's a total mess on the LDC side, as rt.sections_elf_shared has been generalized for Mac and Windows too. And I've had use cases for getting image sections (similar to the .minfo ModuleInfo pointers section) in user code, so those portions would ideally be moved to core.internal or the like.

There were some more druntime adaptations for druntime DLL support on Windows that could be upstreamed now that DMD is apparently getting there too. Edit: I think the main ones were ldc-developers/druntime#197 and ldc-developers/druntime#198.

@rainers
Copy link
Member Author

rainers commented Jan 5, 2024

Actually, my first attempt was starting from LDCs section_elf_shared.d, but that got even messier. Then I realized that only rather small parts need to be added on the Windows side, so opted for this way.

I probably wouldn't adopt it; I'm using https://github.com/ldc-developers/ldc/blob/master/runtime/druntime/src/rt/dso.d now, and that seems to work fine, no need for any dllmain etc.

I guess it needs a bit more compiler magic to link to an extra static library that include dso.d. I'll version the hooks on DigitalMars.

@kinke
Copy link
Contributor

kinke commented Jan 5, 2024

I guess it needs a bit more compiler magic to link to an extra static library that include dso.d. I'll version the hooks on DigitalMars.

Yeah, some tiny magic to include that object file (must be linked in!) when linking a shared library, and bundling that prebuilt object file with the install packages. But it replaced all the previous compiler magic in every object file, so made things much simpler in that regard.

@rainers
Copy link
Member Author

rainers commented Jan 6, 2024

Basic exception support added now, good enough to make the tests in lib/link work. Some of the tests in test/exceptions still fail, though.

assert_fail.d exhibits linker issues with symbols exported by the DLL, but also defined by the executable. The statically linked library does the same, but both definitions being COMDATs it doesn't bother the linker.

@RazvanN7
Copy link
Contributor

@schveiguy Do you think you can review this? I know that you are well versed with the innards of druntime.

@RazvanN7 RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Jan 11, 2024
@schveiguy
Copy link
Member

@RazvanN7 gotta be honest, I don't understand much about windows and dlls. Sorry.

{
if (!cond) assert(false);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether we shouldn't just remove the -release in

DFLAGS+=-O -release

For LDC, I've similarly added -check=assert=on in https://github.com/ldc-developers/ldc/blob/1d4c2cfc5e17e6cfcf7cd154d5e66394162fa981/runtime/druntime/test/profile/Makefile#L26.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be ok, but omitting -release also keeps other stuff and makes optimizations the only difference to the debug build.

@thewilsonator thewilsonator added Merge:auto-merge and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Jan 15, 2024
@dlang-bot dlang-bot merged commit ae7fffe into master Jan 15, 2024
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants