Support merging of EH tables across Win64 DLLs #2874
Conversation
adamdruppe
requested review from
andralex,
CyberShadow,
rainers and
wilzbach
as code owners
|
Thanks for your pull request and interest in making D better, @adamdruppe! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. 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 fetch digger
dub run digger -- build "master + druntime#2874" |
src/rt/dmain2.d
Outdated
| ehSetFn ehSet = cast(ehSetFn) GetProcAddress(mod, "_d_setEhTablePointer"); | ||
|
|
||
| typeof(ehGet()) libraryEh; | ||
| if (ehGet) |
There was a problem hiding this comment.
Please write this as
if (!ehGet)
return mod;
libraryEh = ehGet();
...
src/rt/dmain2.d
Outdated
| // need to clear the DLL's table out of the global list, so first that | ||
| // means fetching the dll's table... | ||
| ehGetFn ehGet = cast(ehGetFn) GetProcAddress(ptr, "_d_innerEhTable"); | ||
| if (ehGetFn) |
src/rt/dmain2.d
Outdated
| { | ||
| foreach (idx, ge; ehTablesGlobal) | ||
| { | ||
| if (ge == entry) |
There was a problem hiding this comment.
if (ge != entry)
continue;
...
src/rt/sections_win64.d
Outdated
| return pbeg[0 .. pend - pbeg]; | ||
| } | ||
|
|
||
| // finished |
There was a problem hiding this comment.
what is this comment referring to?
There was a problem hiding this comment.
it opens with
// Used for EH table merging across DLL boundaries
and then
// finished
ends that block.
|
Another high-level question for y'all: on Win32, this just works because it uses the built-in operating system functions. Why doesn't it just do that on Win64 as well? I'm pretty sure RaiseException works just as well there. |
Yeah, that's pretty unfortunate. Walter didn't want to go the last 5 percent (rough guess, most of the work for unwinding tables seems done). Not using standard exceptions is also a pain in a debugger, because you cannot break on an exception being thrown. LDC uses C++ exception handling on Windows with a slightly modified termination handler. |
src/rt/dmain2.d
Outdated
| @@ -321,7 +410,7 @@ private alias extern(C) int function(char[][] args) MainFunc; | |||
| * runs embedded unittests and then runs the given D main() function, | |||
| * optionally catching and printing any unhandled exceptions. | |||
| */ | |||
| extern (C) int _d_run_main(int argc, char** argv, MainFunc mainFunc) | |||
| export extern (C) int _d_run_main(int argc, char** argv, MainFunc mainFunc) | |||
There was a problem hiding this comment.
AFAICT export produces exported symbols on every executable, too. I would recommend just using a def file in case you actually need any of these symbols being exported.
There was a problem hiding this comment.
oh yeah i meant to remove that before pushing it here but missed a spot.
I was also experimenting with actually making druntime.dll and sprinkled export in parts around the runtime to get it to load. I actually got it sort of working but virtual functions always jumped to 0xffffffffff - i figure that data must not have been indirected correctly by dmd then scaled back my ambitions to this PR.
(I think druntime.dll is the correct solution for my plugin problem but I ran out of time trying to get that path to work.)
There was a problem hiding this comment.
For proper shared runtime/library support, see https://dconf.org/2016/talks/thaut.html
There was a problem hiding this comment.
oh yeah i meant to remove that before pushing it here but missed a spot.
I meant all other functions with changed export, too. For DLL support, I implemented an -exportall switch a couple of years ago. It does what's also done on linux, i.e. export all symbols.
There was a problem hiding this comment.
Yeah, I looked at Thaut's code too, it looks like it did most the work reasonably well so I might try to revive that fork and get it merged when my urgent todo list is a bit shorter.
When/if that gets in, I would then delete all this new code and the GC proxy stuff too I'm pretty sure is useless witht hat.
There was a problem hiding this comment.
When/if that gets in, I would then delete all this new code and the GC proxy stuff too I'm pretty sure is useless witht hat.
I don't remember whether Benjamins' code handled the non-standard exception handling in Win64. I'm not familiar with the implementation, but it doesn't look like it would work out of the box by just patching up import relocations.
adamdruppe
force-pushed
the
win64eh
branch
3 times, most recently
from
3094547 to
c1e88f2
Compare
|
On Tue, Dec 17, 2019 at 11:56:04AM -0800, Rainer Schuetze wrote:
I meant all other functions with changed export, too. For DLL support, I implemented an `-exportall` switch a couple of years ago. It does what's also done on linux, i.e. export all symbols.
On exported symbols from exe, that's basically useless right?
I'm pretty sure GetProcAddress only works on dlls..
|
It was meant to export all symbols of a DLL, an executable would then be linked against its import library and doesn't need to export symbols itself.
It works on any module regardless of extension. That includes the executable. |
adamdruppe
force-pushed
the
win64eh
branch
6 times, most recently
from
2fc6a7a to
deb84f2
Compare
|
sooooo the test appears to be failing because the output says and the test doesn't allow that. am i misreading it? |
Yeah, the output of the test is verified. You can get rid of the error by not automatically exporting the symbols, but rely on a def file for the DLL. Speaking of tests, can you add one in druntime/test/shared or enable some of the posix tests on Windows, too? |
|
The tests from https://github.com/dlang/druntime/pull/2828/files might help here. |
|
Yeah, I think my next step will be to merge those PRs. I'm not in love with the name comparison but it works as a short-term thing. And then the def file is OK too. I just need to fix my Windows build environment, I totally messed it up again this week :( |
|
just pulled john colvin's branch into here too, let's see how the tests go with that..... BTW i updated visual studio earlier and everything broke for the druntime dev. dmd master make -f win64 broke. at least build.d still worked. then druntime make utterly failed - apparently the update killed all the paths it depended on. what a hassle. |
|
BTW I also can't run the tests locally, the MS C compiler complains no include path. Again surely related to the path breakage from that VS update. so fickle. so i'll just push here to run the unittests. at least i can compile my own things again though. |
There are quite some more places that expect pointer identity of TypeInfo instances, e.g. Line 205 in e6b89f6 Line 779 in e6b89f6 Line 238 in e6b89f6 druntime/src/gc/impl/conservative/gc.d Line 3468 in e6b89f6 grep for "typeid.*is" for a couple more places. I'm not sure we want to pay the performance penalty of a string compare in all these places. Maybe some of these can be left as is because they are compiler callbacks where both TypeInfo instances are guaranteed to be from the same binary, but that's not the case for its usage in the (precise) GC. |
|
optlink doesn't like the empty def file, it must at least contain "EXETYPE NT" |
|
Any updates on this? Getting this to the finish line would be great. |
|
oh over christmas i forgot about this and never got back to it after. I think it basically works though, just a few minor fixes and prolly update to master.... |
| @@ -13,6 +13,15 @@ | |||
| */ | |||
| module rt.cast_; | |||
|
|
|||
| // because using == does a dynamic cast, but we | |||
| // are trying to implement dynamic cast. | |||
There was a problem hiding this comment.
Since this is comparing for equality, the name for it should be something like "equals".
| { | ||
| if (a is b) | ||
| return true; | ||
| return (a && b) && a.info.name == b.info.name; |
There was a problem hiding this comment.
When would the ClassInfo ever be null?
There was a problem hiding this comment.
I vaguely recall seeing it happen in December when working on this originally but I don't remember where. Possible I was just being defensive too.
|
For reference, #3138 goes the opposite direction, because template names can be the same for different parameter types. I think the name generation should be fixed before it can be used for type comparison. |
|
I think it's well worth pursuing this and getting it ready to merge. If someone can point me at wherever the names get generated (I found it once, but I can't find it now) I'll see if I can do something there. |
|
OK, I'm coming back to this now. ... idk where that name generation is, I kinda wish it was in druntime, RTInfo could just plop a hash(T.mangleof) and ctfe that right up. ...in fact i could just add that here... |
|
ah the generation is in Which is defined in that same file and seems to actually do the work. @rmanthorpe |
EH across DLLs needs more work with DMD on Win64, see dlang#2874.
EH across DLLs needs more work with DMD on Win64, see dlang#2874.
|
John's PR included here has landed in #3543, so this would definitely need a rebase now, reducing it to the DMD-specific WinEH fix only. The testcase only needs the removal of druntime/test/shared/src/dynamiccast.d Lines 78 to 82 in a8df891 |
|
@adamdruppe are you willing to pursue this? |
|
This was finished almost two years ago. I don't even know what the new merge conflicts are anymore... I think half of it can simply be deleted as other MRs got it, just the table merge itself, and even that only in dmd, since ldc and gdc im pretty sure solved the problem on the compiler side. |
|
Druntime have been merged into DMD. Please re-submit your PR to |
EH across DLLs needs more work with DMD on Win64, see dlang/druntime#2874.
I'm not 100% happy with this yet but wanna get your input at the early stages.
Like the GC proxy, I don't think this is a good solution in the end, but it can be a temporary helper to people trying to use dlls today.
Currently, if you throw an exception in a dll, it cannot cross the boundary correctly for two reasons: the dynamic cast not working (John Colvin already has an open PR to address that... hackily again, but eh) and the EH tables being separate.
This PR addresses the latter problem by merging the tables through a little indirection. Combined with Colvin's PR, we can throw and catch exceptions across dll boundaries.
Longer term, I'd like to see druntime as a dll as a viable option, but that's pretty far off right now and we need something short term too.
My worry with this implementation is it might need some synchronization. I also kinda think all the initLibrary/unloadLibrary stuff would better belong in DllMain. But I don't want to get too invasive at this point.