libc: add per-arch backwards compatability symbol control

[emaste]

Summary

The FreeBSD/arm ABI changed in FreeBSD 10 and the previous one is no longer supported as of FreeBSD 11. FreeBSD/arm64 and FreeBSD/riscv have no releases prior to FreeBSD 11. Do not enable symver backwards compatabilitiy for earlier FreeBSD releases.

FreeBSD 7 is the minimum LIB_MIN_COMPAT as t is the one that introduced libc.so.7.

Do not enable any symver backwards compatability when building WITHOUT_SYMVER.

PR: 207712
Sponsored by: The FreeBSD Foundation

[emaste]

__sym_compat is also used in:
lib/libthr/thread/thr_sem.c
lib/libthread_db/thread_db.c
so we'd need access to the per-arch minimums and the MK_SYMVER disable in their Makefiles too.

Maybe we can set ${LIB_MIN_DEFAULT} somewhere in share/mk and just add CFLAGS+=-DLIB_MIN_DEFAULT=${LIB_MIN_DEFAULT} to the three Makefiles

[emaste]

See also D5601, which addresses this for a few FreeBSD 7 compat functions on AArch64.

[andrew]

lib/libc/arm/Makefile.inc

17

I think this should be 10, we changed to the ARM EABI in 10 and removed the ability to use the old ABI in 11.

[emaste]

lib/libc/Makefile

133

this test is backwards

lib/libc/arm/Makefile.inc

17

this will be 10

[brooks]

lib/libc/gen/semctl.c

44

Probably shouldn't declare the compat versions unless we will use them.

lib/libc/sparc64/Makefile.inc

14

Even if it's a no-op, the should probably be 5.

[emaste]

lib/libc/sparc64/Makefile.inc

14

Or should it be 7, the first version with libc.so.7?

[brooks]

lib/libc/sparc64/Makefile.inc

14

Indeed. Probably also i386 and amd64.

[emaste]

Review feedback

[emaste]

lib/libc/gen/semctl.c

44

Oops, missed that. If everything else looks fine will commit w/ that change.

[emaste]

Linking tests failed on arm64:

