Re: [Qemu-devel] [PATCH 05/10 v4] bsd-user: Implement new syscall print_sysarch and add strace support

[Sean Bruno]

On Tue, 2014-06-10 at 23:53 +0100, Peter Maydell wrote:
> On 8 June 2014 17:57, Sean Bruno <address@hidden> wrote:
> > Signed-off-by: Sean Bruno <address@hidden>
> > ---
> >  bsd-user/freebsd/os-strace.h           | 29 +++++++++++++
> >  bsd-user/freebsd/strace.list           |  2 +-
> >  bsd-user/i386/syscall.h                | 21 +++++++++
> >  bsd-user/i386/target_arch_sysarch.h    | 78 
> > ++++++++++++++++++++++++++++++++++
> >  bsd-user/netbsd/os-strace.h            |  1 +
> >  bsd-user/openbsd/os-strace.h           |  1 +
> >  bsd-user/sparc/syscall.h               | 27 +++++++++++-
> >  bsd-user/sparc/target_arch_sysarch.h   | 52 +++++++++++++++++++++++
> >  bsd-user/sparc64/syscall.h             | 26 +++++++++++-
> >  bsd-user/sparc64/target_arch_sysarch.h | 52 +++++++++++++++++++++++
> >  bsd-user/strace.c                      | 10 +++++
> >  bsd-user/x86_64/syscall.h              | 24 ++++++++++-
> >  bsd-user/x86_64/target_arch_sysarch.h  | 76 
> > +++++++++++++++++++++++++++++++++
> >  13 files changed, 395 insertions(+), 4 deletions(-)
> >  create mode 100644 bsd-user/freebsd/os-strace.h
> >  create mode 100644 bsd-user/i386/target_arch_sysarch.h
> >  create mode 100644 bsd-user/netbsd/os-strace.h
> >  create mode 100644 bsd-user/openbsd/os-strace.h
> >  create mode 100644 bsd-user/sparc/target_arch_sysarch.h
> >  create mode 100644 bsd-user/sparc64/target_arch_sysarch.h
> >  create mode 100644 bsd-user/x86_64/target_arch_sysarch.h
> 
> Unfortunately this breaks build of bsd-user on OpenBSD
> and NetBSD, because they don't provide a do_os_print_sysarch().
> 

Right, I've stubbed out a no-op function and tested on open/netbsd.
I'll send an update today.

<more comments below>

> > --- /dev/null
> > +++ b/bsd-user/i386/target_arch_sysarch.h
> > @@ -0,0 +1,78 @@
> > +/*
> > + *  i386 sysarch system call emulation
> > + *
> > + *  Copyright (c) 2013 Stacey D. Son
> > + *
> > + *  This program is free software; you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation; either version 2 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef __ARCH_SYSARCH_H_
> > +#define __ARCH_SYSARCH_H_
> > +
> > +#include "syscall.h"
> > +
> > +static inline abi_long do_freebsd_arch_sysarch(CPUX86State *env, int op,
> > +        abi_ulong parms)
> > +{
> > +    abi_long ret = 0;
> > +    abi_ulong val;
> > +    int idx;
> > +
> > +    switch (op) {
> > +    case TARGET_FREEBSD_I386_SET_GSBASE:
> > +    case TARGET_FREEBSD_I386_SET_FSBASE:
> 
> Something's wrong here too -- this patch adds these functions
> for each architecture, but it doesn't add the code that calls them,
> and it doesn't delete the copies of this code from syscall.c.
> 
> thanks
> -- PMM
> 


Digging through this for the last couple of days.  Sorry for the slow
response, lots of code flow to follow here.

I think there's some confusion between the strace support for the new
print_sysarch() and the existing syscall do_freebsd_sysarch().  

If I follow the code, the existing do_freebsd_sysarch() syscall is a
programtical way of figuring out what arch is running.  Whereas
print_sysarch() spams the arch into your strace output.

Bearing that in mind, I think that the changes here are indeed correct
for this patchset.