[Crash-utility] Re: [PATCH v4 09/16] x86_64: Add gdb stack unwinding support

On Fri, May 31, 2024 at 5:33 PM <devel-request(a)lists.crash-utility.osci.iowrote: The struct user_regs_bitmap_struct can de defined as below: +struct user_regs_bitmap_struct { + struct user_regs_bitmap_struct uregs; + ulong bitmap; +}; + I would suggest changing the above macro definition to a function, at least there are two advantages: [1] once the task_struct or inactive_task_frame is changed in the future, it is easy to follow its changes. [2] easy to debug, and more readable. Can you keep the same style?(sorry to be picky :-)) + ur_bitmap = (struct user_regs_bitmap_struct *)GETBUF(sizeof(*ur_bitmap)); + memset(ur_bitmap, 0, sizeof(*ur_bitmap)); or + ur_bitmap = (struct user_regs_bitmap_struct *)GETBUF(sizeof(struct user_regs_bitmap_struct))); + memset(ur_bitmap, 0, sizeof(struct user_regs_bitmap_struct)); + It seems this may still rely on the compiler, have you tried another compiler? I'm not sure if there are the same results, just my concerns. Anyway, If there is no better way or solution, at least we should comment on it and point out the current limitations. What do you think? + ur_bitmap->bitmap += 0x3f; Also why is it 0x3f? Can you explain the details? > + } else { > + /* > + * For active tasks in dumpfile, we get regs > through the > + * original way. For active tasks in live, we only > get > + * ip and sp in the end of the function. > + */ > + if (bt->flags & BT_DUMPFILE_SEARCH) { > + FREEBUF(ur_bitmap); > + bt->need_free = FALSE; See the comments in the previous thread. > + *pcp = pcp_save; > + *spp = spp_save; > + return x86_64_get_dumpfile_stack_frame(bt, > pcp, spp); > + } > + } > + } else { > + if (!is_task_active(bt->task)) { > + readmem(*spp, KVADDR, &(ur_bitmap->bp), > + sizeof(ulong), "ur_bitmap->bp", > FAULT_ON_ERROR); > + ur_bitmap->bitmap += 0x10; > + } else { > + if (bt->flags & BT_DUMPFILE_SEARCH) { > + FREEBUF(ur_bitmap); > + bt->need_free = FALSE; > + *pcp = pcp_save; > + *spp = spp_save; > + return x86_64_get_dumpfile_stack_frame(bt, > pcp, spp); > + } > + } > + } > - if (pcp) > - *pcp = x86_64_get_pc(bt); > - if (spp) > - *spp = x86_64_get_sp(bt); > + ur_bitmap->ip = *pcp; > + ur_bitmap->sp = sp; > + ur_bitmap->bitmap += 0x50000; > + > + bt->machdep = ur_bitmap; > + bt->need_free = TRUE; > } > /* > @@ -6458,6 +6578,14 @@ x86_64_ORC_init(void) > MEMBER_OFFSET_INIT(inactive_task_frame_bp, "inactive_task_frame", > "bp"); > MEMBER_OFFSET_INIT(inactive_task_frame_ret_addr, > "inactive_task_frame", "ret_addr"); > + MEMBER_OFFSET_INIT(inactive_task_frame_r15, "inactive_task_frame", > "r15"); > + MEMBER_OFFSET_INIT(inactive_task_frame_r14, "inactive_task_frame", > "r14"); > + MEMBER_OFFSET_INIT(inactive_task_frame_r13, "inactive_task_frame", > "r13"); > + MEMBER_OFFSET_INIT(inactive_task_frame_r12, "inactive_task_frame", > "r12"); > + MEMBER_OFFSET_INIT(inactive_task_frame_flags, > "inactive_task_frame", "flags"); > + MEMBER_OFFSET_INIT(inactive_task_frame_si, "inactive_task_frame", > "si"); > + MEMBER_OFFSET_INIT(inactive_task_frame_di, "inactive_task_frame", > "di"); > + MEMBER_OFFSET_INIT(inactive_task_frame_bx, "inactive_task_frame", > "bx"); > orc->has_signal = MEMBER_EXISTS("orc_entry", "signal"); /* added > at 6.3 */ > orc->has_end = MEMBER_EXISTS("orc_entry", "end"); /* removed > at 6.4 */ > @@ -9071,17 +9199,128 @@ x86_64_get_kvaddr_ranges(struct vaddr_range *vrp) > return cnt; > } > +#define REG_CASE(R, r) \ > + case R##_REGNUM: \ > + if (size != sizeof(ur_bitmap->r)) { \ > + ret = FALSE; break; \ > + } else { \ > + memcpy(value, &ur_bitmap->r, size); \ > + ret = TRUE; break; \ > + } > + > 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) > { > - if (regno >= LAST_REGNUM) > - return FALSE; > + struct bt_info bt_info, bt_setup; > + struct task_context *tc; > + struct user_regs_bitmap_struct *ur_bitmap; > + 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: > + break; > + default: > + return FALSE; > + } Ditto. > - return FALSE; > + tc = CURRENT_CONTEXT(); > + if (!tc) > + return FALSE; > + > + 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); > + > + // reusing the get_dumpfile_regs function to get pt regs structure > + get_dumpfile_regs(&bt_info, &sp, &ip); > + if (bt_info.stackbuf) > + FREEBUF(bt_info.stackbuf); > + ur_bitmap = (struct user_regs_bitmap_struct *)bt_info.machdep; > + if (!ur_bitmap) > + return FALSE; > + > + /* Get all registers from elf notes*/ > + if (!bt_info.need_free) { > + goto get_all; > + } > + Ditto. > + /* Get subset registers from stack frame*/ > + switch (ur_bitmap->bitmap) { > + case 0x5003f: > + switch (regno) { > + case R12_REGNUM ... R15_REGNUM: > + case RBP_REGNUM: > + case RBX_REGNUM: > + case RIP_REGNUM: > + case RSP_REGNUM: > + break; > + default: > + return FALSE; > + } > + break; > + case 0x50010: > + switch (regno) { > + case RBP_REGNUM: > + case RIP_REGNUM: > + case RSP_REGNUM: > + break; > + default: > + return FALSE; > + } > + break; > + case 0x50000: > + switch (regno) { > + case RIP_REGNUM: > + case RSP_REGNUM: > + break; > + default: > + return FALSE; > + } > + break; > + } Can you help to explain more details about several magic number values? Such as 0x5003f, 0x50000, 0x50010. Or you may point out where they come from, it may be helpful to add them here as code comment. Thanks Lianbo + > +get_all: > + switch (regno) { > + REG_CASE(RAX, ax); > + REG_CASE(RBX, bx); > + REG_CASE(RCX, cx); > + REG_CASE(RDX, dx); > + REG_CASE(RSI, si); > + REG_CASE(RDI, di); > + REG_CASE(RBP, bp); > + REG_CASE(RSP, sp); > + REG_CASE(R8, r8); > + REG_CASE(R9, r9); > + REG_CASE(R10, r10); > + REG_CASE(R11, r11); > + REG_CASE(R12, r12); > + REG_CASE(R13, r13); > + REG_CASE(R14, r14); > + REG_CASE(R15, r15); > + REG_CASE(RIP, ip); > + REG_CASE(EFLAGS, flags); > + REG_CASE(CS, cs); > + REG_CASE(SS, ss); > + REG_CASE(DS, ds); > + REG_CASE(ES, es); > + REG_CASE(FS, fs); > + REG_CASE(GS, gs); > + REG_CASE(FS_BASE, fs_base); > + REG_CASE(GS_BASE, gs_base); > + REG_CASE(ORIG_RAX, orig_ax); > + } > + > + if (bt_info.need_free) { > + FREEBUF(ur_bitmap); > + bt_info.need_free = FALSE; > + } > + return ret; > } > /* > -- > 2.40.1