Re: [RFC 4/4] common-user: Allow return codes to be adjusted after sytsem call

[Warner Losh <imp@bsdimp.com>]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

On Mon, Nov 8, 2021 at 8:10 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 11/8/21 3:37 AM, Warner Losh wrote:
> > All the *-users generally use the Linux style of negative return codes
> > for errno. However, other systems, like FreeBSD, have a different
> > convention. Allow those systems to insert code after the syscall that
> > adjusts the return value of the system call to match the native linux
> > format.
> >
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> >   common-user/host/aarch64/safe-syscall.inc.S | 1 +
> >   common-user/host/arm/safe-syscall.inc.S     | 1 +
> >   common-user/host/i386/safe-syscall.inc.S    | 1 +
> >   common-user/host/ppc64/safe-syscall.inc.S   | 1 +
> >   common-user/host/riscv/safe-syscall.inc.S   | 1 +
> >   common-user/host/s390x/safe-syscall.inc.S   | 1 +
> >   common-user/host/x86_64/safe-syscall.inc.S  | 1 +
> >   linux-user/safe-syscall.S                   | 1 +
> >   8 files changed, 8 insertions(+)
> >
> > diff --git a/common-user/host/aarch64/safe-syscall.inc.S
> b/common-user/host/aarch64/safe-syscall.inc.S
> > index bc1f5a9792..81d83e8e79 100644
> > --- a/common-user/host/aarch64/safe-syscall.inc.S
> > +++ b/common-user/host/aarch64/safe-syscall.inc.S
> > @@ -64,6 +64,7 @@ safe_syscall_start:
> >       svc     0x0
> >   safe_syscall_end:
> >       /* code path for having successfully executed the syscall */
> > +     ADJUST_SYSCALL_RETCODE
> >       ret
> >
> >   0:
>
> Not sure about this, really.  Is it really that much cleaner to insert
> this than create
> separate 10-line files, with the adjustment included?
>

While the meat of these files is only around 10 lines, the files themselves
are more like 70 lines with a lot of extra marking to ensure proper
integration with toolchains. Such lines have a tendency, over time, to need
adjusting as new toolchains change the requirements slightly (clang
certainly has forced that in a number of places in FreeBSD's code base, and
every new version seems to require something). The adjustments have all
been 3 lines (gmail seems to hate my formatting):

+#define        ADJUST_SYSCALL_RETCODE \
+    jnb 2f;                    \
+    neg %rax;                  \
+    2:

which is significantly easier to maintain than having to monitor these
files for changes and copying over the changes that happen. Plus, as I'm
upstreaming the arch support, it's one more file I have to include in the
review, it's one more file that may get questions and advice I'll have to
answer with 'it's a verbatim copy of the linux version with these three
lines added, why do I need to make those stylistic changes'. All of which
takes extra time. The upstreaming is going much more slowly than I'd
anticipated (but on the up-side also finding more problems than I thought
were latent in the system), and we're still at about 30k lines (after
starting at about 36k lines, though some of that is due to deleting mips).
It's been running about a month per 1000 lines to get reviewed and
upstreamed. There's about 626 lines in these 6 files, so sharing them
seemed like it would save me a few weeks of upstreaming time as well
(though I'll be the first to admit that translating LoC metrics to time has
a dubious history).

The other alternative I considered was having a #ifdef __FreeBSD__ ..
#endif in all those files, but I thought that even more intrusive.

Warner

[-- Attachment #2: Type: text/html, Size: 4225 bytes --]