PERFORCE change 54797 for review

Marcel Moolenaar marcel at FreeBSD.org
Sun Jun 13 07:32:51 GMT 2004


http://perforce.freebsd.org/chv.cgi?CH=54797

Change 54797 by marcel at 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)
 /*


More information about the p4-projects mailing list