Page MenuHomeFreeBSD

Remove the sed hack for ABI tag notes.
ClosedPublic

Authored by jhb on Jun 10 2020, 4:25 PM.

Details

Summary

Use inline assembly to define ELF notes. This permits placing the notes
in a section with the proper type (SHT_NOTE) directly.

Test Plan
  • booted a RISC-V world built with this in QEMU. Have not done a full tinderbox yet. RISC-V uses an OSABI of none, so relies on the ABI tag for all branding.

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 requested review of this revision.Jun 10 2020, 4:26 PM

I wanted to see if we were ok with using this approach. I have some further changes I'd like to make to add the ABI tag note to shared libraries and rtld (right now you can't direct exec rtld on RISC-V for example). However, I'd like to avoid further proliferation of the sed hack to do that. NetBSD uses assembly sources for this instead of C, but this seems like a simpler step forward for us. NetBSD requires a genassym file due to the use of assembly and keeping it in C at least avoids that, though the use of __XSTRING() is perhaps a bit ugly.

lib/csu/common/crtbrand.c
46 ↗(On Diff #72930)

I would be explicit with the NULs to be clear this is exactly 8 bytes as we need to ensure 4-byte alignment of the following word (and to match the namesz above).

lib/csu/arm/Makefile
23 ↗(On Diff #72930)

The use of STATIC_CFLAGS here is a bit curious. All other platforms use CFLAGS

lib/csu/powerpc/Makefile
22 ↗(On Diff #72930)

Oops, the default rule should work here.

lib/csu/common/crtbrand.c
46 ↗(On Diff #72930)

The alternative might be to throw in a .p2align 2 to enforce the padding?

lib/csu/common/crtbrand.c
46 ↗(On Diff #72930)

Then it's less clear that namesz is correct.

  • Trim redundant rule.
  • Explicitly align note payloads.
  • Drop notes.h as it isn't really used.
lib/csu/common/crtbrand.c
46 ↗(On Diff #72930)

So I could use labels to derive namesz (and perhaps even descsz) as the NetBSD webpage suggests.

lib/csu/common/crtbrand.c
46 ↗(On Diff #72930)

Ah yes, that would work.

I very much like the cleanup/simplification that comes from this

lib/csu/common/crtbrand.c
46 ↗(On Diff #72930)

Sounds good to me.

lib/csu/common/ignore_init.c
146 ↗(On Diff #72932)

I wonder if we could make this 0?

  • Use labels to compute namesz and descsz.
  • Revert "Drop notes.h as it isn't really used."

rtld uses notes.h, so I put it back. I haven't (yet) dropped the #includes of it in crtbrand.c and ignore_init.c. I could perhaps make more use of __XSTRING to use the values from notes.h in the inline assembly, but that seems a bit ugly?

lib/csu/common/ignore_init.c
146 ↗(On Diff #72932)

That's a good question. I'd rather do that as a separate change if so though.

Did you considered making crtbrand.c a standalone .S and then link it into crt1.o with ld -r ? It is pure asm, using asm() for it IMO causes more problems like XSTRING than it provides a convenience.

In D25211#555894, @kib wrote:

Did you considered making crtbrand.c a standalone .S and then link it into crt1.o with ld -r ? It is pure asm, using asm() for it IMO causes more problems like XSTRING than it provides a convenience.

Yes, that seems to be a much cleaner solution. In any case, I'm completely in favor of anything that gets rid of the SED_FIX_NOTE hack... :)

I thought about making it a standalone .S file. It would mean more work in the various Makefiles to link it in? I guess they'd all have to be like i386. NetBSD had to use a genassym file. I'd like to avoid that, but not sure we can include, e.g. <sys/param.h> in assembly to get __FreeBSD_version. I'll play with it and see.

In D25211#556000, @jhb wrote:

I thought about making it a standalone .S file. It would mean more work in the various Makefiles to link it in? I guess they'd all have to be like i386. NetBSD had to use a genassym file. I'd like to avoid that, but not sure we can include, e.g. <sys/param.h> in assembly to get __FreeBSD_version. I'll play with it and see.

param.h definitely has #ifdef LOCORE stuff so it seems to be used by some asm files.

  • Move notes into assembly.
  • Permit <sys/elf_common.h> and <sys/param.h> to work in userland assembly.
  • Convert RISC-V CSU.

I've updated RISC-V to use notes defined in assembly and uploaded it to see what it looks like. I will work on more architectures tomorrow.

lib/csu/common/crtbrand.S
31 ↗(On Diff #73002)

This seemed less work than getting it added into ACFLAGS in various places at least as a hack, but I'd be fine moving this out to ACFLAGS if that is preferred.

I think that riscv csu readability is improved with both notes and asm block moved to .S files.
For LOCORE, can you put it into AFLAGS in lib/csu/Makefile.inc ?

Overall this looks fine for riscv, and should be similarly good for other arches.

  • Convert i386.
  • Define LOCORE in common ACFLAGS.
  • Update amd64.
  • Update aarch64.
  • Update arm.
  • Update mips and powerpc*.
  • Consistent sorting.
  • Use .4byte.
  • Simplify MACHINE_ARCH for arm.
  • ARM requires %note rather than @note.
  • Move NT_FREEBSD_ARCH_TAG to assembly.
This revision is now accepted and ready to land.Jun 12 2020, 6:30 PM

@imp, can you review the sys/arm/include/param.h change?

A 'make tinderbox' passed for me, and 'readelf /usr/obj/<mumble/*/bin/sh' shows all the right notes present in each /bin/sh binary.

These look great. You could do a few of them as separate commits, if you wanted (like the arm param.h), but it's not big deal either way.
I didn't pedantically audit all the new assembler (git is confused about where it came from :).

This revision is now accepted and ready to land.Jun 13 2020, 9:45 PM

I will do the arm param.h separately, but the lib/csu bits have to be all in one go.

This revision was automatically updated to reflect the committed changes.