libc: properly forward the compat syscall references to libsys

[kib]

Summary

same as it was done for setgroups@FBSD_1.0.

[kib]

Also drop no longer needed private exports from libsys

[olce]

See also questions in D52685.

Apparently, __weakref__ does something we want, at least conceptually. But:

1. It doesn't work without the test you added on PIC: lib/libc/gen/gen-compat.h:56:17: error: weakref declaration must have internal linkage. If I then add static to this declaration, I get link errors like the following:

ld: error: undefined symbol: setgroups@FBSD_1.0
>>> referenced by initgroups.c:67 (/usr/src/lib/libc/gen/initgroups.c:67)
>>>               initgroups.o:(freebsd14_initgroups) in archive /usr/obj/usr/src/amd64.amd64/tmp/usr/lib/libc.a

which is strange because setgroups@FBSD_1.0 exists in libc.a, as reported by nm:

freebsd14_setgroups.o:
                 U .cerror
0000000000000000 T __sys_freebsd14_setgroups
0000000000000000 W _freebsd14_setgroups
0000000000000000 W freebsd14_setgroups
0000000000000000 W setgroups@FBSD_1.0

and I thought the target of __weakref__ is not supposed to be weak itself (contrary to the annotated symbol).
Why is the test on PIC the right thing to do?

2. Why do we even need this in addition to the filtering mechanism? Maybe the latter doesn't work on non-exported symbols?

Sorry if I'm lacking some knowledge in this area, but I'd like to get a minimum of understanding of what is going on.

lib/libc/gen/gen-compat.h

49–55 ↗

