Date: Sun, 22 Dec 2013 17:50:31 +0800 From: Howard Su <howard0su@gmail.com> To: Warner Losh <imp@bsdimp.com> Cc: freebsd-arm <freebsd-arm@freebsd.org> Subject: Re: [PATCH] Add stack unwind support for the functions in .ko Message-ID: <CAAvnz_r1WsxC0Q%2BNOWhSs0Wp_Vs9mjpusZVrixsW1v%2BfwiXDpA@mail.gmail.com> In-Reply-To: <C6E93F79-BA1C-48E9-8689-775D88462921@bsdimp.com> References: <CAAvnz_pWZqw-HEB-GTJ_CkErjXrz3v7ozabMXE__vr=SfkBVSg@mail.gmail.com> <CAAvnz_p2vw9=HWbGPePKm_yDxM-bM1h6VaQBeJq9sYXPRb2-Lg@mail.gmail.com> <20131215173042.0dead636@bender.Home> <CAAvnz_qmn%2Ba%2B-aE4GkYw6TDOPzfST2j5WGvHqKX4WWVsVACSzQ@mail.gmail.com> <C6E93F79-BA1C-48E9-8689-775D88462921@bsdimp.com>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --]
here is the new version which address the print registers out problem.
On Monday, December 16, 2013, Warner Losh wrote:
>
> On Dec 15, 2013, at 4:56 PM, Howard Su wrote:
>
> > On Monday, December 16, 2013, Andrew Turner wrote:
> >
> >> On Mon, 9 Dec 2013 22:44:14 +0800
> >> Howard Su <howard0su@gmail.com <javascript:;> <javascript:;>> wrote:
> >>
> >>> Here is a new version which solve the unreadable $a problem. (the fix
> >>> is in ddb/db_main.c in the end of the patch.)
> >>>
> >>> I attached the diff for review.
> >>
> >> I can't comment on the MD parts of the code, but the ARM change looks
> >> good. My only request is to add a kernel option to turn on printing the
> >> registers in the stack trace as it has been useful for tracking down
> >> bugs.
> >>
> > I read the MD code of MIPS. seems it faces the same situation like arm
> > (register pass parameter). I will follow its pattern to print out
> register.
> > better than an option?
>
> I thought MIPS printed the args from those registers as args and ARM was
> the odd man out...
>
> Warner
>
>
--
-Howard
[-- Attachment #2 --]
diff --git a/sys/arm/arm/db_interface.c b/sys/arm/arm/db_interface.c
index 6906650..77d9e50 100644
--- a/sys/arm/arm/db_interface.c
+++ b/sys/arm/arm/db_interface.c
@@ -182,30 +182,39 @@ db_read_bytes(addr, size, data)
char *data;
{
char *src = (char *)addr;
+ jmp_buf jb;
+ void *prev_jb;
+ int ret;
- if (db_validate_address((u_int)src)) {
- db_printf("address %p is invalid\n", src);
- return (-1);
- }
-
- if (size == 4 && (addr & 3) == 0 && ((uintptr_t)data & 3) == 0) {
- *((int*)data) = *((int*)src);
- return (0);
- }
-
- if (size == 2 && (addr & 1) == 0 && ((uintptr_t)data & 1) == 0) {
- *((short*)data) = *((short*)src);
- return (0);
- }
+ prev_jb = kdb_jmpbuf(jb);
+ ret = setjmp(jb);
+ if (ret == 0) {
- while (size-- > 0) {
if (db_validate_address((u_int)src)) {
db_printf("address %p is invalid\n", src);
- return (-1);
+ ret = -1;
+ goto exit;
+ }
+
+ if (size == 4 && (addr & 3) == 0 && ((uintptr_t)data & 3) == 0) {
+ *((int*)data) = *((int*)src);
+ }
+ else if (size == 2 && (addr & 1) == 0 && ((uintptr_t)data & 1) == 0) {
+ *((short*)data) = *((short*)src);
+ }
+ else while (size-- > 0) {
+ if (db_validate_address((u_int)src)) {
+ db_printf("address %p is invalid\n", src);
+ ret = -1;
+ goto exit;
+ }
+ *data++ = *src++;
}
- *data++ = *src++;
}
- return (0);
+exit:
+ (void)kdb_jmpbuf(prev_jb);
+ return (ret);
+
}
/*
@@ -216,36 +225,48 @@ db_write_bytes(vm_offset_t addr, size_t size, char *data)
{
char *dst;
size_t loop;
+ jmp_buf jb;
+ void *prev_jb;
+ int ret;
+
+ prev_jb = kdb_jmpbuf(jb);
+ ret = setjmp(jb);
+ if (ret == 0) {
+ dst = (char *)addr;
+ if (db_validate_address((u_int)dst)) {
+ db_printf("address %p is invalid\n", dst);
+ ret = -1;
+ goto exit;
+ }
- dst = (char *)addr;
- if (db_validate_address((u_int)dst)) {
- db_printf("address %p is invalid\n", dst);
- return (0);
- }
-
- if (size == 4 && (addr & 3) == 0 && ((uintptr_t)data & 3) == 0)
- *((int*)dst) = *((int*)data);
- else
- if (size == 2 && (addr & 1) == 0 && ((uintptr_t)data & 1) == 0)
- *((short*)dst) = *((short*)data);
- else {
- loop = size;
- while (loop-- > 0) {
- if (db_validate_address((u_int)dst)) {
- db_printf("address %p is invalid\n", dst);
- return (-1);
+ if (size == 4 && (addr & 3) == 0 && ((uintptr_t)data & 3) == 0)
+ *((int*)dst) = *((int*)data);
+ else
+ if (size == 2 && (addr & 1) == 0 && ((uintptr_t)data & 1) == 0)
+ *((short*)dst) = *((short*)data);
+ else {
+ loop = size;
+ while (loop-- > 0) {
+ if (db_validate_address((u_int)dst)) {
+ db_printf("address %p is invalid\n", dst);
+ ret = -1;
+ goto exit;
+ }
+ *dst++ = *data++;
}
- *dst++ = *data++;
}
- }
- /* make sure the caches and memory are in sync */
- cpu_icache_sync_range(addr, size);
+ /* make sure the caches and memory are in sync */
+ cpu_icache_sync_range(addr, size);
- /* In case the current page tables have been modified ... */
- cpu_tlb_flushID();
- cpu_cpwait();
- return (0);
+ /* In case the current page tables have been modified ... */
+ cpu_tlb_flushID();
+ cpu_cpwait();
+ }
+
+exit:
+ (void)kdb_jmpbuf(prev_jb);
+ return (ret);
}
diff --git a/sys/arm/arm/db_trace.c b/sys/arm/arm/db_trace.c
index 57119da..abc8741 100644
--- a/sys/arm/arm/db_trace.c
+++ b/sys/arm/arm/db_trace.c
@@ -38,6 +38,7 @@ __FBSDID("$FreeBSD$");
#include <sys/proc.h>
#include <sys/kdb.h>
#include <sys/stack.h>
+#include <sys/linker.h>
#include <machine/armreg.h>
#include <machine/asm.h>
#include <machine/cpufunc.h>
@@ -73,18 +74,13 @@ __FBSDID("$FreeBSD$");
#define EXIDX_CANTUNWIND 1
/* The register names */
+#define A0 1
#define FP 11
#define SP 13
#define LR 14
#define PC 15
/*
- * These are set in the linker script. Their addresses will be
- * either the start or end of the exception table or index.
- */
-extern int extab_start, extab_end, exidx_start, exidx_end;
-
-/*
* Entry types.
* These are the only entry types that have been seen in the kernel.
*/
@@ -131,8 +127,30 @@ struct unwind_state {
static __inline int32_t
db_expand_prel31(uint32_t prel31)
{
+ return ((int32_t)(prel31) << 1) / 2;
+}
+
+struct db_find_index_context_t
+{
+ int valid;
+ caddr_t addr;
+ caddr_t exidx_start, exidx_end;
+};
+
+static int
+db_find_index_file_cb(linker_file_t lf, void* arg)
+{
+ struct db_find_index_context_t *context = (struct db_find_index_context_t*)arg;
+ if (context->addr >= lf->address && context->addr < lf->address + lf->size)
+ {
+ context->exidx_start = linker_file_lookup_symbol(lf, "__exidx_start", 0);
+ context->exidx_end = linker_file_lookup_symbol(lf, "__exidx_end", 0);
+ if (context->exidx_start && context->exidx_end)
+ context->valid = 1;
+ return 1;
+ }
- return ((int32_t)(prel31 & 0x7fffffffu) << 1) / 2;
+ return 0;
}
/*
@@ -148,10 +166,18 @@ db_find_index(uint32_t addr)
int32_t prel31_addr;
uint32_t func_addr;
- start = (struct unwind_idx *)&exidx_start;
+ struct db_find_index_context_t context;
+ context.valid = 0;
+ context.addr = (caddr_t)addr;
+
+ linker_file_foreach(db_find_index_file_cb, &context);
+ if (!context.valid)
+ return 0;
+
+ start = (struct unwind_idx *)context.exidx_start;
min = 0;
- max = (&exidx_end - &exidx_start) / 2;
+ max = (context.exidx_end - context.exidx_start) / 2;
while (min != max) {
mid = min + (max - min + 1) / 2;
@@ -269,7 +295,7 @@ db_unwind_exec_insn(struct unwind_state *state)
/* Stop processing */
state->entries = 0;
- } else if ((insn == INSN_POP_REGS)) {
+ } else if (insn == INSN_POP_REGS) {
unsigned int mask, reg;
mask = db_unwind_exec_read_byte(state);
@@ -352,30 +378,26 @@ db_unwind_tab(struct unwind_state *state)
}
static void
-db_stack_trace_cmd(struct unwind_state *state)
+db_stack_trace_cmd(struct unwind_state *state, int count)
{
struct unwind_idx *index;
const char *name;
db_expr_t value;
db_expr_t offset;
c_db_sym_t sym;
- u_int reg, i;
- char *sep;
- uint16_t upd_mask;
+ u_int j;
bool finished;
finished = false;
- while (!finished) {
+ state->start_pc = state->registers[PC];
+ while (!finished && count--) {
/* Reset the mask of updated registers */
state->update_mask = 0;
- /* The pc value is correct and will be overwritten, save it */
- state->start_pc = state->registers[PC];
-
/* Find the item to run */
index = db_find_index(state->start_pc);
- if (index->insn != EXIDX_CANTUNWIND) {
+ if (index && index->insn != EXIDX_CANTUNWIND) {
if (index->insn & (1U << 31)) {
/* The data is within the instruction */
state->insn = &index->insn;
@@ -396,35 +418,21 @@ db_stack_trace_cmd(struct unwind_state *state)
name = "(null)";
} else
db_symbol_values(sym, &name, &value);
- db_printf("%s() at ", name);
+
db_printsym(state->start_pc, DB_STGY_PROC);
- db_printf("\n");
- db_printf("\t pc = 0x%08x lr = 0x%08x (", state->start_pc,
- state->registers[LR]);
- db_printsym(state->registers[LR], DB_STGY_PROC);
- db_printf(")\n");
- db_printf("\t sp = 0x%08x fp = 0x%08x",
- state->registers[SP], state->registers[FP]);
-
- /* Don't print the registers we have already printed */
- upd_mask = state->update_mask &
- ~((1 << SP) | (1 << FP) | (1 << LR) | (1 << PC));
- sep = "\n\t";
- for (i = 0, reg = 0; upd_mask != 0; upd_mask >>= 1, reg++) {
- if ((upd_mask & 1) != 0) {
- db_printf("%s%sr%d = 0x%08x", sep,
- (reg < 10) ? " " : "", reg,
- state->registers[reg]);
- i++;
- if (i == 2) {
- sep = "\n\t";
- i = 0;
- } else
- sep = " ";
-
- }
+ db_printf("(");
+ for (j = 0; j < 4; j ++) {
+ if (j > 0)
+ db_printf(",");
+ if (state->update_mask & (1 << (A0 + j)))
+ db_printf("%x", state->registers[A0 + j]);
+ else
+ db_printf("?");
}
- db_printf("\n");
+ db_printf(") lr=%08x sp=%08x fp=%08x\n", state->registers[LR], state->registers[SP], state->registers[FP]);
+
+ /* The pc value is correct and will be overwritten, save it */
+ state->start_pc = state->registers[PC];
/*
* Stop if directed to do so, or if we've unwound back to the
@@ -435,14 +443,17 @@ db_stack_trace_cmd(struct unwind_state *state)
* the last frame printed before you see the unwind failure
* message (maybe it needs a STOP_UNWINDING).
*/
- if (index->insn == EXIDX_CANTUNWIND) {
- db_printf("Unable to unwind further\n");
+ if (index && index->insn == EXIDX_CANTUNWIND) {
+ if (count)
+ db_printf("Unable to unwind further\n");
finished = true;
} else if (state->registers[PC] < VM_MIN_KERNEL_ADDRESS) {
- db_printf("Unable to unwind into user mode\n");
+ if (count)
+ db_printf("Unable to unwind into user mode\n");
finished = true;
} else if (state->update_mask == 0) {
- db_printf("Unwind failure (no registers changed)\n");
+ if (count)
+ db_printf("Unwind failure (no registers changed)\n");
finished = true;
}
}
@@ -479,7 +490,7 @@ db_stack_trace_cmd(struct unwind_state *state)
#ifndef __ARM_EABI__ /* The frame format is differend in AAPCS */
static void
-db_stack_trace_cmd(db_expr_t addr, db_expr_t count, boolean_t kernel_only)
+db_stack_trace_cmd(db_expr_t addr, int count, boolean_t kernel_only)
{
u_int32_t *frame, *lastframe;
c_db_sym_t sym;
@@ -608,9 +619,9 @@ db_trace_thread(struct thread *thr, int count)
state.registers[LR] = ctx->un_32.pcb32_lr;
state.registers[PC] = ctx->un_32.pcb32_pc;
- db_stack_trace_cmd(&state);
+ db_stack_trace_cmd(&state, count);
#else
- db_stack_trace_cmd(ctx->un_32.pcb32_r11, -1, TRUE);
+ db_stack_trace_cmd(ctx->un_32.pcb32_r11, count, TRUE);
#endif
} else
db_trace_self();
@@ -632,7 +643,7 @@ db_trace_self(void)
state.registers[LR] = (uint32_t)__builtin_return_address(0);
state.registers[PC] = (uint32_t)db_trace_self;
- db_stack_trace_cmd(&state);
+ db_stack_trace_cmd(&state, -1);
#else
db_addr_t addr;
diff --git a/sys/conf/kmod.mk b/sys/conf/kmod.mk
index bd05878..d0a1f0f 100644
--- a/sys/conf/kmod.mk
+++ b/sys/conf/kmod.mk
@@ -133,6 +133,16 @@ CFLAGS+= -mlongcall -fno-omit-frame-pointer
CFLAGS+= -G0 -fno-pic -mno-abicalls -mlong-calls
.endif
+.if ${MACHINE_CPUARCH} == arm
+.if !defined(WITHOUT_ARM_EABI)
+CFLAGS+= -funwind-tables
+.if ${COMPILER_TYPE} == "clang"
+# clang requires us to tell it to emit assembly with unwind information
+CFLAGS+= -mllvm -arm-enable-ehabi
+.endif
+.endif
+.endif
+
.if defined(DEBUG) || defined(DEBUG_FLAGS)
CTFFLAGS+= -g
.endif
diff --git a/sys/conf/ldscript.arm b/sys/conf/ldscript.arm
index 0d1c7ee..2482ce7 100644
--- a/sys/conf/ldscript.arm
+++ b/sys/conf/ldscript.arm
@@ -57,17 +57,11 @@ SECTIONS
.plt : { *(.plt) }
. = ALIGN(4);
- _extab_start = .;
- PROVIDE(extab_start = .);
.ARM.extab : { *(.ARM.extab) }
- _extab.end = .;
- PROVIDE(extab_end = .);
- _exidx_start = .;
- PROVIDE(exidx_start = .);
+ __exidx_start = .;
.ARM.exidx : { *(.ARM.exidx) }
- _exidx_end = .;
- PROVIDE(exidx_end = .);
+ __exidx_end = .;
/* Adjust the address for the data segment. We want to adjust up to
the same address within the page on the next page up. */
diff --git a/sys/ddb/db_main.c b/sys/ddb/db_main.c
index 6e9286c..f64b2e2 100644
--- a/sys/ddb/db_main.c
+++ b/sys/ddb/db_main.c
@@ -86,6 +86,24 @@ X_db_lookup(db_symtab_t *symtab, const char *symbol)
return (NULL);
}
+/*
+ * check if the name is valid.
+ * invliad the name $a,$d,%t generated by ELF ARM toolchain
+ */
+static int
+db_is_valid_name(const char* name)
+{
+ if (name == NULL || name[0] == '\0')
+ return 0;
+ if (name[0] == '$' && strchr("atd", name[1])
+ && name[2] == '\0')
+ {
+ return 0;
+ }
+
+ return 1;
+}
+
c_db_sym_t
X_db_search_symbol(db_symtab_t *symtab, db_addr_t off, db_strategy_t strat,
db_expr_t *diffp)
@@ -93,11 +111,16 @@ X_db_search_symbol(db_symtab_t *symtab, db_addr_t off, db_strategy_t strat,
c_linker_sym_t lsym;
Elf_Sym *sym, *match;
unsigned long diff;
+ const char* name;
if (symtab->private == NULL) {
if (!linker_ddb_search_symbol((caddr_t)off, &lsym, &diff)) {
- *diffp = (db_expr_t)diff;
- return ((c_db_sym_t)lsym);
+ X_db_symbol_values(symtab, (c_db_sym_t)lsym, &name, NULL);
+ if (db_is_valid_name(name))
+ {
+ *diffp = (db_expr_t)diff;
+ return ((c_db_sym_t)lsym);
+ }
}
return (NULL);
}
@@ -113,8 +136,14 @@ X_db_search_symbol(db_symtab_t *symtab, db_addr_t off, db_strategy_t strat,
ELF_ST_TYPE(sym->st_info) != STT_FUNC &&
ELF_ST_TYPE(sym->st_info) != STT_NOTYPE)
continue;
+
if ((off - sym->st_value) > diff)
continue;
+
+ X_db_symbol_values(symtab, (c_db_sym_t)sym, &name, NULL);
+ if (!db_is_valid_name(name))
+ continue;
+
if ((off - sym->st_value) < diff) {
diff = off - sym->st_value;
match = sym;
help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAAvnz_r1WsxC0Q%2BNOWhSs0Wp_Vs9mjpusZVrixsW1v%2BfwiXDpA>