Fix Issue 23177 - ModuleInfo is not exported on Windows #14200
Conversation
|
Thanks for your pull request and interest in making D better, @rikkimax! 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 references
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 "stable + dmd#14200" |
|
Welp I've messed up something to do with bugzilla referencing. |
rikkimax
changed the title
You can |
The fix is coming next, as soon as I make Windows fail the CI, so I'll word it like that, thanks! |
|
Windows except for OMF has failed the CI. This is what I was wanting to see, pushing fix now. |
|
Good news and bad news. Good news: this PR is doing what I set out to do! Bad news: The test runner is not aware of import and export library generation and is causing failures because MSVC linker is writing a message i.e. |
|
Unfortunately, the test runner is going to have to be linker aware of this particular message. Otherwise, we are going to need to make the tests themselves aware of linkers (which is far worse). I don't think there is any way around this. MSVC linker just doesn't let you disable this message and export tables need to be used far more heavily with D if we want decent shared library support. |
rikkimax
force-pushed
the
fix23177
branch
2 times, most recently
from
4474626 to
677b882
Compare
|
I don't know how to solve this:
|
There definitely is - LDC only exports the ModuleInfos with Note that the export is simple, but the accompanying dllimport is tricky for data symbols, especially if referenced in static data initializers (such as ModuleInfos in other binaries referencing dllimported ones). LDC emits a CRT constructor to 'relocate' these manually at startup. For some context, see |
That comment was only about the message that MSVC linker emits to say that it is generating import and export files. Test suite was hard coded for one output, but it adds output, therefore failed. I did solve it, via changing the test runners to remove the message (it does it for dmd already). |
I know, LDC had the same problem of course. But I don't think it's desirable to always export them (after all, the user sees the linker messages and extra files too), and DMD needs the |
|
Yeah, unfortunately, I'm at the end of what I can do. Unless somebody tells me how to solve that latest problem, this PR is dead sadly. |
|
This is apparently just some MS linker shortcoming that you're hitting now with the embedded /EXPORT directives. See https://github.com/ldc-developers/dmd-testsuite/blob/04d6f174b71032ea2e7ba96db81ed136e6eb1329/runnable/testmodule.d#L3-L7. |
Excellent! Thank you. I couldn't find this documented anywhere. Now I can disable that test :D |
…generation message which causes failures when generated.
…th Unicode symbol names
|
@thewilsonator: Please revert. As the test adaptations clearly show, this shouldn't be done unconditionally! |
It wouldn't matter if we did revert it, the problem still remains. We need to modify mangling so that Unicode will not be emitted for MSVC linker with extern(D). runnable/testmodule.d is an incomplete test, dshell/dll should be including a similar module in its test, which would also not work right now. I've already taken a look mangling code, it doesn't look like it's as simple as just turning on compression or something else, so it's out of my area to fix. |
|
I've found where in dmangle I would need to change, mangleIdentifier but I'm going to need somebody to tell me what to transform it into, e.g. hex. |
|
If we can't find a way around the mangling issue, @kinke is right, we'll need to revert for our non-english speakers, I was wrong about that. |
|
[I wasn't referring to the Unicode issue, but the unconditional export in general, see the revert PR above for rationale.] I don't understand why the extended |
The CI was run with only the test change. It definitely was broken without the fix. |
|
Yeah, but I meant checking the produced code, e.g., running |
Yeah you're asking for something a bit different than what I did. I left it to the linker/system linker to tell me if symbol X was missing. Rather than an explicit where it was referencing it (objdump failed on me to do this during my previous testing of shared library support). |
|
I understand, and the added test should IMO cover the expected behavior just fine. But knowing the problems I had to tackle for this with LDC, I'm just wondering how on earth it can work, and expect some other problem to hide it. So I meant checking the .exe locally on your box with a DMD build, just for confirmation, not complicating the test itself (for now at least, as long as my suspicion hasn't been confirmed yet). And maybe checking that the exe doesn't export some stub or so itself ( |
Okay this comment won't be helpful, as I'm going to need a patched copy of dmd for this. So I'll need to compile dmd with this PR. |
Yeah, I assumed you had one lying around anyway for local testing. :) - I can check it myself too, but can't promise when I find some time. |
|
Nah it's all good, I'll compile it. I try to do as much leg work on these sorts of issues as I can, at the very least it means that somebody else doesn't have to do it and can read my failed attempts instead :) |
|
Ugh, this is going to take me longer than I was expecting. |
|
Output (ignoring Kernel32.dll, cos that won't be helpful): So yeah, it's emitting the reference into the imports @kinke. |
|
Hmm. Please try this main() in the executable and see if you hit no access violation and sane output: void main() {
import std.stdio;
foreach (m; ModuleInfo) {
writeln(m.name);
if (auto i = m.importedModules()) {
foreach (j; i)
writeln(" - ", j.name);
}
}
} |
|
Oh goodness, this really is far more broken than I thought. void main() {
import std.stdio;
foreach (m; ModuleInfo) {
writeln(m.name);
if (auto i = m.importedModules()) {
writeln(i);
stdout.flush;
foreach (j; i) {
writeln(" - ", j.name.ptr);
stdout.flush;
}
}
}
} |
|
Okay, finally. ;) - I guess the import lib contains some seemingly dangerous |
|
At least now we know for certain that it isn't just a simple fix of setting export on ModuleInfo. In the meantime, I'm throwing some more code at the CI to try and solve the mangling issue. #14207 |
I'm first only adding the test for this to ensure it does in fact break what I think it breaks, then I'll push the fix (which I expect to be a one liner).