Switch Android onto the sectionELF style of module registry, but leave out the unused register_dso and friends#2172
Conversation
gen/modules.cpp
Outdated
| LLValue *params[] = {b.getFalse(), minfoRefPtr}; | ||
| b.CreateCall(registerDSO, params); | ||
| if (global.params.targetTriple->getEnvironment() != llvm::Triple::Android) | ||
| b.CreateCall(registerDSO, params); |
There was a problem hiding this comment.
Why would any of the code be needed in the executable? This still emits the registerDSO functions and ctors/dtors that do nothing.
There was a problem hiding this comment.
I thought about removing the call to buildRegisterDSO also, but figured I'd keep this patch minimal. As for the ctor/dtor, I initially simply switched away from the linked list of module info and built it, saw the linker complain that it couldn't find _d_dso_registry, then took out the two calls that added it. It took a couple minutes, as I wasn't about to dive into all the complexity of everything else you do here.
If you think I should just remove this entire last block instead on Android, I can do that.
There was a problem hiding this comment.
What I'm asking for is to just not call any of the stuff that actually generates executable code. This should amount to always taking the !isFirst early return above.
There was a problem hiding this comment.
Doesn't seem like that'll work as I need the __start___minfo/__stop___minfo symbols in druntime, as mentioned above. I'll investigate.
There was a problem hiding this comment.
__start_minfo/__stop_minfo are generated by the linker. The code here merely uses them (to achieve something along the lines of what you are doing in druntime).
There was a problem hiding this comment.
OK, if you say so. :) I don't want to try and understand everything you did here, hence simply reverting that druntime commit on Android for so long.
I just tried exiting early in emitModuleRefToSection where you indicated and the druntime test runner builds and all modules pass, same with a small sample file. I'll try the phobos test runner and dmd testsuite next and update this PR in the way you suggested once everything passes.
There was a problem hiding this comment.
Passes all the tests when done the way you wanted, so updated the PR.
|
Be aware that this might break |
|
|
How do you avoid the module info references being garbage collected? |
|
I'm unclear exactly how I've not looked into why it keeps some sections and throws out others, let me know if you want me to investigate, if there's something I should look for. I had the impression |
joakim-noah
changed the title
joakim-noah
changed the title
|
Submitted the druntime fixes upstream, dlang/druntime#1851, would be nice to get these two pulls in before the final 1.3 release, so I can reduce the patches needed for my ldc Termux package for Android. Anything else holding this pull up, David? |
|
Should be fine - feel free to merge if "green". An explanation in the code would be good, though - the rest of the module registry code is fairly well documented (although it should probably be split up into multiple functions). |
…g upstream, but leave out the dso_registry stuff that's unused on Android.
|
Thanks for the review, added comment and merged. If you approve the upstream druntime pull linked above, I'll go ahead and merge it in the ldc branch. |
2 participants
dmd moved from the linked list at
_Dmodule_refto a bracketed section a couple years ago, so I've been reverting that druntime commit for ldc/Android till now. Turns out I can use thissectionELFcode just fine, as long as I take out the calls to_d_dso_registryand modify druntime to use the slightly different ldc symbols__start___minfoand__stop___minfo, druntime PR forthcoming. Confirmed by cross-compiling the stdlib tests from linux/x64 to Android/ARM with ldc master, and running the stdlib and dmd testsuite natively with master and merge-2.074.I didn't look very closely at
sectionELFhowever, just hacked this in, so if David thinks anything else might cause issues, let me know.