svn commit: r290730 - in stable: 10/sys/amd64/amd64 10/sys/i386/i386 9/sys/amd64/amd64 9/sys/i386/i386

John Baldwin jhb at FreeBSD.org
Thu Nov 12 22:45:53 UTC 2015


Author: jhb
Date: Thu Nov 12 22:45:51 2015
New Revision: 290730
URL: https://svnweb.freebsd.org/changeset/base/290730

Log:
  MFC 285773,285775,285776:
  Various fixes for stack unwinding in DDB on x86.
  
  285773:
  Remove some dead code from DDB's amd64 stack unwinder.
  
  The amd64 port copied some code from i386 to fetch function arguments and
  display them in backtraces. However, it was commented out and can't easily
  be implemented since the function arguments are passed in
  registers rather than on the stack in amd64. Remove it in preparation for
  some bug fixes in this area.
  
  285775:
  Improve stack unwinding on i386 and amd64 after an IP fault.
  
  If we can't find a symbol corresponding to the faulting instruction, assume
  that the previously-executed function is a call and attempt to find the
  calling function using the return address on the stack. Otherwise we end
  up associating the last stack frame with the current call, which is
  incorrect and causes the unwinder to skip printing of the calling function,
  resulting in a confusing backtrace.
  
  285776:
  Let the unwinder handle faults during function prologues or epilogues.
  
  The i386 and amd64 DDB stack unwinders contain code to detect and handle
  the case where the first frame is not completely set up or torn down. This
  code was accidentally unused however, since db_backtrace() was never called
  with a non-NULL trap frame. This change fixes that.
  
  Also remove get_rsp() from the amd64 code. It appears to have come from
  i386, which needs to take into account whether the exception triggered a
  CPL switch, since SS:ESP is only pushed onto the stack if so. On amd64,
  SS:RSP is pushed regardless, so get_rsp() was doing the wrong thing for
  kernel-mode exceptions. As a result, we can also remove custom print
  functions for these registers.

Modified:
  stable/9/sys/amd64/amd64/db_trace.c
  stable/9/sys/i386/i386/db_trace.c
Directory Properties:
  stable/9/sys/   (props changed)

Changes in other areas also in this revision:
Modified:
  stable/10/sys/amd64/amd64/db_trace.c
  stable/10/sys/i386/i386/db_trace.c
Directory Properties:
  stable/10/   (props changed)

Modified: stable/9/sys/amd64/amd64/db_trace.c
==============================================================================
--- stable/9/sys/amd64/amd64/db_trace.c	Thu Nov 12 22:24:39 2015	(r290729)
+++ stable/9/sys/amd64/amd64/db_trace.c	Thu Nov 12 22:45:51 2015	(r290730)
@@ -61,8 +61,6 @@ static db_varfcn_t db_dr5;
 static db_varfcn_t db_dr6;
 static db_varfcn_t db_dr7;
 static db_varfcn_t db_frame;
-static db_varfcn_t db_rsp;
-static db_varfcn_t db_ss;
 
 CTASSERT(sizeof(struct dbreg) == sizeof(((struct pcpu *)NULL)->pc_dbreg));
 
