re: diff: ddb: gather common code for x86

[matthew green <mrg%eterna.com.au@localhost>]

> Here's the diff for ddb(4):
>       - reuse the common code for stack traces
>               (and rely on sizeof(long) in most cases)
>       - use db_read_* api to be usable from crash(8).

great.  i love re-sharing code.  i just have a few comments.

thanks for doing this, and for adm64 crash.


.mrg.


> Index: x86/include/db_machdep.h
> ===================================================================
> RCS file: x86/include/db_machdep.h
> diff -N x86/include/db_machdep.h
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ x86/include/db_machdep.h  6 Apr 2011 11:24:14 -0000
> @@ -0,0 +1,24 @@
> +#ifndef _X86_DB_MACHDEP_H_
> +#define _X86_DB_MACHDEP_H_
[ ... ]

this file is missing it's originator copyright header, i guess?

> +#endif /* _X86_DB_MACHDEP_H_ */
> Index: x86/x86/db_trace.c
> ===================================================================
> RCS file: x86/x86/db_trace.c
> diff -N x86/x86/db_trace.c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ x86/x86/db_trace.c        6 Apr 2011 11:24:14 -0000
> @@ -0,0 +1,323 @@
> +/*   $NetBSD: db_trace.c,v 1.18 2010/07/01 02:38:27 rmind Exp $      */
[ ... ]
> +
> +#ifdef __x86_64__
> +#define set_frame_callpc() do {                              \
> +             frame = (long *)ddb_regs.tf_rbp;        \
> +             callpc = (db_addr_t)ddb_regs.tf_rip;    \
> +     } while (0)
> +#else
> +#define set_frame_callpc() do {                              \
> +             frame = (long *)ddb_regs.tf_ebp;        \
> +             callpc = (db_addr_t)ddb_regs.tf_eip;    \
> +     } while (0)
> +#endif

it would be nice to abstract out these tf_rip and tf_eip (and the ones
that also look in rbp/ebp, etc.) something like.

#ifdef __x86_64__
#define tf_ip_reg tf_rip
#define tf_bp_reg tf_rbp
#else
#define tf_ip_reg tf_eip
#define tf_bp_reg tf_ebp
#endif

> +#ifdef __x86_64__
> +                     argp = &((struct x86_64_frame 
> *)(ddb_regs.tf_rsp-sizeof(long)))->f_arg0;
> +#else
> +                     argp = (long *)&((struct i386_frame 
> *)(ddb_regs.tf_esp-sizeof(long)))->f_arg0;
> +#endif

i think this one could be the same, using "x86_frame"

> +
> +             if (lastframe == 0 && offset == 0 && !have_addr) {
> +                     /* Frame really belongs to next callpc */
> +#ifdef __x86_64__
> +                     struct x86_64_frame *fp = (void 
> *)(ddb_regs.tf_rsp-sizeof(long));
> +#else
> +                     struct i386_frame *fp = (void 
> *)(ddb_regs.tf_esp-sizeof(long));
> +#endif

and again.

> Index: i386/i386/db_machdep.c
> ===================================================================
> RCS file: i386/i386/db_machdep.c
> diff -N i386/i386/db_machdep.c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ i386/i386/db_machdep.c    6 Apr 2011 11:24:14 -0000
> @@ -0,0 +1,299 @@
> +/*   $NetBSD$        */
> +
[ ... ]

this one also needs a copyright.

> +#ifdef __ELF__
> +             if (!strcmp(name, "trap_tss")) {
[ ... ]
> +             }
> +#else
> +             if (!strcmp(name, "_trap_tss")) {
[ ... ]
> +             }
> +#endif /* __ELF__ */

don't bother with this part for now but we should delete the !elf case.

> Index: i386/i386/db_trace.c

this file has #ifdef __x86_64__ in it.  is that right?

> @@ -427,8 +85,11 @@ db_stack_trace_print(db_expr_t addr, boo
>       }
>  
>       if (!have_addr) {
> +#ifdef __x86_64__
>               frame = (int *)ddb_regs.tf_ebp;
>               callpc = (db_addr_t)ddb_regs.tf_eip;
> +#else
> +#endif
>       } else {
>               if (trace_thread) {
>                       struct pcb *pcb;

> Index: amd64/amd64/db_machdep.c

copyright needed here too.

> Index: amd64/conf/GENERIC
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/amd64/conf/GENERIC,v
> retrieving revision 1.318
> diff -u -p -r1.318 GENERIC
> --- amd64/conf/GENERIC        1 Apr 2011 12:11:17 -0000       1.318
> +++ amd64/conf/GENERIC        6 Apr 2011 11:24:14 -0000
[ ... ]
> +options              DDB_TEE_MSGBUF=1
[ ... ]
> -ath* at pci? dev ? function ?        # Atheros 5210/5211/5212 802.11
> +#ath*        at pci? dev ? function ?        # Atheros 5210/5211/5212 802.11
[ ... ]
> -ath* at cardbus? function ?  # Atheros 5210/5211/5212 802.11
> +#ath*        at cardbus? function ?  # Atheros 5210/5211/5212 802.11

i guess you didn't mean to include this part?