--- all_subdir_libexec ---
ld_library_pathfds.o: In function `setup':
/scratch/tmp/emaste/freebsd/libexec/rtld-elf/tests/ld_library_pathfds.c:169: undefined reference to `openat'
ld_library_pathfds.o: In function `opendirat':
/scratch/tmp/emaste/freebsd/libexec/rtld-elf/tests/ld_library_pathfds.c:220: undefined reference to `openat'
/scratch/tmp/emaste/freebsd/libexec/rtld-elf/tests/ld_library_pathfds.c:220: undefined reference to `openat'
cc: error: linker command failed with exit code 1 (use -v to see invocation)
*** [ld_library_pathfds.full] Error code 1

[brooks]

The one other thing that needs to be done is to filter the set of syscall stubs. Probyle with something in lib/libc/sys/Makefile.inc like:

.if ${ARCH_LIB_MIN_COMPAT} > 4
MIASM:=        ${MIASM:Nfreebsd4_*}
.endif
.if ${ARCH_LIB_MIN_COMPAT} > 6
MIASM:=        ${MIASM:Nfreebsd6_*}
.endif
# There are no COMPAT5 syscalls
.if ${ARCH_LIB_MIN_COMPAT} > 7
MIASM:=        ${MIASM:Nfreebsd7_*}
.endif

[bdrewery]

lib/libc/sys/openat.c

44

You need this weak reference to fix the linking error

lib/libc/sys/setcontext.c

43

And this one

lib/libc/sys/swapcontext.c

44

And this one

[bdrewery]

I'm probably wrong. It's a bit confusing.

[kib]

Why do we need this ?

It adds significant complications IMO, while the result is slightly less versions of some symbols exported.

lib/libc/sys/openat.c

45–46

openat in FBSD_1.2 only exists due to libthr bug. If app only linked to libc, then previous versions of libc provided default FBSD_1.1 symbol.

[emaste]

In D5650#120967, @kib wrote:

Why do we need this ?

It adds significant complications IMO, while the result is slightly less versions of some symbols exported.

I don't think it's a significant complication, but in any case I extended what I had done for SYMVER_COMPAT as it conflicted with what Brooks started in D5601.

[brooks]

In D5650#120969, @emaste wrote:

In D5650#120967, @kib wrote:

Why do we need this ?

It adds significant complications IMO, while the result is slightly less versions of some symbols exported.

I don't think it's a significant complication, but in any case I extended what I had done for SYMVER_COMPAT as it conflicted with what Brooks started in D5601.

My motivation is to avoid shipping compatibility bits in platforms that don't need them. I started to run into this with CheriABI, but it also applies to arm64 and riscv. One could argue for not removing bogus compat bit from releases we've already shipped, but let's not add them to new ones. The added complexity here isn't significant compared to libc as a whole.

[emaste]

• add openat, setcontext, swapcontext to map file for the case where compat is not used
• update openat, setcontext, swapcontext compat to 11 - they're not actually FreeBSD 8 or 9 compat, but a fix for inconsistent symver versions in multiple libraries

[emaste]

lib/libc/Makefile

133–150

Although it seems that we don't actually test SYMBOL_VERSIONING anywhere

lib/libc/gen/semctl.c

33–34

Also wrap this in #if LIB_MIN_COMPAT <= 7

[kib]

I still do not like it, and probably I can argue why. Suppose that in 11 we changed the ABI of the symbol read. Then we would get read@@FBSD_1.4 (the default version) and read@FBSD_1.0 (compat shims). What should be N in LIB_MIN_COMPAT <= N check ? Now, suppose that the change is merged to stable/10 and stable/9. Does it affect code in HEAD ?

In fact, this hack is ugly demonstration of our invalid use of symver. The mechanism was designed for very much different use. It was supposed that a new version namespace is created when any symbol is added or ABI of a symbol changes.

lib/libc/Makefile

133–150

I agree, this define is useless, and should be removed.

[emaste]

In D5650#123571, @kib wrote:

I still do not like it, and probably I can argue why. Suppose that in 11 we changed the ABI of the symbol read. Then we would get read@@FBSD_1.4 (the default version) and read@FBSD_1.0 (compat shims). What should be N in LIB_MIN_COMPAT <= N check ?

In light of your comment I think some of the tests in the review are incorrect.

In your case it should be 11: if compat for any FreeBSD < 11 is needed, read@FBSD_1.0 must be provided.

[emaste]

• Update compat #ifs based on @kib's comment: the test should be LIB_MIN_COMPAT < n is the FreeBSD n.0 release that introduced the ABI change.
• Remove unnecessary -DSYMBOL_VERSIONING

[emaste]

Add @jhb (as the author of one of the compat blocks)

[kib]

In D5650#123942, @emaste wrote:

In D5650#123571, @kib wrote:

I still do not like it, and probably I can argue why. Suppose that in 11 we changed the ABI of the symbol read. Then we would get read@@FBSD_1.4 (the default version) and read@FBSD_1.0 (compat shims). What should be N in LIB_MIN_COMPAT <= N check ?

In light of your comment I think some of the tests in the review are incorrect.

In your case it should be 11: if compat for any FreeBSD < 11 is needed, read@FBSD_1.0 must be provided.

IMO this is still wrong. For LIB_MIN_COMPAT 10 you would not get read@FBSD_1.0.

My opinion is that this approach is not workable. The only possible way to save the patch is to offer a binary knob, either we provide whole symver-ed universe, or we only export default versions. In the example, it would be only exporting read@@FBSD_1.4.

[emaste]

In D5650#123950, @kib wrote:

In D5650#123942, @emaste wrote:

In D5650#123571, @kib wrote:

I still do not like it, and probably I can argue why. Suppose that in 11 we changed the ABI of the symbol read. Then we would get read@@FBSD_1.4 (the default version) and read@FBSD_1.0 (compat shims). What should be N in LIB_MIN_COMPAT <= N check ?

In light of your comment I think some of the tests in the review are incorrect.

In your case it should be 11: if compat for any FreeBSD < 11 is needed, read@FBSD_1.0 must be provided.

IMO this is still wrong. For LIB_MIN_COMPAT 10 you would not get read@FBSD_1.0.

I don't follow: in your read example we'd have:

#if LIB_MIN_COMPAT < 11
__sym_compat(read, old_read_compat_fn, FBSD_1.0);
#endif

No FreeBSD/arm64 and FreeBSD/riscv release exists with a read@FBSD_1.0 and they now don't get the compat sym. Every other architecture has LIB_MIN_COMPAT set to 10 or lower, and so includes read@FBSD_1.0.

[emaste]

• Rebase after rS297619
• Add comment on LIB_MIN_COMPAT usage
• Remove syscall wrappers for unsupported releases from MIASM
• Update i386/amd64/powerpc/powerpc64/sparc64 ARCH_LIB_MIN_COMPAT as a consequence of including syscall wrapper stripping

[emaste]

Fix LIB_MIN_COMPAT setting - we need it as a make variable in addition to a -D compiler define

[kib]

I clicked the button, but my opinion is still that the thing is overcomplicated.

I am sure that it will regress very quickly without pestering of the people who understand this stuff.

lib/libc/Makefile

146

This is yet another place for re to modify when making new major release. It should be commented about, and somehow pointed to for re, which maintains internal list of such adjustments for the branching.

Could this be deduced automatically from OSVERSION ?

[bdrewery]

Not sure if this is still relevant but no objection from me.