(On Diff #162602)

A little more informative names?

[olce]

2. Why do we even need this in addition to the filtering mechanism? Maybe the latter doesn't work on non-exported symbols?

In fact we don't need an addition if using non-weak symbols. By just putting back the declaration in gen-compat.h, but this time for __sys_freebsd14_setgroups, and exporting __sys_freebsd14_setgroups from libsys, I've tested that runtime filtering works correctly. Perhaps because __sys_freebsd14_setgroups isn't a weak symbol. This seems consistent with what @kevans reported in https://reviews.freebsd.org/D52641#inline-312676. Since we are exporting all _<name> and __sys_<name> for non-compat' system calls, I don't see why we wouldn't do that for compat' system calls (we would not export <name> itself).

As another alternative, why the __symver in D52685 works isn't entirely clear to me, but it seems simpler than what is done here.

[kib]

In D52687#1203444, @olce wrote:

2. Why do we even need this in addition to the filtering mechanism? Maybe the latter doesn't work on non-exported symbols?

In fact we don't need an addition if using non-weak symbols. By just putting back the declaration in gen-compat.h, but this time for __sys_freebsd14_setgroups, and exporting __sys_freebsd14_setgroups from libsys, I've tested that runtime filtering works correctly. Perhaps because __sys_freebsd14_setgroups isn't a weak symbol. This seems consistent with what @kevans reported in https://reviews.freebsd.org/D52641#inline-312676. Since we are exporting all _<name> and __sys_<name> for non-compat' system calls, I don't see why we wouldn't do that for compat' system calls (we would not export <name> itself).

Exporting additional symbols from libsys is exactly what I want to avoid. We already export the canonical symbol under the right version, and there is no reason not to use it instead of any other. Plus it minimizes the private version space, making libc less tied to implementation details of libsys, allowing simpler replacement of libsys.

As another alternative, why the __symver in D52685 works isn't entirely clear to me, but it seems simpler than what is done here.

symver purpose it versioning of exported symbols. Investigating whatever bug/misfeature made your change work, is not worth the time IMO.

[kib]

Rename FXX to COMPAT_SYSCALL

[olce]

As another alternative, why the __symver in D52685 works isn't entirely clear to me, but it seems simpler than what is done here.

symver purpose it versioning of exported symbols. Investigating whatever bug/misfeature made your change work, is not worth the time IMO.

I'm still not convinced, and here is some doc to support that, quoting:

If you wish to bind a reference to a specific version of the symbol within the shared library, you can use the aliases of convenience (i.e., ‘old_foo’), or you can use the ‘.symver’ directive to specifically bind to an external version of the function in question.

and I still don't like the PIC trickery to have __weakref__ work (which is due to __weakref__ requiring internal visibility; whereas probably hidden one should be acceptable).

Ultimately, I find it likely that __weakref__ works for the exact same reasons (bugs?) that __symver works:

1. Filtering also applies to symbol references within libc, and not only to symbol references outside of it (else these directives would redirect to the stub implementation in libc).
2. It's also likely there's somehow a filtering bug preventing weak symbols from being "filtered". But referencing a strong one (setgroups@FBSD_1.0) works (in both cases).

But you're the expert more than me, and this fixes the old compat 11 syscalls for the time being, so please go ahead.

[kib]

In D52687#1204129, @olce wrote:

As another alternative, why the __symver in D52685 works isn't entirely clear to me, but it seems simpler than what is done here.

symver purpose it versioning of exported symbols. Investigating whatever bug/misfeature made your change work, is not worth the time IMO.

I'm still not convinced, and here is some doc to support that, quoting:

If you wish to bind a reference to a specific version of the symbol within the shared library, you can use the aliases of convenience (i.e., ‘old_foo’), or you can use the ‘.symver’ directive to specifically bind to an external version of the function in question.

and I still don't like the PIC trickery to have __weakref__ work (which is due to __weakref__ requiring internal visibility; whereas probably hidden one should be acceptable).

No, the PIC condition is to only apply weakref for dynamic linking case: the difference is that compat stubs are not in dynamic libc (and even if they were, they should not be used), but they are in static libc.
For static libc, aliasing symbol to symbol@VER cannot work because @VER is silently ignored if not producing dynamically linked objects.
The 'static' part is just how weakref reasonably requires the symbol to have limited scope because the substitution mechanism only works for single .o.

I can summarize it as following: we want freebsd11_fstat in C source to result in resolution to

• fstat@FBSD_1.0 for libc.so (but we do not need the _definition_ of this fstat, it is in libsys.so)
• freebsd11_fstat for libc.a (since if we make it fstat@FBSD_1.0 for libc.a, then final linkage would be to fstat, which is current syscall, not the compat syscall). libc.a provides freebsd11_fstat.

Ultimately, I find it likely that __weakref__ works for the exact same reasons (bugs?) that __symver works:

1. Filtering also applies to symbol references within libc, and not only to symbol references outside of it (else these directives would redirect to the stub implementation in libc).

Well, this is how ELF work in general: regardless where the symbol reference originated, the symbol lookup is same (not true, the special cases are -Bsymbolic or dlopen(RTLD_DEEPBIND, but libc is not special). After all, this is what allows the symbol interposing.

2. It's also likely there's somehow a filtering bug preventing weak symbols from being "filtered". But referencing a strong one (setgroups@FBSD_1.0) works (in both cases).

I do not understand this statement.

But you're the expert more than me, and this fixes the old compat 11 syscalls for the time being, so please go ahead.

I modeled these changes after the Rust approach to FFI with specific link symbol resolution:

extern "C" {
    #[link_name = "fstat@FBSD_1.0"]
    pub fn fstat(fildes: i32, buf: *mut i32) -> i32;
}

pub fn main() {
    let mut x: i32 = 12;
    unsafe {
	fstat(1, &mut x);
    }
}

Internally Rust issues the substitute of the rust-level symbol by link_name. Then the weakref is the exact match on the level of C compiler or as(1) for what Rust does.

But then, there is something in GNU as(1) that makes weakref less useful than it is in llvm toolchain. GNU as does not allow '@' in the weakref alias, as weird as it sounds. So for instance the rustc --emit=asm output cannot be handled by the gnu as. For the same reason, gcc build is broken right now.

[kib]

Switch to symver to make it compatible with GNU as.

[olce]

Thanks a lot for this detailed response.

In D52687#1204284, @kib wrote:

No, the PIC condition is to only apply weakref for dynamic linking case: the difference is that compat stubs are not in dynamic libc (and even if they were, they should not be used), but they are in static libc.

It seems that stubs are always compiled into libc (via libc/Makefile → libc/sys/Makefile.inc → libsys/Makefile.sys), the only difference being the SHARED_CFLAGS+= -D'_SYSCALL_BODY(name)=' line, basically meaning that libc.so also has all the syscall symbols with their versions, it just doesn't have any implementation for them. [For now, why we are doing that and using auxiliary filtering instead of not including these symbols at all and doing standard filtering is beyond me.]

For static libc, aliasing symbol to symbol@VER cannot work because @VER is silently ignored if not producing dynamically linked objects.

Right, I was coming to a similar conclusion after reading some more material, basically versioning does not work when producing static objects.

I can summarize it as following: we want freebsd11_fstat in C source to result in resolution to

• fstat@FBSD_1.0 for libc.so (but we do not need the _definition_ of this fstat, it is in libsys.so)
• freebsd11_fstat for libc.a (since if we make it fstat@FBSD_1.0 for libc.a, then final linkage would be to fstat, which is current syscall, not the compat syscall). libc.a provides freebsd11_fstat.

Thanks, that's very clear. It would be great to actually turn that into comments before the COMPAT_SYSCALL() definition (I can handle it if you prefer).

2. It's also likely there's somehow a filtering bug preventing weak symbols from being "filtered". But referencing a strong one (setgroups@FBSD_1.0) works (in both cases).

I do not understand this statement.

This is a tentative explanation for why exporting freebsd14_setgroups() from libsys (and thus libc because of the inclusion of Makefile.sys) and providing a declaration in gen-compat.h used in libc links at compile-time but does not work at runtime (apparently because it is resolved to accept() as a side-effect of not providing implementation for syscalls symbols in libc.so; but this would have to be confirmed), while doing the exact same things with __sys_freebsd14_setgroups() actually *works* at runtime (I tested both). This makes me suspect that something might be wrong with filtering (or maybe it's deliberate, but I couldn't find any documentation on this behavior).

GNU as does not allow '@' in the weakref alias, as weird as it sounds.

Yes, it seems absurd that this piece is missing from a consumer's point of view.

[kib]

Copy/paste comment into COMPAT_SYSCALL herald.

[olce]

Thanks!