[Crash-utility] Re: [[PATCH v2]] Clean up on top of one-thread-v2

Hi folks, please take a look on v2 of this patch and give it a try. IMHO, Only remaining piece is to clean up x86_64_get_current_task_reg around getting pt_regs. Things done: 1. Rename machdep->get_cpu_reg to machdep->get_current_task_reg and rename x86_64_get_cpu_reg to x86_64_get_current_task_reg, plus corresponding changes for arm64 and ppc64. 2. Untied gdb's TPID from any tasks or CPUs ID. Always use CURRENT_CONTEXT for regs and properties fetching. 3. When CURRENT switched, gdb_change_thread_context() must be called to invalidate caches. 4. Update x86_64_get_current_task_reg to support inactive tasks for offline debugging. 5. Fixed frame selection issue. It was caused by multiple calls of set_context() which calls gdb_change_thread_context() unnecessarily dropping gdb caches. TODO: 1. x86_64_get_current_task_reg is questionable about fetching pt_regs for inactive tasks. How do you gyus get pt_regs structure on modern linux kernels?

PS: I added my approach. More testing required. Known issues 1. After switching from all tasks to one thread: b. unwinding started to work differently around top (garbaged) frames - Not quite an issue for me. Signed-off-by: Alexey Makhalov <alexey.makhalov(a)broadcom.com&gt; --- arm64.c | 4 ++-- crash_target.c | 48 ++++++++++++++++++------------------- defs.h | 10 +++----- gdb_interface.c | 8 +++---- kernel.c | 23 ------------------ ppc64.c | 8 +++---- task.c | 16 ++++--------- x86_64.c | 64 ++++++++++++++++++++++++++++++++++++++++++++----- 8 files changed, 99 insertions(+), 82 deletions(-) diff --git a/arm64.c b/arm64.c index 8ff10bf..340db2d 100644 --- a/arm64.c +++ b/arm64.c @@ -153,7 +153,7 @@ static void arm64_calc_kernel_start(void) } static int -arm64_get_cpu_reg(int cpu, int regno, const char *name, +arm64_get_current_task_reg(int regno, const char *name, int size, void *value) { struct bt_info bt_info, bt_setup; @@ -511,7 +511,7 @@ arm64_init(int when) machdep->dumpfile_init = NULL; machdep->verify_line_number = NULL; machdep->init_kernel_pgd = arm64_init_kernel_pgd; - machdep->get_cpu_reg = arm64_get_cpu_reg; + machdep->get_current_task_reg = arm64_get_current_task_reg; /* use machdep parameters */ arm64_calc_phys_offset(); diff --git a/crash_target.c b/crash_target.c index fb3b634..6e96506 100644 --- a/crash_target.c +++ b/crash_target.c @@ -26,11 +26,8 @@ void crash_target_init (void); extern "C" int gdb_readmem_callback(unsigned long, void *, int, int); -extern "C" int crash_get_cpu_reg (int cpu, int regno, const char *regname, +extern "C" int crash_get_current_task_reg (int regno, const char *regname, int regsize, void *val); -extern "C" void gdb_refresh_regcache(unsigned int cpu); -extern "C" int set_cpu(int cpu, int print_context); -extern "C" int crash_set_thread(ulong); extern "C" int gdb_change_thread_context (ulong); extern "C" void crash_get_current_task_info(unsigned long *pid, char **comm); @@ -72,22 +69,27 @@ public: }; -/* We just get all the registers, so we don't use regno. */ void crash_target::fetch_registers (struct regcache *regcache, int regno) { + int r; gdb_byte regval[16]; - int cpu = inferior_ptid.tid(); struct gdbarch *arch = regcache->arch (); - for (int r = 0; r < gdbarch_num_regs (arch); r++) + if (regno >= 0) { + r = regno; + goto onetime; + } + + for (r = 0; regno == -1 && r < gdbarch_num_regs (arch); r++) { +onetime: const char *regname = gdbarch_register_name(arch, r); int regsize = register_size (arch, r); if (regsize > sizeof (regval)) error (_("fatal error: buffer size is not enough to fit register value")); - if (crash_get_cpu_reg (cpu, r, regname, regsize, (void *)&regval)) + if (crash_get_current_task_reg (r, regname, regsize, (void *)&regval)) regcache->raw_supply (r, regval); else regcache->raw_supply (r, NULL); @@ -142,19 +144,17 @@ crash_target_init (void) extern "C" int gdb_change_thread_context (ulong task) { - inferior* inf = current_inferior(); - int cpu = crash_set_thread(task); - if (cpu < 0) - return FALSE; - - ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu); - thread_info *tp = find_thread_ptid (inf, ptid); - - if (tp == nullptr) - return FALSE; - - target_fetch_registers(get_thread_regcache(tp), -1); - switch_to_thread(tp); - reinit_frame_cache (); - return TRUE; -} \ No newline at end of file + static ulong prev_task = 0; + + /* + * Crash calls this method even if CURRENT task is not updated. + * Ignore it and keep gdb caches active. + */ + if (task == prev_task) + return TRUE; + prev_task = task; + + registers_changed(); + reinit_frame_cache(); + return TRUE; +} diff --git a/defs.h b/defs.h index d7e3b18..7596179 100644 --- a/defs.h +++ b/defs.h @@ -1081,7 +1081,7 @@ struct machdep_table { void (*get_irq_affinity)(int); void (*show_interrupts)(int, ulong *); int (*is_page_ptr)(ulong, physaddr_t *); - int (*get_cpu_reg)(int, int, const char *, int, void *); + int (*get_current_task_reg)(int, const char *, int, void *); int (*is_cpu_prstatus_valid)(int cpu); }; @@ -6124,7 +6124,6 @@ void dump_kernel_table(int); void dump_bt_info(struct bt_info *, char *where); void dump_log(int); void parse_kernel_version(char *); -int crash_set_thread(ulong); #define LOG_LEVEL(v) ((v) & 0x07) #define SHOW_LOG_LEVEL (0x1) @@ -8023,7 +8022,7 @@ extern int have_full_symbols(void); /* * Register numbers must be in sync with gdb/features/i386/64bit-core.c - * to make crash_target->fetch_registers() ---> machdep->get_cpu_reg() + * to make crash_target->fetch_registers() ---> machdep->get_current_task_reg() * working properly. */ enum x86_64_regnum { @@ -8081,7 +8080,7 @@ enum arm64_regnum { /* * Register numbers to make crash_target->fetch_registers() - * ---> machdep->get_cpu_reg() work properly. + * ---> machdep->get_current_task_reg() work properly. * * These register numbers and names are given according to output of * `rs6000_register_name`, because that is what was being used by @@ -8199,7 +8198,4 @@ enum ppc64_regnum { PPC64_VRSAVE_REGNU = 139 }; -/* crash_target.c */ -extern void gdb_refresh_regcache (unsigned int cpu); - #endif /* !GDB_COMMON */ diff --git a/gdb_interface.c b/gdb_interface.c index 28264c2..2e9117a 100644 --- a/gdb_interface.c +++ b/gdb_interface.c @@ -1073,14 +1073,14 @@ unsigned long crash_get_kaslr_offset(void) } /* Callbacks for crash_target */ -int crash_get_cpu_reg (int cpu, int regno, const char *regname, +int crash_get_current_task_reg (int regno, const char *regname, int regsize, void *val); -int crash_get_cpu_reg (int cpu, int regno, const char *regname, +int crash_get_current_task_reg (int regno, const char *regname, int regsize, void *value) { - if (!machdep->get_cpu_reg) + if (!machdep->get_current_task_reg) return FALSE; - return machdep->get_cpu_reg(cpu, regno, regname, regsize, value); + return machdep->get_current_task_reg(regno, regname, regsize, value); } diff --git a/kernel.c b/kernel.c index 446fd78..15b00dc 100644 --- a/kernel.c +++ b/kernel.c @@ -6543,29 +6543,6 @@ set_cpu(int cpu, int print_context) show_context(CURRENT_CONTEXT()); } -int -crash_set_thread(ulong task) -{ - bool found = FALSE; - struct task_context *tc = FIRST_CONTEXT(); - - for (int i = 0; i < RUNNING_TASKS(); i++, tc++) { - if (tc->task == task) { - found = TRUE; - break; - } - } - - if (!found) - return -1; - - if (CURRENT_TASK() == tc->task) - return 0; - - set_context(tc->task, NO_PID, TRUE); - return 0; -} - /* * Collect the irq_desc[] entry along with its associated handler and * action structures. diff --git a/ppc64.c b/ppc64.c index a87e621..aeccb70 100644 --- a/ppc64.c +++ b/ppc64.c @@ -55,7 +55,7 @@ static void ppc64_set_bt_emergency_stack(enum emergency_stack_type type, static char * ppc64_check_eframe(struct ppc64_pt_regs *); static void ppc64_print_eframe(char *, struct ppc64_pt_regs *, struct bt_info *); -static int ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size, +static int ppc64_get_current_task_reg(int regno, const char *name, int size, void *value); static void parse_cmdline_args(void); static int ppc64_paca_percpu_offset_init(int); @@ -706,7 +706,7 @@ ppc64_init(int when) error(FATAL, "cannot malloc hwirqstack buffer space."); } - machdep->get_cpu_reg = ppc64_get_cpu_reg; + machdep->get_current_task_reg = ppc64_get_current_task_reg; ppc64_init_paca_info(); @@ -2506,7 +2506,7 @@ ppc64_print_eframe(char *efrm_str, struct ppc64_pt_regs *regs, } static int -ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size, +ppc64_get_current_task_reg(int regno, const char *name, int size, void *value) { struct bt_info bt_info, bt_setup; @@ -2555,7 +2555,7 @@ ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size, pt_regs = (struct ppc64_pt_regs *)bt_info.machdep; if (!pt_regs) { - error(WARNING, "pt_regs not available for cpu %d\n", cpu); + error(WARNING, "pt_regs not available for cpu %d\n", tc->processor); return FALSE; } diff --git a/task.c b/task.c index f7e5b05..5f1ddad 100644 --- a/task.c +++ b/task.c @@ -11286,19 +11286,11 @@ check_stack_end_magic: fprintf(fp, "No stack overflows detected\n"); } +void crash_get_current_task_info(unsigned long *pid, char **comm); void crash_get_current_task_info(unsigned long *pid, char **comm) { - int i; - struct task_context *tc; + struct task_context *tc = CURRENT_CONTEXT(); - tc = FIRST_CONTEXT(); - for (i = 0; i < RUNNING_TASKS(); i++, tc++) - if (tc->task == CURRENT_TASK()) { - *pid = tc->pid; - *comm = tc->comm; - return; - } - *pid = 0; - *comm = NULL; - return; + *pid = tc->pid; + *comm = tc->comm; } diff --git a/x86_64.c b/x86_64.c index 41e67c6..73bb353 100644 --- a/x86_64.c +++ b/x86_64.c @@ -126,7 +126,7 @@ static int x86_64_get_framesize(struct bt_info *, ulong, ulong, char *); static void x86_64_framesize_debug(struct bt_info *); static void x86_64_get_active_set(void); static int x86_64_get_kvaddr_ranges(struct vaddr_range *); -static int x86_64_get_cpu_reg(int, int, const char *, int, void *); +static int x86_64_get_current_task_reg(int, const char *, int, void *); static int x86_64_verify_paddr(uint64_t); static void GART_init(void); static void x86_64_exception_stacks_init(void); @@ -207,7 +207,7 @@ x86_64_init(int when) machdep->machspec->irq_eframe_link = UNINITIALIZED; machdep->machspec->irq_stack_gap = UNINITIALIZED; machdep->get_kvaddr_ranges = x86_64_get_kvaddr_ranges; - machdep->get_cpu_reg = x86_64_get_cpu_reg; + machdep->get_current_task_reg = x86_64_get_current_task_reg; if (machdep->cmdline_args[0]) parse_cmdline_args(); if ((string = pc->read_vmcoreinfo("relocate"))) { @@ -898,7 +898,7 @@ x86_64_dump_machdep_table(ulong arg) fprintf(fp, " is_page_ptr: x86_64_is_page_ptr()\n"); fprintf(fp, " verify_paddr: x86_64_verify_paddr()\n"); fprintf(fp, " get_kvaddr_ranges: x86_64_get_kvaddr_ranges()\n"); - fprintf(fp, " get_cpu_reg: x86_64_get_cpu_reg()\n"); + fprintf(fp, "get_current_task_reg: x86_64_get_current_task_reg()\n"); fprintf(fp, " init_kernel_pgd: x86_64_init_kernel_pgd()\n"); fprintf(fp, "clear_machdep_cache: x86_64_clear_machdep_cache()\n"); fprintf(fp, " xendump_p2m_create: %s\n", PVOPS_XEN() ? @@ -9186,7 +9186,7 @@ x86_64_get_kvaddr_ranges(struct vaddr_range *vrp) } static int -x86_64_get_cpu_reg(int cpu, int regno, const char *name, +x86_64_get_current_task_reg(int regno, const char *name, int size, void *value) { struct bt_info bt_info, bt_setup; @@ -9195,8 +9195,6 @@ x86_64_get_cpu_reg(int cpu, int regno, const char *name, ulong ip, sp; bool ret = FALSE; - if (VMSS_DUMPFILE()) - return vmware_vmss_get_cpu_reg(cpu, regno, name, size, value); switch (regno) { case RAX_REGNUM ... GS_REGNUM: case FS_BASE_REGNUM ... ORIG_RAX_REGNUM: @@ -9208,6 +9206,60 @@ x86_64_get_cpu_reg(int cpu, int regno, const char *name, tc = CURRENT_CONTEXT(); if (!tc) return FALSE; + + /* + * For inactive task, grab rip, rbp, rbx, r12, r13, r14 and r15 from + * inactive_task_frame (see __switch_to_asm). Other regs saved on + * regular frame. + */ + if (!is_task_active(tc->task)) { + int frame_size = STRUCT_SIZE("inactive_task_frame"); + + /* Only modern kernels supported. */ + if (tt->flags & THREAD_INFO && frame_size == 7 * 8) { + ulong rsp; + int offset = 0; + switch (regno) { + case RSP_REGNUM: + readmem(tc->task + OFFSET(task_struct_thread) + + OFFSET(thread_struct_rsp), KVADDR, + &rsp, sizeof(void *), + "thread_struct rsp", FAULT_ON_ERROR); + rsp += frame_size; + memcpy(value, &rsp, size); + return TRUE; + case RIP_REGNUM: + offset += 8; + case RBP_REGNUM: + offset += 8; + case RBX_REGNUM: + offset += 8; + case R12_REGNUM: + offset += 8; + case R13_REGNUM: + offset += 8; + case R14_REGNUM: + offset += 8; + case R15_REGNUM: + readmem(tc->task + OFFSET(task_struct_thread) + + OFFSET(thread_struct_rsp), KVADDR, + &rsp, sizeof(void *), + "thread_struct rsp", FAULT_ON_ERROR); + readmem(rsp + offset, KVADDR, value, sizeof(void *), + "inactive_thread_frame saved regs", FAULT_ON_ERROR); + return TRUE; + } + } + /* TBD: older kernels support. */ + return FALSE; + } + + /* + * Task is active, grab CPU's registers + */ + if (VMSS_DUMPFILE()) + return vmware_vmss_get_cpu_reg(tc->processor, regno, name, size, value); + BZERO(&bt_setup, sizeof(struct bt_info)); clone_bt_info(&bt_setup, &bt_info, tc); fill_stackbuf(&bt_info); -- 2.39.0