@@ -76,12 +74,12 @@ struct db_variable db_regs[] = {
 	{ "es",		DB_OFFSET(tf_es),	db_frame },
 	{ "fs",		DB_OFFSET(tf_fs),	db_frame },
 	{ "gs",		DB_OFFSET(tf_gs),	db_frame },
-	{ "ss",		NULL,			db_ss },
+	{ "ss",		DB_OFFSET(tf_ss),	db_frame },
 	{ "rax",	DB_OFFSET(tf_rax),	db_frame },
 	{ "rcx",        DB_OFFSET(tf_rcx),	db_frame },
 	{ "rdx",	DB_OFFSET(tf_rdx),	db_frame },
 	{ "rbx",	DB_OFFSET(tf_rbx),	db_frame },
-	{ "rsp",	NULL,			db_rsp },
+	{ "rsp",	DB_OFFSET(tf_rsp),	db_frame },
 	{ "rbp",	DB_OFFSET(tf_rbp),	db_frame },
 	{ "rsi",	DB_OFFSET(tf_rsi),	db_frame },
 	{ "rdi",	DB_OFFSET(tf_rdi),	db_frame },
@@ -130,13 +128,6 @@ DB_DRX_FUNC(dr5)
 DB_DRX_FUNC(dr6)
 DB_DRX_FUNC(dr7)
 
-static __inline long
-get_rsp(struct trapframe *tf)
-{
-	return ((ISPL(tf->tf_cs)) ? tf->tf_rsp :
-	    (db_expr_t)tf + offsetof(struct trapframe, tf_rsp));
-}
-
 static int
 db_frame(struct db_variable *vp, db_expr_t *valuep, int op)
 {
@@ -153,34 +144,6 @@ db_frame(struct db_variable *vp, db_expr
 	return (1);
 }
 
-static int
-db_rsp(struct db_variable *vp, db_expr_t *valuep, int op)
-{
-
-	if (kdb_frame == NULL)
-		return (0);
-
-	if (op == DB_VAR_GET)
-		*valuep = get_rsp(kdb_frame);
-	else if (ISPL(kdb_frame->tf_cs))
-		kdb_frame->tf_rsp = *valuep;
-	return (1);
-}
-
-static int
-db_ss(struct db_variable *vp, db_expr_t *valuep, int op)
-{
-
-	if (kdb_frame == NULL)
-		return (0);
-
-	if (op == DB_VAR_GET)
-		*valuep = (ISPL(kdb_frame->tf_cs)) ? kdb_frame->tf_ss : rss();
-	else if (ISPL(kdb_frame->tf_cs))
-		kdb_frame->tf_ss = *valuep;
-	return (1);
-}
-
 #define NORMAL		0
 #define	TRAP		1
 #define	INTERRUPT	2
@@ -188,9 +151,7 @@ db_ss(struct db_variable *vp, db_expr_t 
 #define	TRAP_INTERRUPT	5
 
 static void db_nextframe(struct amd64_frame **, db_addr_t *, struct thread *);
-static int db_numargs(struct amd64_frame *);
-static void db_print_stack_entry(const char *, int, char **, long *, db_addr_t,
-    void *);
+static void db_print_stack_entry(const char *, db_addr_t, void *);
 static void decode_syscall(int, struct thread *);
 
 static const char * watchtype_str(int type);
@@ -198,62 +159,11 @@ int  amd64_set_watch(int watchnum, unsig
 		    int access, struct dbreg *d);
 int  amd64_clr_watch(int watchnum, struct dbreg *d);
 
-/*
- * Figure out how many arguments were passed into the frame at "fp".
- */
-static int
-db_numargs(fp)
-	struct amd64_frame *fp;
-{
-#if 1
-	return (0);	/* regparm, needs dwarf2 info */
-#else
-	long	*argp;
-	int	inst;
-	int	args;
-
-	argp = (long *)db_get_value((long)&fp->f_retaddr, 8, FALSE);
-	/*
-	 * XXX etext is wrong for LKMs.  We should attempt to interpret
-	 * the instruction at the return address in all cases.  This
-	 * may require better fault handling.
-	 */
-	if (argp < (long *)btext || argp >= (long *)etext) {
-		args = 5;
-	} else {
-		inst = db_get_value((long)argp, 4, FALSE);
-		if ((inst & 0xff) == 0x59)	/* popl %ecx */
-			args = 1;
-		else if ((inst & 0xffff) == 0xc483)	/* addl $Ibs, %esp */
-			args = ((inst >> 16) & 0xff) / 4;
-		else
-			args = 5;
-	}
-	return (args);
-#endif
-}
-
 static void
-db_print_stack_entry(name, narg, argnp, argp, callpc, frame)
-	const char *name;
-	int narg;
-	char **argnp;
-	long *argp;
-	db_addr_t callpc;
-	void *frame;
+db_print_stack_entry(const char *name, db_addr_t callpc, void *frame)
 {
-	db_printf("%s(", name);
-#if 0
-	while (narg) {
-		if (argnp)
-			db_printf("%s=", *argnp++);
-		db_printf("%lr", (long)db_get_value((long)argp, 8, FALSE));
-		argp++;
-		if (--narg != 0)
-			db_printf(",");
-	}
-#endif
-	db_printf(") at ");
+
+	db_printf("%s() at ", name != NULL ? name : "??");
 	db_printsym(callpc, DB_STGY_PROC);
 	if (frame != NULL)
 		db_printf("/frame 0x%lx", (register_t)frame);
@@ -348,7 +258,7 @@ db_nextframe(struct amd64_frame **fp, db
 		return;
 	}
 
-	db_print_stack_entry(name, 0, 0, 0, rip, &(*fp)->f_frame);
+	db_print_stack_entry(name, rip, &(*fp)->f_frame);
 
 	/*
 	 * Point to base of trapframe which is just above the
@@ -357,7 +267,7 @@ db_nextframe(struct amd64_frame **fp, db
 	tf = (struct trapframe *)((long)*fp + 16);
 
 	if (INKERNEL((long) tf)) {
-		rsp = get_rsp(tf);
+		rsp = tf->tf_rsp;
 		rip = tf->tf_rip;
 		rbp = tf->tf_rbp;
 		switch (frame_type) {
@@ -384,17 +294,13 @@ db_nextframe(struct amd64_frame **fp, db
 }
 
 static int
-db_backtrace(struct thread *td, struct trapframe *tf,
-    struct amd64_frame *frame, db_addr_t pc, int count)
+db_backtrace(struct thread *td, struct trapframe *tf, struct amd64_frame *frame,
+    db_addr_t pc, register_t sp, int count)
 {
 	struct amd64_frame *actframe;
-#define MAXNARG	16
-	char *argnames[MAXNARG], **argnp = NULL;
 	const char *name;
-	long *argp;
 	db_expr_t offset;
 	c_db_sym_t sym;
-	int narg;
 	boolean_t first;
 
 	if (count == -1)
@@ -418,48 +324,51 @@ db_backtrace(struct thread *td, struct t
 		 */
 		actframe = frame;
 		if (first) {
-			if (tf != NULL) {
+			first = FALSE;
+			if (sym == C_DB_SYM_NULL && sp != 0) {
+				/*
+				 * If a symbol couldn't be found, we've probably
+				 * jumped to a bogus location, so try and use
+				 * the return address to find our caller.
+				 */
+				db_print_stack_entry(name, pc, NULL);
+				pc = db_get_value(sp, 8, FALSE);
+				if (db_search_symbol(pc, DB_STGY_PROC,
+				    &offset) == C_DB_SYM_NULL)
+					break;
+				continue;
+			} else if (tf != NULL) {
 				int instr;
 
 				instr = db_get_value(pc, 4, FALSE);
 				if ((instr & 0xffffffff) == 0xe5894855) {
 					/* pushq %rbp; movq %rsp, %rbp */
-					actframe = (void *)(get_rsp(tf) - 8);
+					actframe = (void *)(tf->tf_rsp - 8);
 				} else if ((instr & 0xffffff) == 0xe58948) {
 					/* movq %rsp, %rbp */
-					actframe = (void *)get_rsp(tf);
+					actframe = (void *)tf->tf_rsp;
 					if (tf->tf_rbp == 0) {
 						/* Fake frame better. */
 						frame = actframe;
 					}
 				} else if ((instr & 0xff) == 0xc3) {
 					/* ret */
-					actframe = (void *)(get_rsp(tf) - 8);
+					actframe = (void *)(tf->tf_rsp - 8);
 				} else if (offset == 0) {
 					/* Probably an assembler symbol. */
-					actframe = (void *)(get_rsp(tf) - 8);
+					actframe = (void *)(tf->tf_rsp - 8);
 				}
 			} else if (strcmp(name, "fork_trampoline") == 0) {
 				/*
 				 * Don't try to walk back on a stack for a
 				 * process that hasn't actually been run yet.
 				 */
-				db_print_stack_entry(name, 0, 0, 0, pc,
-				    actframe);
+				db_print_stack_entry(name, pc, actframe);
 				break;
 			}
-			first = FALSE;
-		}
-
-		argp = &actframe->f_arg0;
-		narg = MAXNARG;
-		if (sym != NULL && db_sym_numargs(sym, &narg, argnames)) {
-			argnp = argnames;
-		} else {
-			narg = db_numargs(frame);
 		}
 
-		db_print_stack_entry(name, narg, argnp, argp, pc, actframe);
+		db_print_stack_entry(name, pc, actframe);
 
 		if (actframe != frame) {
 			/* `frame' belongs to caller. */
@@ -473,7 +382,7 @@ db_backtrace(struct thread *td, struct t
 		if (INKERNEL((long)pc) && !INKERNEL((long)frame)) {
 			sym = db_search_symbol(pc, DB_STGY_ANY, &offset);
 			db_symbol_values(sym, &name, NULL);
-			db_print_stack_entry(name, 0, 0, 0, pc, frame);
+			db_print_stack_entry(name, pc, frame);
 			break;
 		}
 		if (!INKERNEL((long) frame)) {
@@ -495,17 +404,19 @@ db_trace_self(void)
 	frame = (struct amd64_frame *)rbp;
 	callpc = (db_addr_t)db_get_value((long)&frame->f_retaddr, 8, FALSE);
 	frame = frame->f_frame;
-	db_backtrace(curthread, NULL, frame, callpc, -1);
+	db_backtrace(curthread, NULL, frame, callpc, 0, -1);
 }
 
 int
 db_trace_thread(struct thread *thr, int count)
 {
 	struct pcb *ctx;
+	struct trapframe *tf;
 
 	ctx = kdb_thr_ctx(thr);
-	return (db_backtrace(thr, NULL, (struct amd64_frame *)ctx->pcb_rbp,
-		    ctx->pcb_rip, count));
+	tf = thr == kdb_thread ? kdb_frame : NULL;
+	return (db_backtrace(thr, tf, (struct amd64_frame *)ctx->pcb_rbp,
+	    ctx->pcb_rip, ctx->pcb_rsp, count));
 }
 
 int

Modified: stable/9/sys/i386/i386/db_trace.c
==============================================================================
--- stable/9/sys/i386/i386/db_trace.c	Thu Nov 12 22:24:39 2015	(r290729)
+++ stable/9/sys/i386/i386/db_trace.c	Thu Nov 12 22:45:51 2015	(r290730)
@@ -390,7 +390,7 @@ db_nextframe(struct i386_frame **fp, db_
 
 static int
 db_backtrace(struct thread *td, struct trapframe *tf, struct i386_frame *frame,
-    db_addr_t pc, int count)
+    db_addr_t pc, register_t sp, int count)
 {
 	struct i386_frame *actframe;
 #define MAXNARG	16
@@ -447,7 +447,21 @@ db_backtrace(struct thread *td, struct t
 		 */
 		actframe = frame;
 		if (first) {
-			if (tf != NULL) {
+			first = FALSE;
+			if (sym == C_DB_SYM_NULL && sp != 0) {
+				/*
+				 * If a symbol couldn't be found, we've probably
+				 * jumped to a bogus location, so try and use
+				 * the return address to find our caller.
+				 */
+				db_print_stack_entry(name, 0, 0, 0, pc,
+				    NULL);
+				pc = db_get_value(sp, 4, FALSE);
+				if (db_search_symbol(pc, DB_STGY_PROC,
+				    &offset) == C_DB_SYM_NULL)
+					break;
+				continue;
+			} else if (tf != NULL) {
 				instr = db_get_value(pc, 4, FALSE);
 				if ((instr & 0xffffff) == 0x00e58955) {
 					/* pushl %ebp; movl %esp, %ebp */
@@ -475,7 +489,6 @@ db_backtrace(struct thread *td, struct t
 				    actframe);
 				break;
 			}
-			first = FALSE;
 		}
 
 		argp = &actframe->f_arg0;
@@ -522,17 +535,19 @@ db_trace_self(void)
 	frame = (struct i386_frame *)ebp;
 	callpc = (db_addr_t)db_get_value((int)&frame->f_retaddr, 4, FALSE);
 	frame = frame->f_frame;
-	db_backtrace(curthread, NULL, frame, callpc, -1);
+	db_backtrace(curthread, NULL, frame, callpc, 0, -1);
 }
 
 int
 db_trace_thread(struct thread *thr, int count)
 {
 	struct pcb *ctx;
+	struct trapframe *tf;
 
 	ctx = kdb_thr_ctx(thr);
-	return (db_backtrace(thr, NULL, (struct i386_frame *)ctx->pcb_ebp,
-		    ctx->pcb_eip, count));
+	tf = thr == kdb_thread ? kdb_frame : NULL;
+	return (db_backtrace(thr, tf, (struct i386_frame *)ctx->pcb_ebp,
+	    ctx->pcb_eip, ctx->pcb_esp, count));
 }
 
 int


More information about the svn-src-stable-9 mailing list