-
-
Notifications
You must be signed in to change notification settings - Fork 608
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
[WIP] fix issue 4071 and others: shared runtime DLL for Windows #14849
Conversation
|
Thanks for your pull request, @rainers! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#14849" |
|
Doesn't this imply that both the executable and the dll must each be compiled monolithically, i.e. no separate compilation? |
Yes. There is also an option to only dllimport symbols from druntime/phobos which covers building executables that use the runtime in a DLL. This could be expanded to allow specifying the affected packages/modules on the command line. IIRC that's what Benjamin Thaut also proposed. |
|
Martin told me that the export everything runs up against a 64K limit on exports for Windows DLLs. |
| bool isDllImported(Dsymbol var) | ||
| { | ||
| if (target.os & Target.OS.Posix) | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since dllimport is a windows-only thing, the test should be "if not Windows return false".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same test is used in createImport(). I have changed it to Windows-only nonetheless.
This might be an issue in the long run, but not that different from C++ libraries that export a class hierarchy. Qt5Widgets.dll has >9000 exports, Qt5Gui.dll >25000 exports. druntime and phobos lie in the same ballpark with 14000 and 13000, respectively, according to my last experiments. LDC's DLLs are a bit below that. |
In my ~100+k LOC codebase, it's sitting around 9k exports from my rough estimate recently. This could be decreased by a worthwhile amount with the implicit construction of sum types in function arguments. It isn't going to be a worry unless you do templates galore while exporting everything or scope your binaries very badly IMO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve this because it is behind a switch, so is not the default behavior, and it will support a behavior some people want #15298 (comment)
|
@rainers Is there any chance we can get this rebased? I've been hammering on Walter to get the DLL situation fixed, so we should be good to merge if you are. |
|
I have a rebase at https://github.com/thewilsonator/dmd/tree/rainer_dll but since @kinke redid the makefiles, will need to redo the specific commit that changed |
|
Long term, perhaps it should be a step in the pipeline steps? For now, an optional new pipeline seems to be arguably ok. |
|
Also this looks to be missing the argument reconciliation logic https://github.com/dlang/dmd/pull/15964/files#diff-527cf1019708a4dc99987160134da80d3a0628c6df7a207192ddf3e34575ace7 |
Although the description says this should happen (copied from LDC), it is a change to how |
|
|
I'm struggling with the mixture of (UT_)-DRUNTIMESO and DRUNTIMESOLIB... |
allow writable module info enable CI tests for shared runtime
|
Yay, it's green! Is this good to go now? |
|
The test/shared tests didn't rerun for the shared runtime, now cleaning the generated folder. I'm trying to build the link/lib pair for Windows, too. That will show some remaining issues... |
As expected, these show that there is runtime library support missing for clients of the shared druntime:
I think these should be done in separate PRs. |
|
A thank you to you Rainer over this PR: https://forum.dlang.org/post/ymkubdpxsdnzgypidkwd@forum.dlang.org |
|
Thank you @rainers and @thewilsonator for getting this across the finish line at light-speed! |
This is work in progress.
Basic idea is copied from LDC:
Runtime support for properly attaching to the GC still missing.