> Date: Tue, 21 Mar 2017 15:57:46 +0100 (CET)
> From: Mark Kettenis <mark.kette...@xs4all.nl>
> 
> > Date: Mon, 20 Mar 2017 12:26:28 -0400
> > From: Dale Rahn <dr...@dalerahn.com>
> > 
> > On Mon, Mar 20, 2017 at 11:54:03AM +0100, Mark Kettenis wrote:
> > > > Date: Mon, 20 Mar 2017 13:31:32 +1100
> > > > From: Jonathan Gray <j...@jsg.id.au>
> > > > Cc: tech@openbsd.org
> > > > Mail-Followup-To: Mark Kettenis <mark.kette...@xs4all.nl>, 
> > > > tech@openbsd.org
> > > > Content-Disposition: inline
> > > > X-XS4ALL-DNSBL-Checked: mxdrop306.xs4all.net checked 210.15.216.215 
> > > > against DNS blacklists
> > > > X-CNFS-Analysis: v=2.2 cv=eoad9chX c=1 sm=0 tr=0
> > > >         a=0rbIscUo4apI/L6UuJIdkA==:117 a=0rbIscUo4apI/L6UuJIdkA==:17
> > > >         a=kj9zAlcOel0A:10 a=6Iz7jQTuP9IA:10 a=d_dndPjGE7KARIjjoGsA:9
> > > >         a=CjuIK1q_8ugA:10
> > > > X-Virus-Scanned: by XS4ALL Virus Scanner
> > > > X-XS4ALL-Spam-Score: -0.5 () RP_MATCHES_RCVD, UNPARSEABLE_RELAY
> > > > X-XS4ALL-Spam: NO
> > > > Envelope-To: mark.kette...@xs4all.nl
> > > > 
> > > > On Mon, Mar 20, 2017 at 01:35:04AM +0100, Mark Kettenis wrote:
> > > > > It turns out that pretty much all relevant aarch64 OSes use the same
> > > > > layout for transferring registers in their debug interfaces.  Except
> > > > > for us.  That doesn't make sense and would mean I'd have to do
> > > > > additional work in my lldb porting efforts.
> > > > > 
> > > > > Diff below revises "struct reg" for amd64 to be compatible with what
> > > > > NetBSD provides.  That just matches our naming conventions for struct
> > > > > reg better.  The actual names are largely irrelevant as debuggers
> > > > > hardcode the layouts anyway to support cross-debugging.
> > > > > 
> > > > > This struct isn't actually used yet, so these changes don't really
> > > > > have any ABI consequences.  But once this is in, I'm planning on
> > > > > making core dumps and ptrace actually work on arm64.
> > > > > 
> > > > > ok?
> > > > 
> > > > It looks like NetBSD just has a cross compiled toolchain
> > > > nothing that runs on actual hardware.
> > > > 
> > > > Given FreeBSD does I'd consider that more relevant, and it
> > > > has:
> > > > 
> > > > struct reg {
> > > >         uint64_t x[30];
> > > >         uint64_t lr;
> > > >         uint64_t sp;
> > > >         uint64_t elr;
> > > >         uint32_t spsr;
> > > > };
> > > > 
> > > > struct fpreg {
> > > >         __uint128_t     fp_q[32];
> > > >         uint32_t        fp_sr;
> > > >         uint32_t        fp_cr;
> > > > };
> > > 
> > > The FreeBSD definition is equivalent to the NetBSD one:
> > > 
> > > lr  == x30
> > > elr == pc
> > > 
> > > It's just that the names assigned by NetBSD make a little bit more
> > > sense and follow the pattern we use on many other architectures.
> > > 
> > > NetBSD also includes the userland per-thread-register which I think is
> > > a good idea.
> > > 
> > 
> > Naming and order realistically doesn't matter that much, If it is easier to
> > move registers around to match a userland structure in process_*regs(),
> > that is reasonable. That said: the more I have followed NetBSD's lead in the
> > past, the more I have regretted it later.
> 
> Here is an update diff that actually implementes process_read_regs().
> Changed the struct reg definition slightly to make r_rl explicit to
> match our struct trapframe a bit closer.  That makes the
> process_read_regs() implementation more obvious.
> 
> This is enough to make core dumps useful, so I'd like to commit this
> partial implementation.  Did explicitly zero out the struct fpreg in
> process_read_fpregs() to avoid leaking kernel stack contents.
> 
> > using __uint128_t for FPU is a lot better than the uint64 [64] that is there
> > now.
> 
> I'd like to address that in a future diff.
> 
> ok?
> 
> Index: arm64/process_machdep.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/arm64/process_machdep.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 process_machdep.c
> --- arm64/process_machdep.c   17 Dec 2016 23:38:33 -0000      1.1
> +++ arm64/process_machdep.c   21 Mar 2017 14:52:35 -0000
> @@ -53,12 +53,22 @@
>  int
>  process_read_regs(struct proc *p, struct reg *regs)
>  {
> +     struct trapframe *tf = p->p_addr->u_pcb.pcb_tf;
> +
> +     memcpy(&regs->r_reg[0], &tf->tf_x[0], sizeof(regs->r_reg));
> +     regs->r_lr = tf->tf_lr;
> +     regs->r_sp = tf->tf_sp;
> +     regs->r_pc = tf->tf_elr;
> +     regs->r_spsr = tf->tf_spsr;
> +     regs->r_tpidr = (uint64_t)p->p_addr->u_pcb.pcb_tcb;
> +
>       return(0);
>  }
>  
>  int
>  process_read_fpregs(struct proc *p, struct fpreg *regs)
>  {
> +     memset(&regs, 0, sizeof(*regs));

That should be:

  memset(regs, 0, sizeof(*regs));

Thanks to Peter J. Philipp for pointing that out.

Reply via email to