Re: [PATCH 16/30] bsd-user/signal.c: host_to_target_siginfo_noswap

[Warner Losh <imp@bsdimp.com>]

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

On Thu, Jan 13, 2022 at 12:43 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Sun, 9 Jan 2022 at 16:41, Warner Losh <imp@bsdimp.com> wrote:
> >
> > Implement conversion of host to target siginfo.
> >
> > Signed-off-by: Stacey Son <sson@FreeBSD.org>
> > Signed-off-by: Kyle Evans <kevans@freebsd.org>
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> >  bsd-user/signal.c | 37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >
> > diff --git a/bsd-user/signal.c b/bsd-user/signal.c
> > index 7168d851be8..3fe8b2d9898 100644
> > --- a/bsd-user/signal.c
> > +++ b/bsd-user/signal.c
> > @@ -43,6 +43,43 @@ int target_to_host_signal(int sig)
> >      return sig;
> >  }
> >
> > +/* Siginfo conversion. */
> > +static inline void host_to_target_siginfo_noswap(target_siginfo_t
> *tinfo,
> > +        const siginfo_t *info)
> > +{
> > +    int sig, code;
> > +
> > +    sig = host_to_target_signal(info->si_signo);
> > +    /* XXX should have host_to_target_si_code() */
> > +    code = tswap32(info->si_code);
> > +    tinfo->si_signo = sig;
> > +    tinfo->si_errno = info->si_errno;
> > +    tinfo->si_code = info->si_code;
> > +    tinfo->si_pid = info->si_pid;
> > +    tinfo->si_uid = info->si_uid;
> > +    tinfo->si_status = info->si_status;
> > +    tinfo->si_addr = (abi_ulong)(unsigned long)info->si_addr;
> > +    /* si_value is opaque to kernel */
> > +    tinfo->si_value.sival_ptr =
> > +        (abi_ulong)(unsigned long)info->si_value.sival_ptr;
> > +    if (SIGILL == sig || SIGFPE == sig || SIGSEGV == sig || SIGBUS ==
> sig ||
>
> Don't use yoda-conditions, please. sig == SIGILL, etc.
>
> > +            SIGTRAP == sig) {
> > +        tinfo->_reason._fault._trapno = info->_reason._fault._trapno;
> > +    }
> > +#ifdef SIGPOLL
> > +    if (SIGPOLL == sig) {
> > +        tinfo->_reason._poll._band = info->_reason._poll._band;
> > +    }
> > +#endif
> > +    if (SI_TIMER == code) {
> > +        int timerid;
> > +
> > +        timerid = info->_reason._timer._timerid;
> > +        tinfo->_reason._timer._timerid = timerid;
> > +        tinfo->_reason._timer._overrun = info->_reason._timer._overrun;
> > +    }
> > +}
>
> I think this will only compile on FreeBSD (the other BSDs having
> notably different target_siginfo_t structs); I guess we're OK
> with that ?
>

Yes. bsd-user fork does not compile on the other BSDs. There's too many
things missing, and too few places where specific code is in place. I'm
thinking
that it won't be possible to implement running NetBSD binaries on FreeBSD
or vice versa since they are so different. OpenBSD and NetBSD are a lot
closer to each other, with fewer critical differences in areas like
threads, so
it may be possible there. There's a lot of rework that's needed in this area
to take what's even in bsd-user fork and make it work on NetBSD or
OpenBSD, exactly for reasons like this. I've been ignoring the elephant
in the room for a while now, ever since I realized this fundamental
shift.


> I also commented on the general setup linux-user has for this
> function back in patch 2; I'll let you figure out whether what
> you have here is the right thing for BSD.
>

Yea. I'm still thinking through what you said there (and elsewhere). These
issues may be the root cause of some regressions in arm binaries between
6.1 and 6.2 as I tried to adopt the sigsegv/sigbus changes.

I need to work through those things in our development branch before trying
to fold them into this series. And I'm not yet sure the right way to do
that because
many of the things are likely to be largish changes that may be tough to
manage
keeping this patch series in sync. So I'm going to do all the trivial style
and
tiny bug things first, then tackle this more fundamental issue. I've thought
about it enough to understand that the code in this patch series has some
conceptual mistakes that must be addressed. Having this very detailed
feedback
is quite helpful in laying out the path for me to fix these issues (even if
I don't
ultimately do everything like linux-user, I'll know why it's different
rather than
the current situation where there's much inherited code and the best answer
I could give is 'well linux-user was like that 5 years ago and we needed to
make
these hacks to make things work' which is completely unsatisfying to give
and
to hear.

Warner

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