Date: Sun, 13 Jun 2004 07:30:41 GMT From: Marcel Moolenaar <marcel@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 54797 for review Message-ID: <200406130730.i5D7Uf5i054078@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=54797 Change 54797 by marcel@marcel_nfs on 2004/06/13 07:29:43 Axe ddb_regs. Static storage for thread context in a multi- threaded environment is not at all ideal. It's not needed at all in this case (however, software single stepping has still references to DDB_REGS). We can safely dereference kdb_thrctx. Note that the registers are still taken from the trapframe. Also note that curthread (i.e. the thread that entered the debugger) has a backtrace that starts from kdb_trap(). This needs to be changed, because otherwise single stepping is weird (you need to do a backtrace to see beyond the frame). Hence, it's better to create the context from the frame. Anyway: DDB on i386 works. Backtraces of all threads are all good! I'll check GDB tomorrow morning and worl on the other platforms after that. Affected files ... .. //depot/projects/gdb/sys/ddb/db_main.c#8 edit .. //depot/projects/gdb/sys/ddb/db_print.c#2 edit .. //depot/projects/gdb/sys/ddb/db_run.c#2 edit .. //depot/projects/gdb/sys/ddb/db_thread.c#6 edit .. //depot/projects/gdb/sys/i386/i386/db_trace.c#8 edit .. //depot/projects/gdb/sys/i386/include/db_machdep.h#2 edit Differences ... ==== //depot/projects/gdb/sys/ddb/db_main.c#8 (text+ko) ==== @@ -38,6 +38,7 @@ #include <sys/reboot.h> #include <machine/kdb.h> +#include <machine/pcb.h> #include <machine/setjmp.h> #include <ddb/ddb.h> @@ -49,7 +50,6 @@ KDB_BACKEND(ddb, db_init, db_trace_self, db_trap); -db_regs_t ddb_regs; vm_offset_t ksym_start, ksym_end; boolean_t @@ -196,8 +196,6 @@ if (cnunavailable()) return (0); - ddb_regs = *kdb_frame; - bkpt = IS_BREAKPOINT_TRAP(type, code); watchpt = IS_WATCHPOINT_TRAP(type, code); @@ -208,7 +206,7 @@ } prev_jb = kdb_jmpbuf(jb); if (setjmp(jb) == 0) { - db_dot = PC_REGS(DDB_REGS); + db_dot = PC_REGS(); db_print_thread(); if (bkpt) db_printf("Breakpoint at\t"); @@ -224,7 +222,5 @@ db_restart_at_pc(watchpt); - *kdb_frame = ddb_regs; - return (1); } ==== //depot/projects/gdb/sys/ddb/db_print.c#2 (text+ko) ==== @@ -37,6 +37,9 @@ __FBSDID("$FreeBSD: src/sys/ddb/db_print.c,v 1.27 2003/06/10 22:09:23 obrien Exp $"); #include <sys/param.h> +#include <sys/kdb.h> + +#include <machine/pcb.h> #include <ddb/ddb.h> #include <ddb/db_variables.h> @@ -65,5 +68,5 @@ } db_printf("\n"); } - db_print_loc_and_inst(PC_REGS(DDB_REGS)); + db_print_loc_and_inst(PC_REGS()); } ==== //depot/projects/gdb/sys/ddb/db_run.c#2 (text+ko) ==== @@ -36,6 +36,10 @@ __FBSDID("$FreeBSD: src/sys/ddb/db_run.c,v 1.23 2003/06/10 22:09:23 obrien Exp $"); #include <sys/param.h> +#include <sys/kdb.h> + +#include <machine/kdb.h> +#include <machine/pcb.h> #include <vm/vm.h> @@ -67,10 +71,6 @@ extern void db_clear_single_step(db_regs_t *regs); #endif -#ifdef notused -static void db_single_step(db_regs_t *regs); -#endif - boolean_t db_stop_at_pc(is_breakpoint) boolean_t *is_breakpoint; @@ -78,10 +78,10 @@ register db_addr_t pc; register db_breakpoint_t bkpt; - db_clear_single_step(DDB_REGS); + db_clear_single_step(); db_clear_breakpoints(); db_clear_watchpoints(); - pc = PC_REGS(DDB_REGS); + pc = PC_REGS(); #ifdef FIXUP_PC_AFTER_BREAK if (*is_breakpoint) { @@ -90,7 +90,7 @@ * machine requires it. */ FIXUP_PC_AFTER_BREAK - pc = PC_REGS(DDB_REGS); + pc = PC_REGS(); } #endif @@ -171,7 +171,7 @@ db_restart_at_pc(watchpt) boolean_t watchpt; { - register db_addr_t pc = PC_REGS(DDB_REGS); + register db_addr_t pc = PC_REGS(); if ((db_run_mode == STEP_COUNT) || (db_run_mode == STEP_RETURN) || @@ -205,27 +205,15 @@ * Step over breakpoint/watchpoint. */ db_run_mode = STEP_INVISIBLE; - db_set_single_step(DDB_REGS); + db_set_single_step(); } else { db_set_breakpoints(); db_set_watchpoints(); } } else { - db_set_single_step(DDB_REGS); - } -} - -#ifdef notused -static void -db_single_step(regs) - db_regs_t *regs; -{ - if (db_run_mode == STEP_CONTINUE) { - db_run_mode = STEP_INVISIBLE; - db_set_single_step(regs); + db_set_single_step(); } } -#endif #ifdef SOFTWARE_SSTEP /* ==== //depot/projects/gdb/sys/ddb/db_thread.c#6 (text+ko) ==== @@ -32,6 +32,8 @@ #include <sys/kdb.h> #include <sys/proc.h> +#include <machine/pcb.h> + #include <ddb/ddb.h> #include <ddb/db_command.h> #include <ddb/db_sym.h> @@ -61,16 +63,13 @@ if (hastid) { thr = kdb_thr_lookup(tid); if (thr != NULL) { - *kdb_frame = ddb_regs; err = kdb_thr_select(thr); - if (err == 0) { - ddb_regs = *kdb_frame; - db_dot = PC_REGS(DDB_REGS); - } else { + if (err != 0) { db_printf("unable to switch to thread %d\n", (int)thr->td_tid); return; } + db_dot = PC_REGS(); } else { db_printf("%d: invalid thread\n", (int)tid); return; @@ -78,7 +77,7 @@ } db_print_thread(); - db_print_loc_and_inst(PC_REGS(DDB_REGS)); + db_print_loc_and_inst(PC_REGS()); } void ==== //depot/projects/gdb/sys/i386/i386/db_trace.c#8 (text+ko) ==== @@ -55,46 +55,38 @@ static db_varfcn_t db_dr5; static db_varfcn_t db_dr6; static db_varfcn_t db_dr7; +static db_varfcn_t db_esp; +static db_varfcn_t db_frame; static db_varfcn_t db_ss; -static db_varfcn_t db_esp; - -static __inline int -get_esp(struct trapframe *tf) -{ - return ((ISPL(tf->tf_cs)) ? tf->tf_esp : - (db_expr_t)tf + offsetof(struct trapframe, tf_esp)); -} /* * Machine register set. */ +#define DB_OFFSET(x) (db_expr_t *)offsetof(struct trapframe, x) struct db_variable db_regs[] = { - { "cs", &ddb_regs.tf_cs, FCN_NULL }, - { "ds", &ddb_regs.tf_ds, FCN_NULL }, - { "es", &ddb_regs.tf_es, FCN_NULL }, - { "fs", &ddb_regs.tf_fs, FCN_NULL }, -#if 0 - { "gs", &ddb_regs.tf_gs, FCN_NULL }, -#endif - { "ss", NULL, db_ss }, - { "eax", &ddb_regs.tf_eax, FCN_NULL }, - { "ecx", &ddb_regs.tf_ecx, FCN_NULL }, - { "edx", &ddb_regs.tf_edx, FCN_NULL }, - { "ebx", &ddb_regs.tf_ebx, FCN_NULL }, - { "esp", NULL, db_esp }, - { "ebp", &ddb_regs.tf_ebp, FCN_NULL }, - { "esi", &ddb_regs.tf_esi, FCN_NULL }, - { "edi", &ddb_regs.tf_edi, FCN_NULL }, - { "eip", &ddb_regs.tf_eip, FCN_NULL }, - { "efl", &ddb_regs.tf_eflags, FCN_NULL }, - { "dr0", NULL, db_dr0 }, - { "dr1", NULL, db_dr1 }, - { "dr2", NULL, db_dr2 }, - { "dr3", NULL, db_dr3 }, - { "dr4", NULL, db_dr4 }, - { "dr5", NULL, db_dr5 }, - { "dr6", NULL, db_dr6 }, - { "dr7", NULL, db_dr7 }, + { "cs", DB_OFFSET(tf_cs), db_frame }, + { "ds", DB_OFFSET(tf_ds), db_frame }, + { "es", DB_OFFSET(tf_es), db_frame }, + { "fs", DB_OFFSET(tf_fs), db_frame }, + { "ss", NULL, db_ss }, + { "eax", DB_OFFSET(tf_eax), db_frame }, + { "ecx", DB_OFFSET(tf_ecx), db_frame }, + { "edx", DB_OFFSET(tf_edx), db_frame }, + { "ebx", DB_OFFSET(tf_ebx), db_frame }, + { "esp", NULL, db_esp }, + { "ebp", DB_OFFSET(tf_ebp), db_frame }, + { "esi", DB_OFFSET(tf_esi), db_frame }, + { "edi", DB_OFFSET(tf_edi), db_frame }, + { "eip", DB_OFFSET(tf_eip), db_frame }, + { "efl", DB_OFFSET(tf_eflags), db_frame }, + { "dr0", NULL, db_dr0 }, + { "dr1", NULL, db_dr1 }, + { "dr2", NULL, db_dr2 }, + { "dr3", NULL, db_dr3 }, + { "dr4", NULL, db_dr4 }, + { "dr5", NULL, db_dr5 }, + { "dr6", NULL, db_dr6 }, + { "dr7", NULL, db_dr7 }, }; struct db_variable *db_eregs = db_regs + sizeof(db_regs)/sizeof(db_regs[0]); @@ -121,23 +113,43 @@ DB_DRX_FUNC(dr6) DB_DRX_FUNC(dr7) +static __inline int +get_esp(struct trapframe *tf) +{ + return ((ISPL(tf->tf_cs)) ? tf->tf_esp : + (db_expr_t)tf + (uintptr_t)DB_OFFSET(tf_esp)); +} + static int -db_esp (struct db_variable *vp, db_expr_t *valuep, int op) +db_frame(struct db_variable *vp, db_expr_t *valuep, int op) +{ + int *reg; + + reg = (int *)((uintptr_t)kdb_frame + (db_expr_t)vp->valuep); + if (op == DB_VAR_GET) + *valuep = *reg; + else + *reg = *valuep; + return (0); +} + +static int +db_esp(struct db_variable *vp, db_expr_t *valuep, int op) { if (op == DB_VAR_GET) *valuep = get_esp(kdb_frame); - else if (ISPL(ddb_regs.tf_cs)) - ddb_regs.tf_esp = *valuep; + else if (ISPL(kdb_frame->tf_cs)) + kdb_frame->tf_esp = *valuep; return (0); } static int -db_ss (struct db_variable *vp, db_expr_t *valuep, int op) +db_ss(struct db_variable *vp, db_expr_t *valuep, int op) { if (op == DB_VAR_GET) - *valuep = (ISPL(ddb_regs.tf_cs)) ? ddb_regs.tf_ss : rss(); - else if (ISPL(ddb_regs.tf_cs)) - ddb_regs.tf_ss = *valuep; + *valuep = (ISPL(kdb_frame->tf_cs)) ? kdb_frame->tf_ss : rss(); + else if (ISPL(kdb_frame->tf_cs)) + kdb_frame->tf_ss = *valuep; return (0); } ==== //depot/projects/gdb/sys/i386/include/db_machdep.h#2 (text+ko) ==== @@ -30,30 +30,23 @@ #define _MACHINE_DB_MACHDEP_H_ #include <machine/frame.h> -#include <machine/psl.h> #include <machine/trap.h> -#define i386_saved_state trapframe - typedef vm_offset_t db_addr_t; /* address - unsigned */ typedef int db_expr_t; /* expression - signed */ -typedef struct i386_saved_state db_regs_t; -extern db_regs_t ddb_regs; /* register state */ -#define DDB_REGS (&ddb_regs) - -#define PC_REGS(regs) ((db_addr_t)(regs)->tf_eip) +#define PC_REGS() ((db_addr_t)kdb_thrctx->pcb_eip) #define BKPT_INST 0xcc /* breakpoint instruction */ #define BKPT_SIZE (1) /* size of breakpoint inst */ #define BKPT_SET(inst) (BKPT_INST) -#define BKPT_SKIP ddb_regs.tf_eip += 1 +#define BKPT_SKIP kdb_frame->tf_eip += 1 -#define FIXUP_PC_AFTER_BREAK ddb_regs.tf_eip -= 1; +#define FIXUP_PC_AFTER_BREAK kdb_frame->tf_eip -= 1; -#define db_clear_single_step(regs) ((regs)->tf_eflags &= ~PSL_T) -#define db_set_single_step(regs) ((regs)->tf_eflags |= PSL_T) +#define db_clear_single_step kdb_cpu_clear_singlestep +#define db_set_single_step kdb_cpu_set_singlestep #define IS_BREAKPOINT_TRAP(type, code) ((type) == T_BPTFLT) /*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200406130730.i5D7Uf5i054078>