I think it is backward. I do not see a reason to have these live in libsys, esp. they 'fat' C implementation. Placing them in libc is more logical IMO, and it seems that the only reason they were attempted to be moved to libsys is use of the __libsys_interposed array. with __libsys_interposed_slot() you can place them in libc trivially, same as I handled sleep/usleep(3).

In D44111#1006732, @kib wrote:

I think it is backward. I do not see a reason to have these live in libsys, esp. they 'fat' C implementation. Placing them in libc is more logical IMO, and it seems that the only reason they were attempted to be moved to libsys is use of the __libsys_interposed array. with __libsys_interposed_slot() you can place them in libc trivially, same as I handled sleep/usleep(3).

Sure, I can do that instead.

In D44111#1006732, @kib wrote:

I think it is backward. I do not see a reason to have these live in libsys, esp. they 'fat' C implementation. Placing them in libc is more logical IMO, and it seems that the only reason they were attempted to be moved to libsys is use of the __libsys_interposed array. with __libsys_interposed_slot() you can place them in libc trivially, same as I handled sleep/usleep(3).

Looking into it more, I'm less certain this is the right take. These are all thin wrappers that would now get a little more expensive due to an extra function call. More importantly, why should interposed system calls have their <sys>() interface provided by libc, when non-interposed ones come from libsys since they are the same as _<sys>() and __sys_<sys>()? Conceivably we could generate <sys>() for non-interposed ones if we really want to go that route, but I'm not excited by doing that.

(Note that this review only fixes the ones where I messed up the move by not moving symbol map entries, there are another ~38 of them that were previously in lib/libc/sys and thus already handled.)

One argument in favor of moving all the interposed functions back to libc is that this review stack effectively reverts rGbd6060a1c661b3. I find the log message for that commit frustrating as it doesn't describe the reported issue (correctness, performance, ???), only the change.

In D44111#1007126, @brooks wrote:

One argument in favor of moving all the interposed functions back to libc is that this review stack effectively reverts rGbd6060a1c661b3. I find the log message for that commit frustrating as it doesn't describe the reported issue (correctness, performance, ???), only the change.

The commit message is quite explicit explaining the breakage (libthr interposition of the signal handlers was overwritten, which makes libthr critical sections nonoperational and cancellation broken among other things).

In D44111#1007124, @brooks wrote:

In D44111#1006732, @kib wrote:

I think it is backward. I do not see a reason to have these live in libsys, esp. they 'fat' C implementation. Placing them in libc is more logical IMO, and it seems that the only reason they were attempted to be moved to libsys is use of the __libsys_interposed array. with __libsys_interposed_slot() you can place them in libc trivially, same as I handled sleep/usleep(3).

Looking into it more, I'm less certain this is the right take. These are all thin wrappers that would now get a little more expensive due to an extra function call. More importantly, why should interposed system calls have their <sys>() interface provided by libc, when non-interposed ones come from libsys since they are the same as _<sys>() and __sys_<sys>()? Conceivably we could generate <sys>() for non-interposed ones if we really want to go that route, but I'm not excited by doing that.

(Note that this review only fixes the ones where I messed up the move by not moving symbol map entries, there are another ~38 of them that were previously in lib/libc/sys and thus already handled.)

My take is ideally libsys.so should only export 'real' syscalls, and anything that is handled by intermediate code (e.g. interposed) should be left to upper layers e.g. libc. But this is too much for this stage, so we moved some interpositions into libsys. The sys_XXX symbol movement reflects that, and since both libsys and sys_XXX are implementation details, it is fine. After we fix and harden the current libsys extraction step, we could re-visit that. We must do something before we claim libsys is part of the ABI.

Additional argument is that all the 'syscalls' like open/wait/etc are no longer syscalls, they forward the call to more generic openat/waitid.

I'm going to go the route of pulling these symbols back into libc.

brooks mentioned this in D44241: lib{c,sys}: return wrapped syscall APIs to libc.Mar 5 2024, 9:51 PM

brooks mentioned this in rGd7847a8d3514: lib{c,sys}: return wrapped syscall APIs to libc.Mar 13 2024, 6:37 PM