Split the ELF feature note into a separate file that is linked into
*crt1.o the same as crtbrand.S was before. crtbrand.o is now linked
into crti.o on all platforms.
Details
Details
- Reviewers
kib jrtc27 - Commits
- rS363263: Include ABI note tag in shared libraries.
- make tinderbox and readelf -n of libc.so.7 and /bin/sh before/after. /bin/sh still has all the same notes as before, and libc.so.7 now has the FreeBSD ABI tag note
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I wonder if it makes sense to use separate compilation for all .S/.s files and glue them together with ld -r for crti.o instead of includes ?
Comment Actions
That's conceptually nicer. I was initially worried because ld -r is a bit of a strange beast and I wondered how well exercised it is already on the more niche architectures, especially with LLD, but we already use it for kernel modules (even for __KLD_SHARED we first go via ld -r before ld -shared) which should be far more taxing for the linker than these few simple files.
| lib/csu/common/feature_note.S | ||
|---|---|---|
| 31 ↗ | (On Diff #73200) | Why has this gone, forcing the crti.S files to include the header for it, whilst crtbrand.S still includes sys/param.h? |
Comment Actions
We already depend on ld -r to build crt1.o. Building crtbrand.o separately does add more boilerplate to all of the Makefiles. I almost consolidated a lot of the Makefile logic, but i386 has the extra steps to deal with the magic _start1 symbol that no other platforms have, so I couldn't quite collapse it down. Doing so would make it more feasible to have more explicit build rules (right now crti.o is an implicit rule from crti.S). Currently on platforms with crt1_s.S we #include the notes there for similar reasons (more concise build rules), though it's also a bit of inherited behavior as previously the C versions were #included in crt1.c.
| lib/csu/common/feature_note.S | ||
|---|---|---|
| 31 ↗ | (On Diff #73200) | This file isn't used in crti.S (it's the feature note only used in executables) and it's note doesn't use the __FreeBSD_version value from <sys/param.h> |
| lib/csu/arm/crt1_s.S | ||
|---|---|---|
| 47 ↗ | (On Diff #73200) | This #include is only here for the arm file as it has its own special note which needs MACHINE_ARCH. Other crt1.s files do not #include <sys/param.h> |
| lib/csu/amd64/Makefile | ||
|---|---|---|
| 29 ↗ | (On Diff #73339) | I do not understand the patch. There you list feature_note.o as dependency and as input for ld -r. But you also include the feature_note.S into crt1_s.S. |
| lib/csu/amd64/Makefile | ||
|---|---|---|
| 29 ↗ | (On Diff #73339) | That was in the previous change to move the notes into assembly. In that change architectures fell into two camps: some architectures had an assembly entry point in crt1_s.S that was combined with crt1_c.c to build crt1.o, and some just had a C version. For the former set, I #include'd crtbrand.S (which is now feature_note.S) in crt1_s.S. For the latter set, I renamed crt1.c to crt1_c.c and use ld -r to combine crt1_c.o with the note files compiled as .o files. In this change, I am replacing the #include of crtbrand.S in crt1_s.S with an #include of feature_note.S, and on architectures without crt1_s.S, replacing crtbrand.o with feature_note.o. In part I did this because we had depended on #include's before. I could however just always compile the notes separately as .o files even on platforms with crt1_s.S. Then the changes were would be more uniform in the makefiles. Alternatively, I could wait to clean that up in followups if I try to move more of the duplicated makefile bits to Makefile.inc. |
| lib/csu/amd64/Makefile | ||
|---|---|---|
| 29 ↗ | (On Diff #73339) | Well but then feature_note.o should not appear in dependency line from Makefile ? I think it is worth moving all notes to per-note .o files. |
| lib/csu/amd64/Makefile | ||
|---|---|---|
| 29 ↗ | (On Diff #73339) | For platforms that do not have crt1_s.S like amd64, we build the notes as standalone .o. For platforms that have a crt1_s.S I was using #include. I will go back and do an earlier change before this that always uses standalone .o files for notes though as a precursor to this. It's more work due to all the duplication in these Makefiles. |
| lib/csu/amd64/Makefile | ||
|---|---|---|
| 29 ↗ | (On Diff #73633) | And I am again confused. Where the crtbrand.o go ? |
| lib/csu/amd64/Makefile | ||
|---|---|---|
| 29 ↗ | (On Diff #73633) | I see, I think I do not like it. crti.o only defined stubs for prologue/epilogue of init/fini functions, I do not think it is kind of 'ABI mandated' as crt1.o. Binaries linked without crt1.o are not compliant, but I never thought that crti.o could be omitted for same effect. https://docs.oracle.com/cd/E88353_01/html/E37853/crti.o-7.html seems to vaguely support my opinion. |
| lib/csu/amd64/Makefile | ||
|---|---|---|
| 29 ↗ | (On Diff #73633) | How else do you propose to link crtbrand.o into all objects? I had looked at doing this via crtbegin.o instead of crti.o. Originally I didn't do that since I was using #include, but since we are using ld -r always now, I could instead change it to be linked into crtbegin.o if you think that is more correct? We need it linked into all binaries and shared objects. |
| lib/csu/amd64/Makefile | ||
|---|---|---|
| 29 ↗ | (On Diff #73633) | FWIW, NetBSD puts it in their crti.s (via #include "sysident.S"). But maybe crtbegin.o does make more sense given we want to kill off _init/_fini... |
| lib/csu/amd64/Makefile | ||
|---|---|---|
| 29 ↗ | (On Diff #73633) | I think I would be fine if you but the note both into crt1.o and crti.o but make it comdat. Like this .section .note.tag, "aG", @note, .freebsd.noteG, comdat I have further question, what is the intent of providing the note in all shared libraries ? I see the use of it for rtld, but how do you plan to take it into work for real dso ? |
| lib/csu/amd64/Makefile | ||
|---|---|---|
| 29 ↗ | (On Diff #73633) | ldd currently checks the EI_OSABI if you give it a shared library, but AArch64 and RISC-V always use ELFOSABI_NONE and instead rely on just the notes, which are only present in executables. So if you try to run ldd on any of their shared libraries, you get told "not a FreeBSD ELF shared object". Branding all shared libraries would ensure ldd can detect everything correctly by changing it to look for the note (and, presumably, also allowing ELFOSABI_FREEBSD so it still works on old non-AArch64/RISC-V shared libraries). |
| lib/csu/amd64/Makefile | ||
|---|---|---|
| 29 ↗ | (On Diff #73633) | So, if we remove the e_ident[EI_OSABI] check from ldd, or make it conditional on per-arch basis, what are other reasons to have the note in dso (I already listed rtld) ? |
Comment Actions
It also affects what OSABI GDB assigns if you do 'file /path/to/dso' or 'add-symbol-file /path/to/dso'. Mostly this would matter in the context of GDB used on a non-FreeBSD host (on FreeBSD hosts the fallback default OSABI is FreeBSD, so when you fail to recognize a shared library, it ends up still working due to the accident).
Removing the EI_OSABI check on aarch64 and riscv64 means ldd would try to dlopen() Linux shared libraries on those platforms instead of erroring out.
Comment Actions
I would say that this is all cosmetics, but I see that you do not agree.
Then I suggest trying the comdat idea to put the ABI note into both crt1.o and crti.o, to keep crt1.o self-contained ABI marker.
Comment Actions
I think it's just as cosmetic as setting EI_OSABI in the e_ident of the dso. To my mind we should use the brand in all the places we set the OSABI in e_ident as on newer architectures that rely solely on the brand, the brand is the replacement for the EI_OSABI flag. I'm happy to have the note in both though. I'll work on that.
Comment Actions
You did checked that on all arches binaries get only one brand note ? Also at least on some arch with (external) gcc build ?
Comment Actions
Hmm, I got duplicate brands without the change you suggested to use comdat, but with comdat it went back to a single note. However, I did not test with external GCC. I can do some tests with that.
Comment Actions
I tried with external GCC and it seems to have worked, at least as far as the builds got before they died. Recent sys/tree.h changes don't build with GCC 6.
Comment Actions
Sorry for being too annoying. So for the external gcc, at leat one binary was built and it contains exactly one abi note ?
Comment Actions
No problem. It turns out I didn't get a binary built, only libc since the <sys/tree.h> doesn't build with GCC 6 and we end up with libelf failing to build. :( I will probably just start turning off some warnings in GCC 6 to get it green again.
Comment Actions
Wouldn't 'make buildenv' and make in some binary' directory work at that stage, when both csu and libc are built ?
Comment Actions
After a few more bandaids to the build system, I was able to build an amd64 cat binary and it does indeed only have a single copy of the brand note.