core.sys.elf: Move shared ELF types and values to a common module#3784
core.sys.elf: Move shared ELF types and values to a common module#3784RazvanN7 merged 1 commit intodlang:masterfrom ibuclaw:coresyself
Conversation
ibuclaw
requested review from
Burgos,
MartinNowak,
andralex and
wilzbach
as code owners
|
Thanks for your pull request, @ibuclaw! 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 + druntime#3784" |
| extern (D) | ||
| { | ||
| auto ELF32_ST_VISIBILITY(O)(O o) { return o & 0x07; } | ||
| alias ELF32_ST_VISIBILITY ELF64_ST_VISIBILITY; | ||
| } |
There was a problem hiding this comment.
For reviewers:
Solaris defines ELF32_ST_VISIBILITY differently to all other Linux/BSDs (where it's return o & 0x03).
| struct Elf32_Nhdr | ||
| { | ||
| Elf32_Word n_namesz; |
There was a problem hiding this comment.
For reviewers:
Regarding ELF notes, while there might be some common note sections, these are largely platform-specific.
| struct Elf32_Cap | ||
| { | ||
| Elf32_Word c_tag; |
There was a problem hiding this comment.
For reviewers:
The ELF capability table is a platform specific section.
dkorpel
left a comment
dkorpel
left a comment
There was a problem hiding this comment.
@WalterBright has been against efforts to factor out common definitions between platforms because of the following pattern:
- Even though there's a lot in common, there's still a lot of subtle differences sprinkled all over the place
- Someone encounters an error in the definition on one platform
- The definition is located and fixed
- Oops! Because the versioning logic wasn't simple, other platforms were inadvertently changed as well, and now they're broken
Now if it really is a Glibc header and the separation between the common module and platform-specific parts is rock solid, then I think we're okay. I'm don't have much experience with different linux platforms, but I trust your judgement.
There's also libelf for non-elf platforms, so to say these are OS specific is wrong.
It says glibc/elf/elf.h in the reference URL, not glibc/sysdeps/linux/elf/elf.h. So any platform that uses glibc as its primary C runtime would have the same bindings - Linux, Hurd, kFreebsd, kSolaris. Do we really want 3000 lines of bindings copied 4 times? Also wrt Linux, has anyone consulted uclibc, musl? Maybe there's differences between them and glibc, in which case all of core.sys.linux.elf is invalid bindings. |
dkorpel
left a comment
dkorpel
left a comment
There was a problem hiding this comment.
I suggest waiting a bit before merging this so @WalterBright (or someone else) has a chance to chime in.
Having a talk with some people, there is the gABI spec (https://refspecs.linuxfoundation.org/elf/gabi4+/contents.html), and well as many psABI's to describe ELF for a specific processor (x86_64, SPARC, IA64, RISCV). It'll take time to unpick, but what's described there could be used as the baseline, and everything else thrown in os bindings. It probably won't reduce the amount of duplication by 4000, but it will by some measure though. |
|
@ibuclaw I think this can be merged now, or do you want to supersede this and go straight to bindings based on the gABI spec? |
|
Ping @ibuclaw |
4 participants
As noted in #3780, core.sys.linux.elf is really bindings for a Glibc header, rather than a Linux one. However, the same structures and constants crop up across all OS-specific modules, of which the BSD ones are all copies of the FreeBSD elf module.
This commonizes it all into a core.sys.elf package. Per-OS macros and values still remain.
Bootstrapped on x86_64-pc-solaris2.11 without any issues, lets see what the testsuite has to say about Linux and FreeBSD.