Page MenuHomeFreeBSD

Include ABI note tag in shared libraries.
ClosedPublic

Authored by jhb on Jun 16 2020, 10:10 PM.

Details

Summary

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.

Test Plan
  • 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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb created this revision.Jun 16 2020, 10:10 PM
jhb requested review of this revision.Jun 16 2020, 10:10 PM
kib added a comment.Jun 16 2020, 11:09 PM

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 ?

In D25304#558060, @kib wrote:

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 ?

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?

jhb added a comment.Jun 17 2020, 6:17 AM

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>

jhb added inline comments.Jun 17 2020, 6:18 AM
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>

jhb updated this revision to Diff 73339.Jun 19 2020, 4:12 PM
  • Assemble crti.o from crti_s.o and crtbrand.o.
  • Fix link of crt1.o.
kib added inline comments.Jun 19 2020, 7:49 PM
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.

jhb added inline comments.Jun 19 2020, 8:37 PM
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.

kib added inline comments.Jun 19 2020, 8:51 PM
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.

jhb added inline comments.Jun 20 2020, 6:41 PM
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.

jhb updated this revision to Diff 73633.Jun 25 2020, 2:08 PM
  • Rebase.
kib added inline comments.Jun 25 2020, 9:06 PM
lib/csu/amd64/Makefile
29 ↗(On Diff #73633)

And I am again confused.

Where the crtbrand.o go ?

jhb added inline comments.Jun 25 2020, 9:17 PM
lib/csu/Makefile.inc
40 ↗(On Diff #73633)

Here we link crti_s.o (generated from per-arch crti.S) and crtbrand.o to build crti.o. This is then included in both shared libraries and executables.

lib/csu/amd64/Makefile
29 ↗(On Diff #73633)

It is part of crti.o now.

jhb edited the summary of this revision. (Show Details)Jun 25 2020, 9:18 PM
kib added inline comments.Jun 25 2020, 9:47 PM
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.

jhb added inline comments.Jun 25 2020, 10:57 PM
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.

jrtc27 added inline comments.Jun 25 2020, 11:07 PM
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...

kib added inline comments.Jun 26 2020, 3:08 AM
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 ?

jrtc27 added inline comments.Jun 26 2020, 12:04 PM
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).

kib added inline comments.Jun 26 2020, 6:00 PM
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) ?

jhb added a comment.Jun 26 2020, 6:41 PM

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.

kib added a comment.Jun 26 2020, 7:09 PM
In D25304#562372, @jhb wrote:

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.

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.

jhb added a comment.Jun 26 2020, 7:23 PM
In D25304#562386, @kib wrote:
In D25304#562372, @jhb wrote:

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.

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.

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.

jhb updated this revision to Diff 74260.Thu, Jul 9, 9:44 PM
  • Rebase and rework to link crtbrand in both crt1.o and crti.o.
kib accepted this revision.Fri, Jul 10, 9:58 AM

You did checked that on all arches binaries get only one brand note ? Also at least on some arch with (external) gcc build ?

This revision is now accepted and ready to land.Fri, Jul 10, 9:58 AM
jhb added a comment.Sat, Jul 11, 6:59 PM

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.

jhb added a comment.Wed, Jul 15, 10:57 PM

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.

kib added a comment.Wed, Jul 15, 11:02 PM

Sorry for being too annoying. So for the external gcc, at leat one binary was built and it contains exactly one abi note ?

jhb added a comment.Thu, Jul 16, 3:48 PM

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.

kib added a comment.Thu, Jul 16, 4:39 PM

Wouldn't 'make buildenv' and make in some binary' directory work at that stage, when both csu and libc are built ?

jhb added a comment.Thu, Jul 16, 5:59 PM

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.

kib added a comment.Thu, Jul 16, 6:25 PM

Great, I accepted the review before.

This revision was automatically updated to reflect the committed changes.