11:22 p.m.
The Problem:
============
Currently crash is unable to show function arguments and local variables, as
gdb can do. And functionality for moving between frames ('up'/'down') is
not
working in crash.
Crash has 'gdb passthroughs' for things gdb can do, but the gdb passthroughs
'bt', 'frame', 'info locals', 'up', 'down' are not
working either, due to
gdb not getting the register values from `crash_target::fetch_registers`,
which then uses `machdep->get_cpu_reg`, which is not implemented for PPC64
Proposed Solution:
==================
Fix the gdb passthroughs by implementing "machdep->get_cpu_reg" for PPC64.
This way, "gdb mode in crash" will support this feature for both ELF and
kdump-compressed vmcore formats, while "gdb" would only have supported ELF
format
This way other features of 'gdb', such as seeing
backtraces/registers/variables/arguments/local variables, moving up and
down stack frames, can be used with any ppc64 vmcore, irrespective of
being ELF format or kdump-compressed format.
Note: This doesn't support live debugging on ppc64, since registers are not
available to be read
Implications on Architectures:
====================================
No architecture other than PPC64 has been affected, other than in case of
'frame' command
As mentioned in patch #2, since frame will not be prohibited, so it will print:
crash> frame
#0 <unavailable> in ?? ()
Instead of before prohibited message:
crash> frame
crash: prohibited gdb command: frame
Major change will be in 'gdb mode' on PPC64, that it will print the frames, and
local variables, instead of failing with errors showing no frame, or showing
that couldn't get PC, it will be able to give all this information.
Testing:
========
Git tree with this patch series applied:
https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
To test various gdb passthroughs:
(crash) set
(crash) set gdb on
gdb> thread
gdb> bt
gdb> info threads
gdb> info threads
gdb> info locals
gdb> info variables irq_rover_lock
gdb> info args
gdb> thread 2
gdb> set gdb off
(crash) set
(crash) set -c 6
(crash) gdb thread
(crash) bt
(crash) gdb bt
(crash) frame
(crash) gdb up
(crash) gdb down
(crash) info locals
Known Issues:
=============
1. In gdb mode, 'bt' might fail to show backtrace in few vmcores collected
from older kernels. This is a known issue due to register mismatch, and
its fix has been merged upstream:
This can also cause some 'invalid kernel virtual address' errors during gdb
unwinding the stack registers
Commit: https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
Fixing GDB passthroughs on other architectures
==============================================
Much of the work for making gdb passthroughs like 'gdb bt', 'gdb
thread', 'gdb info locals' etc. has been done by the patches introducing
'machdep->get_cpu_reg' and this series fixing some issues in that.
Other architectures should be able to fix these gdb functionalities by
simply implementing 'machdep->get_cpu_reg (cpu, regno, ...)'.
The reasoning behind that has been explained with a diagram in commit
description of patch #1
I will assist with my findings/observations fixing it on ppc64 whenever needed.
Changelog:
==========
V9:
+ minor change in patch #5: sync gdb context on a 'set' and 'set -p'
+ add taoliu's patch for using current context, and fixes in ppc64_get_cpu_reg
V8:
+ use get_active_task instead of depending on CURRENT_CONTEXT in ppc64_get_cpu_reg
+ rebase to upstream/master (5977936c0a91)
V7:
+ move changes in gdb-10.2.patch to the end (minor change in patch #3,4,5)
+ fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
+ use ascii diagram in patch #1 description
V6:
+ changes in patch #5: fix bug introduced in v5 that caused initial gdb thread
to be thread 1
V5:
+ changes in patch #1: made ppc64_get_cpu_reg static, and remove unreachable
code
+ changes in patch #3: fixed typo 'ppc64_renum' instead of
'ppc64_regnum',
remove unneeded if condition
+ changes in patch #5: implement refresh regcache on per thread, instead of all
threads at once
V4:
+ fix segmentation fault in live debugging (change in patch #1)
+ mention live debugging not supported in cover letter and patch #1
+ fixed some checkpatch warnings (change in patch #5)
V3:
+ default gdb thread will be the crashing thread, instead of being
thread '0'
+ synchronise crash cpu and gdb thread context
+ fix bug in gdb_interface, that replaced gdb's output stream, losing
output in some cases, such as info threads and extra output in info
variables
+ fix 'info threads'
RFC V2:
- removed patch implementing 'frame', 'up', 'down' in crash
- updated the cover letter by removing the mention of those commands other
than the respective gdb passthrough
Aditya Gupta (5):
ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
remove 'frame' from prohibited commands list
synchronise cpu context changes between crash/gdb
fix gdb_interface: restore gdb's output streams at end of
gdb_interface
fix 'info threads' command
crash_target.c | 44 ++++++++++++++++
defs.h | 130 +++++++++++++++++++++++++++++++++++++++++++++++-
gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
gdb_interface.c | 2 +-
kernel.c | 47 +++++++++++++++--
ppc64.c | 95 +++++++++++++++++++++++++++++++++--
task.c | 14 ++++++
tools.c | 2 +-
8 files changed, 434 insertions(+), 10 deletions(-)
--
2.41.0
11:22 p.m.
New subject: [PATCH v9 1/6] ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
Currently, gdb passthroughs of 'bt', 'frame', 'up',
'down', 'info
locals' don't work. This is due to gdb not knowing the register values to
unwind the stack frames
Every gdb passthrough goes through `gdb_interface`. And then, gdb expects
`crash_target::fetch_registers` to give it the register values, which is
dependent on `machdep->get_cpu_reg` to read the register values for
specific architecture.
----------------------------
gdb passthrough (eg. "bt") | |
crash -------------------------> | |
| gdb_interface |
| |
| |
| ---------------------- |
fetch_registers | | | |
crash_target<-------------------------+--| gdb | |
--------------------------+->| | |
Registers (SP,NIP, etc.)| | | |
| | | |
| ---------------------- |
----------------------------
Implement `machdep->get_cpu_reg` on PPC64, so that crash provides the
register values to gdb to unwind stack frames properly
With these changes, on powerpc, 'bt' command output in gdb mode, will look
like this:
gdb> bt
#0 0xc0000000002a53e8 in crash_setup_regs (oldregs=<optimized out>,
newregs=0xc00000000486f8d8) at ./arch/powerpc/include/asm/kexec.h:69
#1 __crash_kexec (regs=<optimized out>) at kernel/kexec_core.c:974
#2 0xc000000000168918 in panic (fmt=<optimized out>) at kernel/panic.c:358
#3 0xc000000000b735f8 in sysrq_handle_crash (key=<optimized out>) at
drivers/tty/sysrq.c:155
#4 0xc000000000b742cc in __handle_sysrq (key=key@entry=99,
check_mask=check_mask@entry=false) at drivers/tty/sysrq.c:602
#5 0xc000000000b7506c in write_sysrq_trigger (file=<optimized out>,
buf=<optimized out>, count=2, ppos=<optimized out>) at
drivers/tty/sysrq.c:1163
#6 0xc00000000069a7bc in pde_write (ppos=<optimized out>, count=<optimized
out>, buf=<optimized out>, file=<optimized out>, pde=0xc000000009ed3a80) at
fs/proc/inode.c:340
#7 proc_reg_write (file=<optimized out>, buf=<optimized out>,
count=<optimized out>, ppos=<optimized out>) at fs/proc/inode.c:352
#8 0xc0000000005b3bbc in vfs_write (file=file@entry=0xc00000009dda7d00,
buf=buf@entry=0xebcfc7c6040 <error: Cannot access memory at address 0xebcfc7c6040>,
count=count@entry=2, pos=pos@entry=0xc00000000486fda0) at fs/read_write.c:582
instead of earlier output without this patch:
gdb> bt
#0 <unavailable> in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
Also, 'get_dumpfile_regs' has been introduced to get registers from
multiple supported vmcore formats. Correspondingly a flag 'BT_NO_PRINT_REGS'
has been introduced to tell helper functions to get registers, to not
print registers with every call to backtrace in gdb.
Note: This feature to support GDB unwinding doesn't support live debugging
Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
---
defs.h | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
kernel.c | 33 +++++++++++++++
ppc64.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 263 insertions(+), 3 deletions(-)
diff --git a/defs.h b/defs.h
index 98650e8780bf..414f039b35f1 100644
--- a/defs.h
+++ b/defs.h
@@ -6095,6 +6095,7 @@ int load_module_symbols_helper(char *);
void unlink_module(struct load_module *);
int check_specified_module_tree(char *, char *);
int is_system_call(char *, ulong);
+void get_dumpfile_regs(struct bt_info*, ulong*, ulong*);
void generic_dump_irq(int);
void generic_get_irq_affinity(int);
void generic_show_interrupts(int, ulong *);
@@ -6194,6 +6195,7 @@ ulong cpu_map_addr(const char *type);
#define BT_REGS_NOT_FOUND (0x4000000000000ULL)
#define BT_OVERFLOW_STACK (0x8000000000000ULL)
#define BT_SKIP_IDLE (0x10000000000000ULL)
+#define BT_NO_PRINT_REGS (0x20000000000000ULL)
#define BT_SYMBOL_OFFSET (BT_SYMBOLIC_ARGS)
#define BT_REF_HEXVAL (0x1)
@@ -8050,4 +8052,124 @@ enum x86_64_regnum {
LAST_REGNUM
};
+/*
+ * Register numbers to make crash_target->fetch_registers()
+ * ---> machdep->get_cpu_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
+ * crash_target::fetch_registers in case of PPC64
+ */
+enum ppc64_regnum {
+ PPC64_R0_REGNUM = 0,
+ PPC64_R1_REGNUM,
+ PPC64_R2_REGNUM,
+ PPC64_R3_REGNUM,
+ PPC64_R4_REGNUM,
+ PPC64_R5_REGNUM,
+ PPC64_R6_REGNUM,
+ PPC64_R7_REGNUM,
+ PPC64_R8_REGNUM,
+ PPC64_R9_REGNUM,
+ PPC64_R10_REGNUM,
+ PPC64_R11_REGNUM,
+ PPC64_R12_REGNUM,
+ PPC64_R13_REGNUM,
+ PPC64_R14_REGNUM,
+ PPC64_R15_REGNUM,
+ PPC64_R16_REGNUM,
+ PPC64_R17_REGNUM,
+ PPC64_R18_REGNUM,
+ PPC64_R19_REGNUM,
+ PPC64_R20_REGNUM,
+ PPC64_R21_REGNUM,
+ PPC64_R22_REGNUM,
+ PPC64_R23_REGNUM,
+ PPC64_R24_REGNUM,
+ PPC64_R25_REGNUM,
+ PPC64_R26_REGNUM,
+ PPC64_R27_REGNUM,
+ PPC64_R28_REGNUM,
+ PPC64_R29_REGNUM,
+ PPC64_R30_REGNUM,
+ PPC64_R31_REGNUM,
+
+ PPC64_F0_REGNUM = 32,
+ PPC64_F1_REGNUM,
+ PPC64_F2_REGNUM,
+ PPC64_F3_REGNUM,
+ PPC64_F4_REGNUM,
+ PPC64_F5_REGNUM,
+ PPC64_F6_REGNUM,
+ PPC64_F7_REGNUM,
+ PPC64_F8_REGNUM,
+ PPC64_F9_REGNUM,
+ PPC64_F10_REGNUM,
+ PPC64_F11_REGNUM,
+ PPC64_F12_REGNUM,
+ PPC64_F13_REGNUM,
+ PPC64_F14_REGNUM,
+ PPC64_F15_REGNUM,
+ PPC64_F16_REGNUM,
+ PPC64_F17_REGNUM,
+ PPC64_F18_REGNUM,
+ PPC64_F19_REGNUM,
+ PPC64_F20_REGNUM,
+ PPC64_F21_REGNUM,
+ PPC64_F22_REGNUM,
+ PPC64_F23_REGNUM,
+ PPC64_F24_REGNUM,
+ PPC64_F25_REGNUM,
+ PPC64_F26_REGNUM,
+ PPC64_F27_REGNUM,
+ PPC64_F28_REGNUM,
+ PPC64_F29_REGNUM,
+ PPC64_F30_REGNUM,
+ PPC64_F31_REGNUM,
+
+ PPC64_PC_REGNUM = 64,
+ PPC64_MSR_REGNUM = 65,
+ PPC64_CR_REGNUM = 66,
+ PPC64_LR_REGNUM = 67,
+ PPC64_CTR_REGNUM = 68,
+ PPC64_XER_REGNUM = 69,
+ PPC64_FPSCR_REGNUM = 70,
+
+ PPC64_VR0_REGNUM = 106,
+ PPC64_VR1_REGNUM,
+ PPC64_VR2_REGNUM,
+ PPC64_VR3_REGNUM,
+ PPC64_VR4_REGNUM,
+ PPC64_VR5_REGNUM,
+ PPC64_VR6_REGNUM,
+ PPC64_VR7_REGNUM,
+ PPC64_VR8_REGNUM,
+ PPC64_VR9_REGNUM,
+ PPC64_VR10_REGNUM,
+ PPC64_VR11_REGNUM,
+ PPC64_VR12_REGNUM,
+ PPC64_VR13_REGNUM,
+ PPC64_VR14_REGNUM,
+ PPC64_VR15_REGNUM,
+ PPC64_VR16_REGNUM,
+ PPC64_VR17_REGNUM,
+ PPC64_VR18_REGNUM,
+ PPC64_VR19_REGNUM,
+ PPC64_VR20_REGNUM,
+ PPC64_VR21_REGNUM,
+ PPC64_VR22_REGNUM,
+ PPC64_VR23_REGNUM,
+ PPC64_VR24_REGNUM,
+ PPC64_VR25_REGNUM,
+ PPC64_VR26_REGNUM,
+ PPC64_VR27_REGNUM,
+ PPC64_VR28_REGNUM,
+ PPC64_VR29_REGNUM,
+ PPC64_VR30_REGNUM,
+ PPC64_VR31_REGNUM,
+
+ PPC64_VSCR_REGNUM = 138,
+ PPC64_VRSAVE_REGNU = 139
+};
+
#endif /* !GDB_COMMON */
diff --git a/kernel.c b/kernel.c
index 1728b70c1b5c..661ef0e237eb 100644
--- a/kernel.c
+++ b/kernel.c
@@ -3533,6 +3533,39 @@ get_lkcd_regs(struct bt_info *bt, ulong *eip, ulong *esp)
machdep->get_stack_frame(bt, eip, esp);
}
+void
+get_dumpfile_regs(struct bt_info *bt, ulong *eip, ulong *esp)
+{
+ bt->flags |= BT_NO_PRINT_REGS;
+
+ if (NETDUMP_DUMPFILE())
+ get_netdump_regs(bt, eip, esp);
+ else if (KDUMP_DUMPFILE())
+ get_kdump_regs(bt, eip, esp);
+ else if (DISKDUMP_DUMPFILE())
+ get_diskdump_regs(bt, eip, esp);
+ else if (KVMDUMP_DUMPFILE())
+ get_kvmdump_regs(bt, eip, esp);
+ else if (LKCD_DUMPFILE())
+ get_lkcd_regs(bt, eip, esp);
+ else if (XENDUMP_DUMPFILE())
+ get_xendump_regs(bt, eip, esp);
+ else if (SADUMP_DUMPFILE())
+ get_sadump_regs(bt, eip, esp);
+ else if (VMSS_DUMPFILE())
+ get_vmware_vmss_regs(bt, eip, esp);
+ else if (REMOTE_PAUSED()) {
+ if (!is_task_active(bt->task) || !get_remote_regs(bt, eip, esp))
+ machdep->get_stack_frame(bt, eip, esp);
+ } else
+ machdep->get_stack_frame(bt, eip, esp);
+
+ bt->flags &= ~BT_NO_PRINT_REGS;
+
+ bt->instptr = *eip;
+ bt->stkptr = *esp;
+}
+
/*
* Store the head of the kernel module list for future use.
diff --git a/ppc64.c b/ppc64.c
index e8930a139e0d..870b0fbb49ce 100644
--- a/ppc64.c
+++ b/ppc64.c
@@ -55,6 +55,8 @@ 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,
+ void *value);
static void parse_cmdline_args(void);
static int ppc64_paca_percpu_offset_init(int);
static void ppc64_init_cpu_info(void);
@@ -704,6 +706,8 @@ ppc64_init(int when)
error(FATAL, "cannot malloc hwirqstack buffer space.");
}
+ machdep->get_cpu_reg = ppc64_get_cpu_reg;
+
ppc64_init_paca_info();
if (!machdep->hz) {
@@ -2501,6 +2505,105 @@ ppc64_print_eframe(char *efrm_str, struct ppc64_pt_regs *regs,
ppc64_print_nip_lr(regs, 1);
}
+static int
+ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
+ void *value)
+{
+ struct bt_info bt_info, bt_setup;
+ struct task_context *tc;
+ ulong task;
+ struct ppc64_pt_regs *pt_regs;
+ ulong ip, sp;
+
+ if (LIVE()) {
+ /* doesn't support reading registers in live dump */
+ return FALSE;
+ }
+
+ /* Currently only handling registers available in ppc64_pt_regs:
+ *
+ * 0-31: r0-r31
+ * 64: pc/nip
+ * 65: msr
+ *
+ * 67: lr
+ * 68: ctr
+ */
+ switch (regno) {
+ case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
+
+ case PPC64_PC_REGNUM:
+ case PPC64_MSR_REGNUM:
+ case PPC64_LR_REGNUM:
+ case PPC64_CTR_REGNUM:
+ break;
+
+ default:
+ // return false if we can't get that register
+ if (CRASHDEBUG(1))
+ error(WARNING, "unsupported register, regno=%d\n", regno);
+ return FALSE;
+ }
+
+ task = get_active_task(cpu);
+ tc = task_to_context(task);
+ 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);
+ pt_regs = (struct ppc64_pt_regs *)bt_info.machdep;
+
+ if (!pt_regs) {
+ error(WARNING, "pt_regs not available for cpu %d\n", cpu);
+ return FALSE;
+ }
+
+ switch (regno) {
+ case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
+ if (size != sizeof(pt_regs->gpr[regno]))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->gpr[regno], size);
+ break;
+
+ case PPC64_PC_REGNUM:
+ if (size != sizeof(pt_regs->nip))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->nip, size);
+ break;
+
+ case PPC64_MSR_REGNUM:
+ if (size != sizeof(pt_regs->msr))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->msr, size);
+ break;
+
+ case PPC64_LR_REGNUM:
+ if (size != sizeof(pt_regs->link))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->link, size);
+ break;
+
+ case PPC64_CTR_REGNUM:
+ if (size != sizeof(pt_regs->ctr))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->ctr, size);
+ break;
+ }
+
+ /* free buffer allocated by fill_stackbuf */
+ if (bt_info.stackbuf)
+ FREEBUF(bt_info.stackbuf);
+
+ return TRUE;
+}
+
/*
* For vmcore typically saved with KDump or FADump, get SP and IP values
* from the saved ptregs.
@@ -2613,9 +2716,11 @@ ppc64_get_dumpfile_stack_frame(struct bt_info *bt_in, ulong *nip,
ulong *ksp)
pt_regs = (struct ppc64_pt_regs *)bt->machdep;
ur_nip = pt_regs->nip;
ur_ksp = pt_regs->gpr[1];
- /* Print the collected regs for panic task. */
- ppc64_print_regs(pt_regs);
- ppc64_print_nip_lr(pt_regs, 1);
+ if (!(bt->flags & BT_NO_PRINT_REGS)) {
+ /* Print the collected regs for panic task. */
+ ppc64_print_regs(pt_regs);
+ ppc64_print_nip_lr(pt_regs, 1);
+ }
} else if ((pc->flags & KDUMP) ||
((pc->flags & DISKDUMP) &&
(*diskdump_flags & KDUMP_CMPRS_LOCAL))) {
--
2.43.0
3:37 a.m.
New subject: [PATCH v9 1/6] ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
On Thu, Feb 22, 2024 at 10:52:56AM +0530, Aditya Gupta wrote:
Currently, gdb passthroughs of 'bt', 'frame',
'up', 'down', 'info
locals' don't work. This is due to gdb not knowing the register values to
unwind the stack frames
Every gdb passthrough goes through `gdb_interface`. And then, gdb expects
`crash_target::fetch_registers` to give it the register values, which is
dependent on `machdep->get_cpu_reg` to read the register values for
specific architecture.
----------------------------
gdb passthrough (eg. "bt") | |
crash -------------------------> | |
| gdb_interface |
| |
| |
| ---------------------- |
fetch_registers | | | |
crash_target<-------------------------+--| gdb | |
--------------------------+->| | |
Registers (SP,NIP, etc.)| | | |
| | | |
| ---------------------- |
----------------------------
Implement `machdep->get_cpu_reg` on PPC64, so that crash provides the
register values to gdb to unwind stack frames properly
With these changes, on powerpc, 'bt' command output in gdb mode, will look
like this:
gdb> bt
#0 0xc0000000002a53e8 in crash_setup_regs (oldregs=<optimized out>,
newregs=0xc00000000486f8d8) at ./arch/powerpc/include/asm/kexec.h:69
#1 __crash_kexec (regs=<optimized out>) at kernel/kexec_core.c:974
#2 0xc000000000168918 in panic (fmt=<optimized out>) at kernel/panic.c:358
#3 0xc000000000b735f8 in sysrq_handle_crash (key=<optimized out>) at
drivers/tty/sysrq.c:155
#4 0xc000000000b742cc in __handle_sysrq (key=key@entry=99,
check_mask=check_mask@entry=false) at drivers/tty/sysrq.c:602
#5 0xc000000000b7506c in write_sysrq_trigger (file=<optimized out>,
buf=<optimized out>, count=2, ppos=<optimized out>) at
drivers/tty/sysrq.c:1163
#6 0xc00000000069a7bc in pde_write (ppos=<optimized out>, count=<optimized
out>, buf=<optimized out>, file=<optimized out>, pde=0xc000000009ed3a80) at
fs/proc/inode.c:340
#7 proc_reg_write (file=<optimized out>, buf=<optimized out>,
count=<optimized out>, ppos=<optimized out>) at fs/proc/inode.c:352
#8 0xc0000000005b3bbc in vfs_write (file=file@entry=0xc00000009dda7d00,
buf=buf@entry=0xebcfc7c6040 <error: Cannot access memory at address 0xebcfc7c6040>,
count=count@entry=2, pos=pos@entry=0xc00000000486fda0) at fs/read_write.c:582
instead of earlier output without this patch:
gdb> bt
#0 <unavailable> in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
Also, 'get_dumpfile_regs' has been introduced to get registers from
multiple supported vmcore formats. Correspondingly a flag 'BT_NO_PRINT_REGS'
has been introduced to tell helper functions to get registers, to not
print registers with every call to backtrace in gdb.
Note: This feature to support GDB unwinding doesn't support live debugging
Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
---
defs.h | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
kernel.c | 33 +++++++++++++++
ppc64.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 263 insertions(+), 3 deletions(-)
diff --git a/defs.h b/defs.h
index 98650e8780bf..414f039b35f1 100644
--- a/defs.h
+++ b/defs.h
@@ -6095,6 +6095,7 @@ int load_module_symbols_helper(char *);
void unlink_module(struct load_module *);
int check_specified_module_tree(char *, char *);
int is_system_call(char *, ulong);
+void get_dumpfile_regs(struct bt_info*, ulong*, ulong*);
void generic_dump_irq(int);
void generic_get_irq_affinity(int);
void generic_show_interrupts(int, ulong *);
@@ -6194,6 +6195,7 @@ ulong cpu_map_addr(const char *type);
#define BT_REGS_NOT_FOUND (0x4000000000000ULL)
#define BT_OVERFLOW_STACK (0x8000000000000ULL)
#define BT_SKIP_IDLE (0x10000000000000ULL)
+#define BT_NO_PRINT_REGS (0x20000000000000ULL)
#define BT_SYMBOL_OFFSET (BT_SYMBOLIC_ARGS)
#define BT_REF_HEXVAL (0x1)
@@ -8050,4 +8052,124 @@ enum x86_64_regnum {
LAST_REGNUM
};
+/*
+ * Register numbers to make crash_target->fetch_registers()
+ * ---> machdep->get_cpu_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
+ * crash_target::fetch_registers in case of PPC64
+ */
+enum ppc64_regnum {
+ PPC64_R0_REGNUM = 0,
+ PPC64_R1_REGNUM,
+ PPC64_R2_REGNUM,
+ PPC64_R3_REGNUM,
+ PPC64_R4_REGNUM,
+ PPC64_R5_REGNUM,
+ PPC64_R6_REGNUM,
+ PPC64_R7_REGNUM,
+ PPC64_R8_REGNUM,
+ PPC64_R9_REGNUM,
+ PPC64_R10_REGNUM,
+ PPC64_R11_REGNUM,
+ PPC64_R12_REGNUM,
+ PPC64_R13_REGNUM,
+ PPC64_R14_REGNUM,
+ PPC64_R15_REGNUM,
+ PPC64_R16_REGNUM,
+ PPC64_R17_REGNUM,
+ PPC64_R18_REGNUM,
+ PPC64_R19_REGNUM,
+ PPC64_R20_REGNUM,
+ PPC64_R21_REGNUM,
+ PPC64_R22_REGNUM,
+ PPC64_R23_REGNUM,
+ PPC64_R24_REGNUM,
+ PPC64_R25_REGNUM,
+ PPC64_R26_REGNUM,
+ PPC64_R27_REGNUM,
+ PPC64_R28_REGNUM,
+ PPC64_R29_REGNUM,
+ PPC64_R30_REGNUM,
+ PPC64_R31_REGNUM,
+
+ PPC64_F0_REGNUM = 32,
+ PPC64_F1_REGNUM,
+ PPC64_F2_REGNUM,
+ PPC64_F3_REGNUM,
+ PPC64_F4_REGNUM,
+ PPC64_F5_REGNUM,
+ PPC64_F6_REGNUM,
+ PPC64_F7_REGNUM,
+ PPC64_F8_REGNUM,
+ PPC64_F9_REGNUM,
+ PPC64_F10_REGNUM,
+ PPC64_F11_REGNUM,
+ PPC64_F12_REGNUM,
+ PPC64_F13_REGNUM,
+ PPC64_F14_REGNUM,
+ PPC64_F15_REGNUM,
+ PPC64_F16_REGNUM,
+ PPC64_F17_REGNUM,
+ PPC64_F18_REGNUM,
+ PPC64_F19_REGNUM,
+ PPC64_F20_REGNUM,
+ PPC64_F21_REGNUM,
+ PPC64_F22_REGNUM,
+ PPC64_F23_REGNUM,
+ PPC64_F24_REGNUM,
+ PPC64_F25_REGNUM,
+ PPC64_F26_REGNUM,
+ PPC64_F27_REGNUM,
+ PPC64_F28_REGNUM,
+ PPC64_F29_REGNUM,
+ PPC64_F30_REGNUM,
+ PPC64_F31_REGNUM,
+
+ PPC64_PC_REGNUM = 64,
+ PPC64_MSR_REGNUM = 65,
+ PPC64_CR_REGNUM = 66,
+ PPC64_LR_REGNUM = 67,
+ PPC64_CTR_REGNUM = 68,
+ PPC64_XER_REGNUM = 69,
+ PPC64_FPSCR_REGNUM = 70,
+
+ PPC64_VR0_REGNUM = 106,
+ PPC64_VR1_REGNUM,
+ PPC64_VR2_REGNUM,
+ PPC64_VR3_REGNUM,
+ PPC64_VR4_REGNUM,
+ PPC64_VR5_REGNUM,
+ PPC64_VR6_REGNUM,
+ PPC64_VR7_REGNUM,
+ PPC64_VR8_REGNUM,
+ PPC64_VR9_REGNUM,
+ PPC64_VR10_REGNUM,
+ PPC64_VR11_REGNUM,
+ PPC64_VR12_REGNUM,
+ PPC64_VR13_REGNUM,
+ PPC64_VR14_REGNUM,
+ PPC64_VR15_REGNUM,
+ PPC64_VR16_REGNUM,
+ PPC64_VR17_REGNUM,
+ PPC64_VR18_REGNUM,
+ PPC64_VR19_REGNUM,
+ PPC64_VR20_REGNUM,
+ PPC64_VR21_REGNUM,
+ PPC64_VR22_REGNUM,
+ PPC64_VR23_REGNUM,
+ PPC64_VR24_REGNUM,
+ PPC64_VR25_REGNUM,
+ PPC64_VR26_REGNUM,
+ PPC64_VR27_REGNUM,
+ PPC64_VR28_REGNUM,
+ PPC64_VR29_REGNUM,
+ PPC64_VR30_REGNUM,
+ PPC64_VR31_REGNUM,
+
+ PPC64_VSCR_REGNUM = 138,
+ PPC64_VRSAVE_REGNU = 139
+};
+
#endif /* !GDB_COMMON */
diff --git a/kernel.c b/kernel.c
index 1728b70c1b5c..661ef0e237eb 100644
--- a/kernel.c
+++ b/kernel.c
@@ -3533,6 +3533,39 @@ get_lkcd_regs(struct bt_info *bt, ulong *eip, ulong *esp)
machdep->get_stack_frame(bt, eip, esp);
}
+void
+get_dumpfile_regs(struct bt_info *bt, ulong *eip, ulong *esp)
+{
+ bt->flags |= BT_NO_PRINT_REGS;
+
+ if (NETDUMP_DUMPFILE())
+ get_netdump_regs(bt, eip, esp);
+ else if (KDUMP_DUMPFILE())
+ get_kdump_regs(bt, eip, esp);
+ else if (DISKDUMP_DUMPFILE())
+ get_diskdump_regs(bt, eip, esp);
+ else if (KVMDUMP_DUMPFILE())
+ get_kvmdump_regs(bt, eip, esp);
+ else if (LKCD_DUMPFILE())
+ get_lkcd_regs(bt, eip, esp);
+ else if (XENDUMP_DUMPFILE())
+ get_xendump_regs(bt, eip, esp);
+ else if (SADUMP_DUMPFILE())
+ get_sadump_regs(bt, eip, esp);
+ else if (VMSS_DUMPFILE())
+ get_vmware_vmss_regs(bt, eip, esp);
+ else if (REMOTE_PAUSED()) {
+ if (!is_task_active(bt->task) || !get_remote_regs(bt, eip, esp))
+ machdep->get_stack_frame(bt, eip, esp);
+ } else
+ machdep->get_stack_frame(bt, eip, esp);
+
+ bt->flags &= ~BT_NO_PRINT_REGS;
+
+ bt->instptr = *eip;
+ bt->stkptr = *esp;
+}
+
/*
* Store the head of the kernel module list for future use.
diff --git a/ppc64.c b/ppc64.c
index e8930a139e0d..870b0fbb49ce 100644
--- a/ppc64.c
+++ b/ppc64.c
@@ -55,6 +55,8 @@ 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,
+ void *value);
static void parse_cmdline_args(void);
static int ppc64_paca_percpu_offset_init(int);
static void ppc64_init_cpu_info(void);
@@ -704,6 +706,8 @@ ppc64_init(int when)
error(FATAL, "cannot malloc hwirqstack buffer space.");
}
+ machdep->get_cpu_reg = ppc64_get_cpu_reg;
+
ppc64_init_paca_info();
if (!machdep->hz) {
@@ -2501,6 +2505,105 @@ ppc64_print_eframe(char *efrm_str, struct ppc64_pt_regs *regs,
ppc64_print_nip_lr(regs, 1);
}
+static int
+ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
+ void *value)
+{
+ struct bt_info bt_info, bt_setup;
+ struct task_context *tc;
+ ulong task;
+ struct ppc64_pt_regs *pt_regs;
+ ulong ip, sp;
+
+ if (LIVE()) {
+ /* doesn't support reading registers in live dump */
+ return FALSE;
+ }
What do you think if we remove the LIVE() check for v10? According to my test(with my
x86_64 patchsets), with ppc64 arbitrary task stack unwinding support, we can view the
stack unwinding for non-active tasks for live debug:
crash> sys
KERNEL: /usr/lib/debug/lib/modules/5.14.0-425.el9.ppc64le/vmlinux
DUMPFILE: /proc/kcore
CPUS: 8
DATE: Tue Feb 27 04:19:36 EST 2024
UPTIME: 01:11:56
LOAD AVERAGE: 0.26, 0.09, 1.70
TASKS: 196
NODENAME: ibm-p9z-26-lp13.virt.pnr.lab.eng.rdu2.redhat.com
RELEASE: 5.14.0-425.el9.ppc64le
VERSION: #1 SMP Wed Feb 21 15:29:04 EST 2024
MACHINE: ppc64le (2900 Mhz)
MEMORY: 8 GB
crash> set 1
PID: 1
COMMAND: "systemd"
TASK: c0000000035fc900 [THREAD_INFO: c0000000035fc900]
CPU: 1
STATE: TASK_INTERRUPTIBLE
crash> bt
PID: 1 TASK: c0000000035fc900 CPU: 1 COMMAND: "systemd"
#0 [c00000000369fa60] __schedule at c000000000fc3c58
#1 [c00000000369fb20] schedule at c000000000fc411c
#2 [c00000000369fb50] schedule_hrtimeout_range_clock at c000000000fcd2a4
#3 [c00000000369fc00] ep_poll at c00000000063640c
#4 [c00000000369fcf0] do_epoll_wait at c000000000636584
#5 [c00000000369fd40] sys_epoll_wait at c000000000636608
#6 [c00000000369fdb0] system_call_exception at c00000000002e994
#7 [c00000000369fe10] system_call_vectored_common at c00000000000bfe8
crash> gdb bt
#0 0xc000000000fc3c58 in context_switch (rf=0xc00000000369fac0, next=0x0, prev=0x0,
rq=0x0) at kernel/sched/core.c:5409
#1 __schedule (sched_mode=sched_mode@entry=0) at kernel/sched/core.c:6737
#2 0xc000000000fc411c in schedule_loop (sched_mode=<optimized out>) at
kernel/sched/core.c:6807
#3 schedule () at kernel/sched/core.c:6816
#4 0xc000000000fcd2a4 in schedule_hrtimeout_range_clock (expires=<optimized out>,
delta=<optimized out>, mode=<optimized out>, clock_id=<optimized out>)
at kernel/time/hrtimer.c:2297
#5 0xc00000000063640c in ep_poll (ep=0xc000000034b32e80, events=0x0,
events@entry=0x1001d23a9e0, maxevents=maxevents@entry=82, timeout=timeout@entry=0x0) at
fs/eventpoll.c:1904
#6 0xc000000000636584 in do_epoll_wait (epfd=epfd@entry=4,
events=events@entry=0x1001d23a9e0, maxevents=maxevents@entry=82, to=0x0) at
fs/eventpoll.c:2288
#7 0xc000000000636608 in __do_sys_epoll_wait (timeout=<optimized out>,
maxevents=82, events=0x1001d23a9e0, epfd=4) at fs/eventpoll.c:2300
#8 __se_sys_epoll_wait (epfd=4, events=1100000504288, maxevents=82, timeout=<optimized
out>) at fs/eventpoll.c:2295
#9 0xc00000000002e994 in system_call_exception (r3=4, r4=1100000504288, r5=82, r6=-1,
r7=140735513318112, r8=140735513287592, r0=<optimized out>, regs=0xc00000000369fe80)
at arch/powerpc/kernel/interrupt.c:221
#10 0xc00000000000bfe8 in system_call_vectored_common () at
arch/powerpc/kernel/interrupt_64.S:209
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
However for those active tasks in live mode, stack unwind will fail, but I think it
doesn't matter because "bt" and "gdb bt" both cannot show stacks:
crash> ps
PID PPID CPU TASK ST %MEM VSZ RSS COMM
0 0 0 c000000002af6380 RU 0.0 0 0 [swapper/0]
0 0 1 c0000000035f9000 RU 0.0 0 0
[swapper/1]
0 0 2 c0000000035f0180 RU 0.0 0 0 [swapper/2]
0 0 3 c0000000035c5580 RU 0.0 0 0 [swapper/3]
0 0 4 c0000000035fac80 RU 0.0 0 0 [swapper/4]
0 0 5 c0000000035c7200 RU 0.0 0 0 [swapper/5]
0 0 6 c0000000035c0000 RU 0.0 0 0 [swapper/6]
0 0 7 c0000000035dc800 RU 0.0 0 0 [swapper/7]
1 0 1 c0000000035fc900 IN 0.2 182272 14016 systemd
crash> set c0000000035f0180
PID: 0
COMMAND: "swapper/2"
TASK: c0000000035f0180 (1 of 8) [THREAD_INFO: c0000000035f0180]
CPU: 2
STATE: TASK_RUNNING (ACTIVE)
crash> bt
PID: 0 TASK: c0000000035f0180 CPU: 2 COMMAND: "swapper/2"
(active)
crash> gdb bt
#0 0xc000000003847d50 in ?? ()
gdb: invalid kernel virtual address: fffffffffffffffc type: "gdb_readmem
callback"
gdb: invalid kernel virtual address: fffffffffffffff8 type: "gdb_readmem
callback"
gdb: invalid kernel virtual address: fffffffffffffffc type: "gdb_readmem
callback"
gdb: invalid kernel virtual address: fffffffffffffff8 type: "gdb_readmem
callback"
#1 0x0000000000000000 in ?? ()
What do you think?
Thanks,
Tao Liu
+
+ /* Currently only handling registers available in ppc64_pt_regs:
+ *
+ * 0-31: r0-r31
+ * 64: pc/nip
+ * 65: msr
+ *
+ * 67: lr
+ * 68: ctr
+ */
+ switch (regno) {
+ case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
+
+ case PPC64_PC_REGNUM:
+ case PPC64_MSR_REGNUM:
+ case PPC64_LR_REGNUM:
+ case PPC64_CTR_REGNUM:
+ break;
+
+ default:
+ // return false if we can't get that register
+ if (CRASHDEBUG(1))
+ error(WARNING, "unsupported register, regno=%d\n", regno);
+ return FALSE;
+ }
+
+ task = get_active_task(cpu);
+ tc = task_to_context(task);
+ 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);
+ pt_regs = (struct ppc64_pt_regs *)bt_info.machdep;
+
+ if (!pt_regs) {
+ error(WARNING, "pt_regs not available for cpu %d\n", cpu);
+ return FALSE;
+ }
+
+ switch (regno) {
+ case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
+ if (size != sizeof(pt_regs->gpr[regno]))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->gpr[regno], size);
+ break;
+
+ case PPC64_PC_REGNUM:
+ if (size != sizeof(pt_regs->nip))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->nip, size);
+ break;
+
+ case PPC64_MSR_REGNUM:
+ if (size != sizeof(pt_regs->msr))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->msr, size);
+ break;
+
+ case PPC64_LR_REGNUM:
+ if (size != sizeof(pt_regs->link))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->link, size);
+ break;
+
+ case PPC64_CTR_REGNUM:
+ if (size != sizeof(pt_regs->ctr))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->ctr, size);
+ break;
+ }
+
+ /* free buffer allocated by fill_stackbuf */
+ if (bt_info.stackbuf)
+ FREEBUF(bt_info.stackbuf);
+
+ return TRUE;
+}
+
/*
* For vmcore typically saved with KDump or FADump, get SP and IP values
* from the saved ptregs.
@@ -2613,9 +2716,11 @@ ppc64_get_dumpfile_stack_frame(struct bt_info *bt_in, ulong *nip,
ulong *ksp)
pt_regs = (struct ppc64_pt_regs *)bt->machdep;
ur_nip = pt_regs->nip;
ur_ksp = pt_regs->gpr[1];
- /* Print the collected regs for panic task. */
- ppc64_print_regs(pt_regs);
- ppc64_print_nip_lr(pt_regs, 1);
+ if (!(bt->flags & BT_NO_PRINT_REGS)) {
+ /* Print the collected regs for panic task. */
+ ppc64_print_regs(pt_regs);
+ ppc64_print_nip_lr(pt_regs, 1);
+ }
} else if ((pc->flags & KDUMP) ||
((pc->flags & DISKDUMP) &&
(*diskdump_flags & KDUMP_CMPRS_LOCAL))) {
--
2.43.0
4:29 a.m.
New subject: [PATCH v9 1/6] ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
Sorry Tao, I missed this mail earlier, as it got marked as read somehow.
On Tue, Feb 27, 2024 at 05:37:05PM +0800, Tao Liu wrote:
On Thu, Feb 22, 2024 at 10:52:56AM +0530, Aditya Gupta wrote:
>
> <...snip...>
>
> +static int
> +ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
> + void *value)
> +{
> + struct bt_info bt_info, bt_setup;
> + struct task_context *tc;
> + ulong task;
> + struct ppc64_pt_regs *pt_regs;
> + ulong ip, sp;
> +
> + if (LIVE()) {
> + /* doesn't support reading registers in live dump */
> + return FALSE;
> + }
What do you think if we remove the LIVE() check for v10? According to my test(with my
x86_64 patchsets), with ppc64 arbitrary task stack unwinding support, we can view the
stack unwinding for non-active tasks for live debug:
crash> sys
KERNEL: /usr/lib/debug/lib/modules/5.14.0-425.el9.ppc64le/vmlinux
DUMPFILE: /proc/kcore
CPUS: 8
DATE: Tue Feb 27 04:19:36 EST 2024
UPTIME: 01:11:56
LOAD AVERAGE: 0.26, 0.09, 1.70
TASKS: 196
NODENAME: ibm-p9z-26-lp13.virt.pnr.lab.eng.rdu2.redhat.com
RELEASE: 5.14.0-425.el9.ppc64le
VERSION: #1 SMP Wed Feb 21 15:29:04 EST 2024
MACHINE: ppc64le (2900 Mhz)
MEMORY: 8 GB
crash> set 1
PID: 1
COMMAND: "systemd"
TASK: c0000000035fc900 [THREAD_INFO: c0000000035fc900]
CPU: 1
STATE: TASK_INTERRUPTIBLE
crash> bt
PID: 1 TASK: c0000000035fc900 CPU: 1 COMMAND: "systemd"
#0 [c00000000369fa60] __schedule at c000000000fc3c58
#1 [c00000000369fb20] schedule at c000000000fc411c
#2 [c00000000369fb50] schedule_hrtimeout_range_clock at c000000000fcd2a4
#3 [c00000000369fc00] ep_poll at c00000000063640c
#4 [c00000000369fcf0] do_epoll_wait at c000000000636584
#5 [c00000000369fd40] sys_epoll_wait at c000000000636608
#6 [c00000000369fdb0] system_call_exception at c00000000002e994
#7 [c00000000369fe10] system_call_vectored_common at c00000000000bfe8
crash> gdb bt
#0 0xc000000000fc3c58 in context_switch (rf=0xc00000000369fac0, next=0x0, prev=0x0,
rq=0x0) at kernel/sched/core.c:5409
#1 __schedule (sched_mode=sched_mode@entry=0) at kernel/sched/core.c:6737
#2 0xc000000000fc411c in schedule_loop (sched_mode=<optimized out>) at
kernel/sched/core.c:6807
#3 schedule () at kernel/sched/core.c:6816
#4 0xc000000000fcd2a4 in schedule_hrtimeout_range_clock (expires=<optimized out>,
delta=<optimized out>, mode=<optimized out>, clock_id=<optimized out>)
at kernel/time/hrtimer.c:2297
#5 0xc00000000063640c in ep_poll (ep=0xc000000034b32e80, events=0x0,
events@entry=0x1001d23a9e0, maxevents=maxevents@entry=82, timeout=timeout@entry=0x0) at
fs/eventpoll.c:1904
#6 0xc000000000636584 in do_epoll_wait (epfd=epfd@entry=4,
events=events@entry=0x1001d23a9e0, maxevents=maxevents@entry=82, to=0x0) at
fs/eventpoll.c:2288
#7 0xc000000000636608 in __do_sys_epoll_wait (timeout=<optimized out>,
maxevents=82, events=0x1001d23a9e0, epfd=4) at fs/eventpoll.c:2300
#8 __se_sys_epoll_wait (epfd=4, events=1100000504288, maxevents=82,
timeout=<optimized out>) at fs/eventpoll.c:2295
#9 0xc00000000002e994 in system_call_exception (r3=4, r4=1100000504288, r5=82, r6=-1,
r7=140735513318112, r8=140735513287592, r0=<optimized out>, regs=0xc00000000369fe80)
at arch/powerpc/kernel/interrupt.c:221
#10 0xc00000000000bfe8 in system_call_vectored_common () at
arch/powerpc/kernel/interrupt_64.S:209
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
However for those active tasks in live mode, stack unwind will fail, but I think it
doesn't matter because "bt" and "gdb bt" both cannot show stacks:
Seems good. Currently, I marked it as 'not supported in live debug'
since we only add 1 thread for CPU 0, and don't add any gdb thread for
other CPUs. So, setting gdb context fails (as there is no thread).
I see in your patches you are adding gdb thread, if the thread is not
there. After that it should work.
Can you remove the LIVE() check along with your patches itself ?
Even if I remove it now, this series still doesn't support this feature
in live debug, and will introduce a silent bug, just showing same
backtrace for all CPUs in live debug (THREAD 1, CPU 0).
Thanks,
Aditya Gupta
>
> crash> ps
> PID PPID CPU TASK ST %MEM VSZ RSS COMM
> 0 0 0 c000000002af6380 RU 0.0 0 0 [swapper/0]
> > 0 0 1 c0000000035f9000 RU 0.0 0 0 [swapper/1]
> > 0 0 2 c0000000035f0180 RU 0.0 0 0 [swapper/2]
> > 0 0 3 c0000000035c5580 RU 0.0 0 0 [swapper/3]
> > 0 0 4 c0000000035fac80 RU 0.0 0 0 [swapper/4]
> > 0 0 5 c0000000035c7200 RU 0.0 0 0 [swapper/5]
> > 0 0 6 c0000000035c0000 RU 0.0 0 0 [swapper/6]
> > 0 0 7 c0000000035dc800 RU 0.0 0 0 [swapper/7]
> 1 0 1 c0000000035fc900 IN 0.2 182272 14016 systemd
> crash> set c0000000035f0180
> PID: 0
> COMMAND: "swapper/2"
> TASK: c0000000035f0180 (1 of 8) [THREAD_INFO: c0000000035f0180]
> CPU: 2
> STATE: TASK_RUNNING (ACTIVE)
> crash> bt
> PID: 0 TASK: c0000000035f0180 CPU: 2 COMMAND: "swapper/2"
> (active)
> crash> gdb bt
> #0 0xc000000003847d50 in ?? ()
> gdb: invalid kernel virtual address: fffffffffffffffc type: "gdb_readmem
callback"
> gdb: invalid kernel virtual address: fffffffffffffff8 type: "gdb_readmem
callback"
> gdb: invalid kernel virtual address: fffffffffffffffc type: "gdb_readmem
callback"
> gdb: invalid kernel virtual address: fffffffffffffff8 type: "gdb_readmem
callback"
> #1 0x0000000000000000 in ?? ()
>
> What do you think?
>
> Thanks,
> Tao Liu
> > +
> > + /* Currently only handling registers available in ppc64_pt_regs:
> > + *
> > + * 0-31: r0-r31
> > + * 64: pc/nip
> > + * 65: msr
> > + *
> > + * 67: lr
> > + * 68: ctr
> > + */
> > + switch (regno) {
> > + case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
> > +
> > + case PPC64_PC_REGNUM:
> > + case PPC64_MSR_REGNUM:
> > + case PPC64_LR_REGNUM:
> > + case PPC64_CTR_REGNUM:
> > + break;
> > +
> > + default:
> > + // return false if we can't get that register
> > + if (CRASHDEBUG(1))
> > + error(WARNING, "unsupported register, regno=%d\n", regno);
> > + return FALSE;
> > + }
> > +
> > + task = get_active_task(cpu);
> > + tc = task_to_context(task);
> > + 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);
> > + pt_regs = (struct ppc64_pt_regs *)bt_info.machdep;
> > +
> > + if (!pt_regs) {
> > + error(WARNING, "pt_regs not available for cpu %d\n", cpu);
> > + return FALSE;
> > + }
> > +
> > + switch (regno) {
> > + case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
> > + if (size != sizeof(pt_regs->gpr[regno]))
> > + return FALSE; // size mismatch
> > +
> > + memcpy(value, &pt_regs->gpr[regno], size);
> > + break;
> > +
> > + case PPC64_PC_REGNUM:
> > + if (size != sizeof(pt_regs->nip))
> > + return FALSE; // size mismatch
> > +
> > + memcpy(value, &pt_regs->nip, size);
> > + break;
> > +
> > + case PPC64_MSR_REGNUM:
> > + if (size != sizeof(pt_regs->msr))
> > + return FALSE; // size mismatch
> > +
> > + memcpy(value, &pt_regs->msr, size);
> > + break;
> > +
> > + case PPC64_LR_REGNUM:
> > + if (size != sizeof(pt_regs->link))
> > + return FALSE; // size mismatch
> > +
> > + memcpy(value, &pt_regs->link, size);
> > + break;
> > +
> > + case PPC64_CTR_REGNUM:
> > + if (size != sizeof(pt_regs->ctr))
> > + return FALSE; // size mismatch
> > +
> > + memcpy(value, &pt_regs->ctr, size);
> > + break;
> > + }
> > +
> > + /* free buffer allocated by fill_stackbuf */
> > + if (bt_info.stackbuf)
> > + FREEBUF(bt_info.stackbuf);
> > +
> > + return TRUE;
> > +}
> > +
> > /*
> > * For vmcore typically saved with KDump or FADump, get SP and IP values
> > * from the saved ptregs.
> > @@ -2613,9 +2716,11 @@ ppc64_get_dumpfile_stack_frame(struct bt_info *bt_in,
ulong *nip, ulong *ksp)
> > pt_regs = (struct ppc64_pt_regs *)bt->machdep;
> > ur_nip = pt_regs->nip;
> > ur_ksp = pt_regs->gpr[1];
> > - /* Print the collected regs for panic task. */
> > - ppc64_print_regs(pt_regs);
> > - ppc64_print_nip_lr(pt_regs, 1);
> > + if (!(bt->flags & BT_NO_PRINT_REGS)) {
> > + /* Print the collected regs for panic task. */
> > + ppc64_print_regs(pt_regs);
> > + ppc64_print_nip_lr(pt_regs, 1);
> > + }
> > } else if ((pc->flags & KDUMP) ||
> > ((pc->flags & DISKDUMP) &&
> > (*diskdump_flags & KDUMP_CMPRS_LOCAL))) {
> > --
> > 2.43.0
> >
>
5:39 a.m.
New subject: [PATCH v9 1/6] ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
Hi Aditya,
On Tue, Mar 5, 2024 at 6:30 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
Sorry Tao, I missed this mail earlier, as it got marked as read somehow.
On Tue, Feb 27, 2024 at 05:37:05PM +0800, Tao Liu wrote:
> On Thu, Feb 22, 2024 at 10:52:56AM +0530, Aditya Gupta wrote:
> >
> > <...snip...>
> >
> > +static int
> > +ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
> > + void *value)
> > +{
> > + struct bt_info bt_info, bt_setup;
> > + struct task_context *tc;
> > + ulong task;
> > + struct ppc64_pt_regs *pt_regs;
> > + ulong ip, sp;
> > +
> > + if (LIVE()) {
> > + /* doesn't support reading registers in live dump */
> > + return FALSE;
> > + }
>
> What do you think if we remove the LIVE() check for v10? According to my test(with
my x86_64 patchsets), with ppc64 arbitrary task stack unwinding support, we can view the
stack unwinding for non-active tasks for live debug:
>
> crash> sys
> KERNEL: /usr/lib/debug/lib/modules/5.14.0-425.el9.ppc64le/vmlinux
> DUMPFILE: /proc/kcore
> CPUS: 8
> DATE: Tue Feb 27 04:19:36 EST 2024
> UPTIME: 01:11:56
> LOAD AVERAGE: 0.26, 0.09, 1.70
> TASKS: 196
> NODENAME: ibm-p9z-26-lp13.virt.pnr.lab.eng.rdu2.redhat.com
> RELEASE: 5.14.0-425.el9.ppc64le
> VERSION: #1 SMP Wed Feb 21 15:29:04 EST 2024
> MACHINE: ppc64le (2900 Mhz)
> MEMORY: 8 GB
> crash> set 1
> PID: 1
> COMMAND: "systemd"
> TASK: c0000000035fc900 [THREAD_INFO: c0000000035fc900]
> CPU: 1
> STATE: TASK_INTERRUPTIBLE
> crash> bt
> PID: 1 TASK: c0000000035fc900 CPU: 1 COMMAND: "systemd"
> #0 [c00000000369fa60] __schedule at c000000000fc3c58
> #1 [c00000000369fb20] schedule at c000000000fc411c
> #2 [c00000000369fb50] schedule_hrtimeout_range_clock at c000000000fcd2a4
> #3 [c00000000369fc00] ep_poll at c00000000063640c
> #4 [c00000000369fcf0] do_epoll_wait at c000000000636584
> #5 [c00000000369fd40] sys_epoll_wait at c000000000636608
> #6 [c00000000369fdb0] system_call_exception at c00000000002e994
> #7 [c00000000369fe10] system_call_vectored_common at c00000000000bfe8
> crash> gdb bt
> #0 0xc000000000fc3c58 in context_switch (rf=0xc00000000369fac0, next=0x0, prev=0x0,
rq=0x0) at kernel/sched/core.c:5409
> #1 __schedule (sched_mode=sched_mode@entry=0) at kernel/sched/core.c:6737
> #2 0xc000000000fc411c in schedule_loop (sched_mode=<optimized out>) at
kernel/sched/core.c:6807
> #3 schedule () at kernel/sched/core.c:6816
> #4 0xc000000000fcd2a4 in schedule_hrtimeout_range_clock (expires=<optimized
out>, delta=<optimized out>, mode=<optimized out>, clock_id=<optimized
out>) at kernel/time/hrtimer.c:2297
> #5 0xc00000000063640c in ep_poll (ep=0xc000000034b32e80, events=0x0,
events@entry=0x1001d23a9e0, maxevents=maxevents@entry=82, timeout=timeout@entry=0x0) at
fs/eventpoll.c:1904
> #6 0xc000000000636584 in do_epoll_wait (epfd=epfd@entry=4,
events=events@entry=0x1001d23a9e0, maxevents=maxevents@entry=82, to=0x0) at
fs/eventpoll.c:2288
> #7 0xc000000000636608 in __do_sys_epoll_wait (timeout=<optimized out>,
maxevents=82, events=0x1001d23a9e0, epfd=4) at fs/eventpoll.c:2300
> #8 __se_sys_epoll_wait (epfd=4, events=1100000504288, maxevents=82,
timeout=<optimized out>) at fs/eventpoll.c:2295
> #9 0xc00000000002e994 in system_call_exception (r3=4, r4=1100000504288, r5=82,
r6=-1, r7=140735513318112, r8=140735513287592, r0=<optimized out>,
regs=0xc00000000369fe80) at arch/powerpc/kernel/interrupt.c:221
> #10 0xc00000000000bfe8 in system_call_vectored_common () at
arch/powerpc/kernel/interrupt_64.S:209
> Backtrace stopped: previous frame inner to this frame (corrupt stack?)
>
> However for those active tasks in live mode, stack unwind will fail, but I think it
doesn't matter because "bt" and "gdb bt" both cannot show stacks:
Seems good. Currently, I marked it as 'not supported in live debug'
since we only add 1 thread for CPU 0, and don't add any gdb thread for
other CPUs. So, setting gdb context fails (as there is no thread).
I see in your patches you are adding gdb thread, if the thread is not
there. After that it should work.
Can you remove the LIVE() check along with your patches itself ?
Even if I remove it now, this series still doesn't support this feature
in live debug, and will introduce a silent bug, just showing same
backtrace for all CPUs in live debug (THREAD 1, CPU 0).
OK, seems reasonable. I will remove the LIVE() check in my patchset.
Thanks,
Tao Liu
Thanks,
Aditya Gupta
>
> crash> ps
> PID PPID CPU TASK ST %MEM VSZ RSS COMM
> 0 0 0 c000000002af6380 RU 0.0 0 0 [swapper/0]
> > 0 0 1 c0000000035f9000 RU 0.0 0 0 [swapper/1]
> > 0 0 2 c0000000035f0180 RU 0.0 0 0 [swapper/2]
> > 0 0 3 c0000000035c5580 RU 0.0 0 0 [swapper/3]
> > 0 0 4 c0000000035fac80 RU 0.0 0 0 [swapper/4]
> > 0 0 5 c0000000035c7200 RU 0.0 0 0 [swapper/5]
> > 0 0 6 c0000000035c0000 RU 0.0 0 0 [swapper/6]
> > 0 0 7 c0000000035dc800 RU 0.0 0 0 [swapper/7]
> 1 0 1 c0000000035fc900 IN 0.2 182272 14016 systemd
> crash> set c0000000035f0180
> PID: 0
> COMMAND: "swapper/2"
> TASK: c0000000035f0180 (1 of 8) [THREAD_INFO: c0000000035f0180]
> CPU: 2
> STATE: TASK_RUNNING (ACTIVE)
> crash> bt
> PID: 0 TASK: c0000000035f0180 CPU: 2 COMMAND: "swapper/2"
> (active)
> crash> gdb bt
> #0 0xc000000003847d50 in ?? ()
> gdb: invalid kernel virtual address: fffffffffffffffc type: "gdb_readmem
callback"
> gdb: invalid kernel virtual address: fffffffffffffff8 type: "gdb_readmem
callback"
> gdb: invalid kernel virtual address: fffffffffffffffc type: "gdb_readmem
callback"
> gdb: invalid kernel virtual address: fffffffffffffff8 type: "gdb_readmem
callback"
> #1 0x0000000000000000 in ?? ()
>
> What do you think?
>
> Thanks,
> Tao Liu
> > +
> > + /* Currently only handling registers available in ppc64_pt_regs:
> > + *
> > + * 0-31: r0-r31
> > + * 64: pc/nip
> > + * 65: msr
> > + *
> > + * 67: lr
> > + * 68: ctr
> > + */
> > + switch (regno) {
> > + case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
> > +
> > + case PPC64_PC_REGNUM:
> > + case PPC64_MSR_REGNUM:
> > + case PPC64_LR_REGNUM:
> > + case PPC64_CTR_REGNUM:
> > + break;
> > +
> > + default:
> > + // return false if we can't get that register
> > + if (CRASHDEBUG(1))
> > + error(WARNING, "unsupported register,
regno=%d\n", regno);
> > + return FALSE;
> > + }
> > +
> > + task = get_active_task(cpu);
> > + tc = task_to_context(task);
> > + 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);
> > + pt_regs = (struct ppc64_pt_regs *)bt_info.machdep;
> > +
> > + if (!pt_regs) {
> > + error(WARNING, "pt_regs not available for cpu %d\n",
cpu);
> > + return FALSE;
> > + }
> > +
> > + switch (regno) {
> > + case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
> > + if (size != sizeof(pt_regs->gpr[regno]))
> > + return FALSE; // size mismatch
> > +
> > + memcpy(value, &pt_regs->gpr[regno], size);
> > + break;
> > +
> > + case PPC64_PC_REGNUM:
> > + if (size != sizeof(pt_regs->nip))
> > + return FALSE; // size mismatch
> > +
> > + memcpy(value, &pt_regs->nip, size);
> > + break;
> > +
> > + case PPC64_MSR_REGNUM:
> > + if (size != sizeof(pt_regs->msr))
> > + return FALSE; // size mismatch
> > +
> > + memcpy(value, &pt_regs->msr, size);
> > + break;
> > +
> > + case PPC64_LR_REGNUM:
> > + if (size != sizeof(pt_regs->link))
> > + return FALSE; // size mismatch
> > +
> > + memcpy(value, &pt_regs->link, size);
> > + break;
> > +
> > + case PPC64_CTR_REGNUM:
> > + if (size != sizeof(pt_regs->ctr))
> > + return FALSE; // size mismatch
> > +
> > + memcpy(value, &pt_regs->ctr, size);
> > + break;
> > + }
> > +
> > + /* free buffer allocated by fill_stackbuf */
> > + if (bt_info.stackbuf)
> > + FREEBUF(bt_info.stackbuf);
> > +
> > + return TRUE;
> > +}
> > +
> > /*
> > * For vmcore typically saved with KDump or FADump, get SP and IP values
> > * from the saved ptregs.
> > @@ -2613,9 +2716,11 @@ ppc64_get_dumpfile_stack_frame(struct bt_info *bt_in,
ulong *nip, ulong *ksp)
> > pt_regs = (struct ppc64_pt_regs *)bt->machdep;
> > ur_nip = pt_regs->nip;
> > ur_ksp = pt_regs->gpr[1];
> > - /* Print the collected regs for panic task. */
> > - ppc64_print_regs(pt_regs);
> > - ppc64_print_nip_lr(pt_regs, 1);
> > + if (!(bt->flags & BT_NO_PRINT_REGS)) {
> > + /* Print the collected regs for panic task. */
> > + ppc64_print_regs(pt_regs);
> > + ppc64_print_nip_lr(pt_regs, 1);
> > + }
> > } else if ((pc->flags & KDUMP) ||
> > ((pc->flags & DISKDUMP) &&
> > (*diskdump_flags & KDUMP_CMPRS_LOCAL))) {
> > --
> > 2.43.0
> >
>
5:41 a.m.
New subject: [PATCH v9 1/6] ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
Hi Tao,
On Tue, Mar 05, 2024 at 07:39:00PM +0800, Tao Liu wrote:
Hi Aditya,
On Tue, Mar 5, 2024 at 6:30 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
>
> Sorry Tao, I missed this mail earlier, as it got marked as read somehow.
>
> On Tue, Feb 27, 2024 at 05:37:05PM +0800, Tao Liu wrote:
> > On Thu, Feb 22, 2024 at 10:52:56AM +0530, Aditya Gupta wrote:
> > >
> > > <...snip...>
> > >
> > > +static int
> > > +ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
> > > + void *value)
> > > +{
> > > + struct bt_info bt_info, bt_setup;
> > > + struct task_context *tc;
> > > + ulong task;
> > > + struct ppc64_pt_regs *pt_regs;
> > > + ulong ip, sp;
> > > +
> > > + if (LIVE()) {
> > > + /* doesn't support reading registers in live dump */
> > > + return FALSE;
> > > + }
> >
> > What do you think if we remove the LIVE() check for v10? According to my
test(with my x86_64 patchsets), with ppc64 arbitrary task stack unwinding support, we can
view the stack unwinding for non-active tasks for live debug:
> >
> > crash> sys
> > KERNEL: /usr/lib/debug/lib/modules/5.14.0-425.el9.ppc64le/vmlinux
> > DUMPFILE: /proc/kcore
> > CPUS: 8
> > DATE: Tue Feb 27 04:19:36 EST 2024
> > UPTIME: 01:11:56
> > LOAD AVERAGE: 0.26, 0.09, 1.70
> > TASKS: 196
> > NODENAME: ibm-p9z-26-lp13.virt.pnr.lab.eng.rdu2.redhat.com
> > RELEASE: 5.14.0-425.el9.ppc64le
> > VERSION: #1 SMP Wed Feb 21 15:29:04 EST 2024
> > MACHINE: ppc64le (2900 Mhz)
> > MEMORY: 8 GB
> > crash> set 1
> > PID: 1
> > COMMAND: "systemd"
> > TASK: c0000000035fc900 [THREAD_INFO: c0000000035fc900]
> > CPU: 1
> > STATE: TASK_INTERRUPTIBLE
> > crash> bt
> > PID: 1 TASK: c0000000035fc900 CPU: 1 COMMAND: "systemd"
> > #0 [c00000000369fa60] __schedule at c000000000fc3c58
> > #1 [c00000000369fb20] schedule at c000000000fc411c
> > #2 [c00000000369fb50] schedule_hrtimeout_range_clock at c000000000fcd2a4
> > #3 [c00000000369fc00] ep_poll at c00000000063640c
> > #4 [c00000000369fcf0] do_epoll_wait at c000000000636584
> > #5 [c00000000369fd40] sys_epoll_wait at c000000000636608
> > #6 [c00000000369fdb0] system_call_exception at c00000000002e994
> > #7 [c00000000369fe10] system_call_vectored_common at c00000000000bfe8
> > crash> gdb bt
> > #0 0xc000000000fc3c58 in context_switch (rf=0xc00000000369fac0, next=0x0,
prev=0x0, rq=0x0) at kernel/sched/core.c:5409
> > #1 __schedule (sched_mode=sched_mode@entry=0) at kernel/sched/core.c:6737
> > #2 0xc000000000fc411c in schedule_loop (sched_mode=<optimized out>) at
kernel/sched/core.c:6807
> > #3 schedule () at kernel/sched/core.c:6816
> > #4 0xc000000000fcd2a4 in schedule_hrtimeout_range_clock (expires=<optimized
out>, delta=<optimized out>, mode=<optimized out>, clock_id=<optimized
out>) at kernel/time/hrtimer.c:2297
> > #5 0xc00000000063640c in ep_poll (ep=0xc000000034b32e80, events=0x0,
events@entry=0x1001d23a9e0, maxevents=maxevents@entry=82, timeout=timeout@entry=0x0) at
fs/eventpoll.c:1904
> > #6 0xc000000000636584 in do_epoll_wait (epfd=epfd@entry=4,
events=events@entry=0x1001d23a9e0, maxevents=maxevents@entry=82, to=0x0) at
fs/eventpoll.c:2288
> > #7 0xc000000000636608 in __do_sys_epoll_wait (timeout=<optimized out>,
maxevents=82, events=0x1001d23a9e0, epfd=4) at fs/eventpoll.c:2300
> > #8 __se_sys_epoll_wait (epfd=4, events=1100000504288, maxevents=82,
timeout=<optimized out>) at fs/eventpoll.c:2295
> > #9 0xc00000000002e994 in system_call_exception (r3=4, r4=1100000504288, r5=82,
r6=-1, r7=140735513318112, r8=140735513287592, r0=<optimized out>,
regs=0xc00000000369fe80) at arch/powerpc/kernel/interrupt.c:221
> > #10 0xc00000000000bfe8 in system_call_vectored_common () at
arch/powerpc/kernel/interrupt_64.S:209
> > Backtrace stopped: previous frame inner to this frame (corrupt stack?)
> >
> > However for those active tasks in live mode, stack unwind will fail, but I
think it doesn't matter because "bt" and "gdb bt" both cannot show
stacks:
>
> Seems good. Currently, I marked it as 'not supported in live debug'
> since we only add 1 thread for CPU 0, and don't add any gdb thread for
> other CPUs. So, setting gdb context fails (as there is no thread).
>
> I see in your patches you are adding gdb thread, if the thread is not
> there. After that it should work.
>
> Can you remove the LIVE() check along with your patches itself ?
> Even if I remove it now, this series still doesn't support this feature
> in live debug, and will introduce a silent bug, just showing same
> backtrace for all CPUs in live debug (THREAD 1, CPU 0).
OK, seems reasonable. I will remove the LIVE() check in my patchset.
Sure, Thanks Tao !
- Aditya Gupta
>
> Thanks,
> Tao Liu
>
> >
> > Thanks,
> > Aditya Gupta
> >
> > >
> > > crash> ps
> > > PID PPID CPU TASK ST %MEM VSZ RSS COMM
> > > 0 0 0 c000000002af6380 RU 0.0 0 0
[swapper/0]
> > > > 0 0 1 c0000000035f9000 RU 0.0 0 0
[swapper/1]
> > > > 0 0 2 c0000000035f0180 RU 0.0 0 0
[swapper/2]
> > > > 0 0 3 c0000000035c5580 RU 0.0 0 0
[swapper/3]
> > > > 0 0 4 c0000000035fac80 RU 0.0 0 0
[swapper/4]
> > > > 0 0 5 c0000000035c7200 RU 0.0 0 0
[swapper/5]
> > > > 0 0 6 c0000000035c0000 RU 0.0 0 0
[swapper/6]
> > > > 0 0 7 c0000000035dc800 RU 0.0 0 0
[swapper/7]
> > > 1 0 1 c0000000035fc900 IN 0.2 182272 14016
systemd
> > > crash> set c0000000035f0180
> > > PID: 0
> > > COMMAND: "swapper/2"
> > > TASK: c0000000035f0180 (1 of 8) [THREAD_INFO: c0000000035f0180]
> > > CPU: 2
> > > STATE: TASK_RUNNING (ACTIVE)
> > > crash> bt
> > > PID: 0 TASK: c0000000035f0180 CPU: 2 COMMAND:
"swapper/2"
> > > (active)
> > > crash> gdb bt
> > > #0 0xc000000003847d50 in ?? ()
> > > gdb: invalid kernel virtual address: fffffffffffffffc type:
"gdb_readmem callback"
> > > gdb: invalid kernel virtual address: fffffffffffffff8 type:
"gdb_readmem callback"
> > > gdb: invalid kernel virtual address: fffffffffffffffc type:
"gdb_readmem callback"
> > > gdb: invalid kernel virtual address: fffffffffffffff8 type:
"gdb_readmem callback"
> > > #1 0x0000000000000000 in ?? ()
> > >
> > > What do you think?
> > >
> > > Thanks,
> > > Tao Liu
> > > > +
> > > > + /* Currently only handling registers available in ppc64_pt_regs:
> > > > + *
> > > > + * 0-31: r0-r31
> > > > + * 64: pc/nip
> > > > + * 65: msr
> > > > + *
> > > > + * 67: lr
> > > > + * 68: ctr
> > > > + */
> > > > + switch (regno) {
> > > > + case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
> > > > +
> > > > + case PPC64_PC_REGNUM:
> > > > + case PPC64_MSR_REGNUM:
> > > > + case PPC64_LR_REGNUM:
> > > > + case PPC64_CTR_REGNUM:
> > > > + break;
> > > > +
> > > > + default:
> > > > + // return false if we can't get that register
> > > > + if (CRASHDEBUG(1))
> > > > + error(WARNING, "unsupported register,
regno=%d\n", regno);
> > > > + return FALSE;
> > > > + }
> > > > +
> > > > + task = get_active_task(cpu);
> > > > + tc = task_to_context(task);
> > > > + 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);
> > > > + pt_regs = (struct ppc64_pt_regs *)bt_info.machdep;
> > > > +
> > > > + if (!pt_regs) {
> > > > + error(WARNING, "pt_regs not available for cpu
%d\n", cpu);
> > > > + return FALSE;
> > > > + }
> > > > +
> > > > + switch (regno) {
> > > > + case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
> > > > + if (size != sizeof(pt_regs->gpr[regno]))
> > > > + return FALSE; // size mismatch
> > > > +
> > > > + memcpy(value, &pt_regs->gpr[regno], size);
> > > > + break;
> > > > +
> > > > + case PPC64_PC_REGNUM:
> > > > + if (size != sizeof(pt_regs->nip))
> > > > + return FALSE; // size mismatch
> > > > +
> > > > + memcpy(value, &pt_regs->nip, size);
> > > > + break;
> > > > +
> > > > + case PPC64_MSR_REGNUM:
> > > > + if (size != sizeof(pt_regs->msr))
> > > > + return FALSE; // size mismatch
> > > > +
> > > > + memcpy(value, &pt_regs->msr, size);
> > > > + break;
> > > > +
> > > > + case PPC64_LR_REGNUM:
> > > > + if (size != sizeof(pt_regs->link))
> > > > + return FALSE; // size mismatch
> > > > +
> > > > + memcpy(value, &pt_regs->link, size);
> > > > + break;
> > > > +
> > > > + case PPC64_CTR_REGNUM:
> > > > + if (size != sizeof(pt_regs->ctr))
> > > > + return FALSE; // size mismatch
> > > > +
> > > > + memcpy(value, &pt_regs->ctr, size);
> > > > + break;
> > > > + }
> > > > +
> > > > + /* free buffer allocated by fill_stackbuf */
> > > > + if (bt_info.stackbuf)
> > > > + FREEBUF(bt_info.stackbuf);
> > > > +
> > > > + return TRUE;
> > > > +}
> > > > +
> > > > /*
> > > > * For vmcore typically saved with KDump or FADump, get SP and IP
values
> > > > * from the saved ptregs.
> > > > @@ -2613,9 +2716,11 @@ ppc64_get_dumpfile_stack_frame(struct bt_info
*bt_in, ulong *nip, ulong *ksp)
> > > > pt_regs = (struct ppc64_pt_regs *)bt->machdep;
> > > > ur_nip = pt_regs->nip;
> > > > ur_ksp = pt_regs->gpr[1];
> > > > - /* Print the collected regs for panic task. */
> > > > - ppc64_print_regs(pt_regs);
> > > > - ppc64_print_nip_lr(pt_regs, 1);
> > > > + if (!(bt->flags & BT_NO_PRINT_REGS)) {
> > > > + /* Print the collected regs for panic task. */
> > > > + ppc64_print_regs(pt_regs);
> > > > + ppc64_print_nip_lr(pt_regs, 1);
> > > > + }
> > > > } else if ((pc->flags & KDUMP) ||
> > > > ((pc->flags & DISKDUMP) &&
> > > > (*diskdump_flags & KDUMP_CMPRS_LOCAL))) {
> > > > --
> > > > 2.43.0
> > > >
> > >
> >
>
11:22 p.m.
New subject: [PATCH v9 2/6] remove 'frame' from prohibited commands list
Currently, 'frame' is prohibited on every architecture, even in gdb mode.
Since the passthrough is working on few architectures (eg. ppc64, and
vmss x86_64), so remove it from prohibited list, so that 'frame' command
can be used
This has an implication on all architectures, that 'frame' now outputs
this in default mode:
```
crash> frame
#0 <unavailable> in ?? ()
```
instead of the before prohibited message
```
crash> frame
crash: prohibited gdb command: frame
```
Though, 'frame' should work in gdb mode for architectures that implement
'machdep->get_cpu_reg'
Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
---
gdb_interface.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gdb_interface.c b/gdb_interface.c
index b14319c66147..8f99a0d7fd54 100644
--- a/gdb_interface.c
+++ b/gdb_interface.c
@@ -711,7 +711,7 @@ static char *prohibited_list[] = {
"watch", "rwatch", "awatch", "attach",
"continue", "c", "fg", "detach",
"finish", "handle", "interrupt", "jump",
"kill", "next", "nexti",
"signal", "step", "s", "stepi",
"target", "until", "delete",
- "clear", "disable", "enable", "condition",
"ignore", "frame", "catch",
+ "clear", "disable", "enable", "condition",
"ignore", "catch",
"tcatch", "return", "file", "exec-file",
"core-file", "symbol-file",
"load", "si", "ni", "shell", "sy",
NULL /* must be last */
--
2.43.0
11:22 p.m.
New subject: [PATCH v9 3/6] synchronise cpu context changes between crash/gdb
Currently, both crash context and gdb's current thread context were
pretty independent, and could be different, for example, crash commands
might be working on thread 6 (CPU 5), but GDB passthroughs will be
working on thread 2 (CPU 1).
This was not a problem earlier since interaction of crash and gdb was
not depending on current context for most part. But with more
interactions, this patch should solve any issues due to mismatch in
context in crash/gdb.
Synchronise 'thread' command in gdb with 'set -c' command in crash.
1. crash -> gdb synchronisation:
Everytime crash's context changes, a helper is called to switch to
the thread on that CPU in gdb. The function has been implemented in
crash_target.c, since gdb functions are accessible inside
'crash_target.c', and the thread ID to CPU ID mapping is also done
by the crash_target, during initially registering the threads with
gdb. With this implementation, GDB's default thread initially also
changes to the crashing thread, so a switch to crashing thread
manually isn't required anymore
2. gdb -> crash synchronisation:
gdb has been patched to call 'set_cpu' whenever user switches to any
thread.
Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
---
crash_target.c | 24 ++++++++++++++++++++++++
defs.h | 3 +++
gdb-10.2.patch | 30 ++++++++++++++++++++++++++++++
kernel.c | 8 +++++++-
task.c | 4 +++-
5 files changed, 67 insertions(+), 2 deletions(-)
diff --git a/crash_target.c b/crash_target.c
index 455480679741..a9d7eea8a80e 100644
--- a/crash_target.c
+++ b/crash_target.c
@@ -29,6 +29,8 @@ extern "C" int gdb_readmem_callback(unsigned long, void *,
int, int);
extern "C" int crash_get_nr_cpus(void);
extern "C" int crash_get_cpu_reg (int cpu, int regno, const char *regname,
int regsize, void *val);
+extern "C" int gdb_change_cpu_context (unsigned int cpu);
+extern "C" int set_cpu (int cpu);
/* The crash target. */
@@ -133,3 +135,25 @@ crash_target_init (void)
/* Now, set up the frame cache. */
reinit_frame_cache ();
}
+
+/*
+ * Change gdb's thread context to the thread on given CPU
+ **/
+extern "C" int
+gdb_change_cpu_context(unsigned int cpu)
+{
+ ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu);
+ inferior *inf = current_inferior ();
+ thread_info *tp = find_thread_ptid (inf, ptid);
+
+ if (tp == nullptr)
+ return FALSE;
+
+ /* Making sure that crash's context is same */
+ set_cpu(cpu);
+
+ /* Switch to the thread */
+ switch_to_thread(tp);
+ return TRUE;
+}
+
diff --git a/defs.h b/defs.h
index 414f039b35f1..98cc2ea58a46 100644
--- a/defs.h
+++ b/defs.h
@@ -8172,4 +8172,7 @@ enum ppc64_regnum {
PPC64_VRSAVE_REGNU = 139
};
+/* crash_target.c */
+extern int gdb_change_cpu_context (unsigned int cpu);
+
#endif /* !GDB_COMMON */
diff --git a/gdb-10.2.patch b/gdb-10.2.patch
index a7018a249118..677bd9b35882 100644
--- a/gdb-10.2.patch
+++ b/gdb-10.2.patch
@@ -10,6 +10,7 @@
tar xvzmf gdb-10.2.tar.gz \
gdb-10.2/gdb/symtab.c \
+ gdb-10.2/gdb/thread.c \
gdb-10.2/gdb/printcmd.c \
gdb-10.2/gdb/symfile.c \
gdb-10.2/gdb/Makefile.in \
@@ -16057,3 +16058,32 @@ exit 0
m10200-dis.c
m10200-opc.c
m10300-dis.c
+--- gdb-10.2/gdb/thread.c.orig
++++ gdb-10.2/gdb/thread.c
+@@ -58,6 +58,11 @@ static int highest_thread_num;
+ /* The current/selected thread. */
+ static thread_info *current_thread_;
+
++#ifdef CRASH_MERGE
++/* Function to set cpu, defined by crash-utility */
++extern "C" void set_cpu (int);
++#endif
++
+ /* RAII type used to increase / decrease the refcount of each thread
+ in a given list of threads. */
+
+@@ -1896,7 +1901,13 @@ thread_command (const char *tidstr, int from_tty)
+ {
+ ptid_t previous_ptid = inferior_ptid;
+
+- thread_select (tidstr, parse_thread_id (tidstr, NULL));
++ struct thread_info* thread_id = parse_thread_id (tidstr, NULL);
++
++#ifdef CRASH_MERGE
++ set_cpu(thread_id->ptid.tid());
++#endif
++
++ thread_select (tidstr, thread_id);
+
+ /* Print if the thread has not changed, otherwise an event will
+ be sent. */
diff --git a/kernel.c b/kernel.c
index 661ef0e237eb..93af36fc0667 100644
--- a/kernel.c
+++ b/kernel.c
@@ -6527,7 +6527,13 @@ set_cpu(int cpu)
if (hide_offline_cpu(cpu))
error(FATAL, "invalid cpu number: cpu %d is OFFLINE\n", cpu);
- if ((task = get_active_task(cpu)))
+ task = get_active_task(cpu);
+
+ /* Check if context is already set to given cpu */
+ if (task == CURRENT_TASK())
+ return;
+
+ if (task)
set_context(task, NO_PID);
else
error(FATAL, "cannot determine active task on cpu %ld\n", cpu);
diff --git a/task.c b/task.c
index ebdb5be3786f..3a190cafbacb 100644
--- a/task.c
+++ b/task.c
@@ -5301,7 +5301,9 @@ set_context(ulong task, ulong pid)
if (found) {
CURRENT_CONTEXT() = tc;
- return TRUE;
+
+ /* change the selected thread in gdb, according to current context */
+ return gdb_change_cpu_context(tc->processor);
} else {
if (task)
error(INFO, "cannot set context for task: %lx\n", task);
--
2.43.0
11:22 p.m.
New subject: [PATCH v9 4/6] fix gdb_interface: restore gdb's output streams at end of gdb_interface
Currently for most gdb_interface call with a non-null file pointer,
GDB's output stream is replaced with the passed file pointer
'info threads', which is a gdb passthrough, fails to print all thread
after support was added to get registers from crash_target:
Id Target Id Frame
1 CPU 0 <unavailable> in ?? ()
2 CPU 1
Also, it was noticed that 'info threads' will print all threads when
'info threads' is run 2nd and subsequent times.
This incomplete output of 'info threads' was due to a subtle bug in
gdb_interface. For any passthrough, it switches the gdb output and
stderr streams to another FILE* passed in `struct gnu_request`.
But it does not restore the original FILE* stream gdb was using.
For each thread, which does not have it's registers yet, gdb goes to
crash_target::fetch_registers to ask for registers. Which in turn might
call 'datatype_info', which causes gdb's output stream to be set to
'/dev/null'
So all output after the call to datatype_info will go to /dev/null, and
hence the data to be printed is lost.
When 'info threads' is run a second time, it sets GDB's output streams
to something which is NOT /dev/null, and since it already has registers
for each thread, it does not go to 'crash_target::fetch_registers' and
hence the output stream is consistent, and we get complete output
So what happened is this:
1. GDB prints thread 1 (CPU 0) correctly since it had it's registers
cached during initialisation in 'crash_target_init' (at that times
registers where still unavailable, so it shows unavailable)
2. At CPU 1, to print the frame, it goes to 'crash_target::fetch_registers'
to get the registers
3. crash_target then calls 'machdep->get_cpu_reg' which finally ends up
at 'get_netdump_regs' which uses 'datatype_info' to get info of
'elf_prstatus.pr_reg'
4. Inside 'datatype_info', we set the file stream to /dev/null
(pc->nullfp). Then it passes this request object to gdb_interface
5. gdb_interface replaces GDB's output stream to the passed stream, ie.
/dev/null
6. Now, ALL FUTURE WRITES EVEN BY GDB, SUCH AS FOR THE 'info threads'
COMMAND GOES TO THIS STREAM gnu_request->fp, WHICH is /dev/null,
until someone else sets the non-null stream later
7. Similar behaviour occurs with all other threads
8. And hence we lose all subsequent output by GDB also
When 'info threads' is run a second time:
1. 'gdb_pass_through' passes a default FILE* to 'gdb_interface', which
is not /dev/null
2. Since the registers are cached for all threads now, it does not go
back to 'crash_target' for registers, and hence no call is made to
'datatype_info', hence GDB's output stream is not changing
3. Since the gdb's stream, even though not what it was using originally,
it's still valid, and not '/dev/null', hence we get the complete output
Fix this by restoring the original output streams, after gdb_interface
has handled the output
Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
---
gdb-10.2.patch | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/gdb-10.2.patch b/gdb-10.2.patch
index 677bd9b35882..1fb421286551 100644
--- a/gdb-10.2.patch
+++ b/gdb-10.2.patch
@@ -16087,3 +16087,56 @@ exit 0
/* Print if the thread has not changed, otherwise an event will
be sent. */
+--- gdb-10.2/gdb/ui-file.h.orig
++++ gdb-10.2/gdb/ui-file.h
+@@ -195,6 +195,7 @@ public:
+
+ bool can_emit_style_escape () override;
+
++ FILE *get_stream(void);
+ /* Sets the internal stream to FILE, and saves the FILE's file
+ descriptor in M_FD. */
+ void set_stream (FILE *file);
+--- gdb-10.2/gdb/ui-file.c.orig
++++ gdb-10.2/gdb/ui-file.c
+@@ -161,6 +161,12 @@ stdio_file::~stdio_file ()
+ fclose (m_file);
+ }
+
++FILE*
++stdio_file::get_stream(void)
++{
++ return m_file;
++}
++
+ void
+ stdio_file::set_stream (FILE *file)
+ {
+--- gdb-10.2/gdb/symtab.c.orig
++++ gdb-10.2/gdb/symtab.c
+@@ -6964,8 +6964,12 @@ void
+ gdb_command_funnel_1(struct gnu_request *req)
+ {
+ struct symbol *sym;
++ FILE *original_stdout_stream = nullptr;
++ FILE *original_stderr_stream = nullptr;
+
+ if (req->command != GNU_VERSION && req->command !=
GNU_USER_PRINT_OPTION) {
++ original_stdout_stream = (dynamic_cast< stdio_file *
>gdb_stdout)->get_stream();
++ original_stderr_stream = (dynamic_cast< stdio_file *
>gdb_stderr)->get_stream();
+ (dynamic_cast<stdio_file
*>gdb_stdout)->set_stream(req->fp);
+ (dynamic_cast<stdio_file
*>gdb_stderr)->set_stream(req->fp);
+ }
+@@ -7068,6 +7072,12 @@ gdb_command_funnel_1(struct gnu_request *req)
+ req->flags |= GNU_COMMAND_FAILED;
+ break;
+ }
++
++ /* Restore the streams gdb output was using */
++ if (original_stdout_stream)
++ (dynamic_cast<stdio_file
*>gdb_stdout)->set_stream(original_stdout_stream);
++ if (original_stderr_stream)
++ (dynamic_cast<stdio_file
*>gdb_stderr)->set_stream(original_stderr_stream);
+ }
+
+ /*
--
2.43.0
11:23 p.m.
New subject: [PATCH v9 5/6] fix 'info threads' command
Currently the output is like this ppc64le:
crash> info threads
Id Target Id Frame
1 CPU 0 <unavailable> in ?? ()
2 CPU 1 0xc000000000025d00 in ppc_panic_event (this=<optimized
out>, event=<optimized out>, ptr=0x0) at arch/powerpc/kernel/setup-common.c:712
3 CPU 2 0xc000000000025d00 in ppc_panic_event (this=<optimized
out>, event=<optimized out>, ptr=0x0) at arch/powerpc/kernel/setup-common.c:712
4 CPU 3 0xc000000000025d00 in ppc_panic_event (this=<optimized
out>, event=<optimized out>, ptr=0x0) at arch/powerpc/kernel/setup-common.c:712
* 5 CPU 4 0xc000000000025d00 in ppc_panic_event (this=<optimized
out>, event=<optimized out>, ptr=0x0) at arch/powerpc/kernel/setup-common.c:712
6 CPU 5 0xc000000000025d00 in ppc_panic_event (this=<optimized
out>, event=<optimized out>, ptr=0x0) at arch/powerpc/kernel/setup-common.c:712
7 CPU 6 0xc000000000025d00 in ppc_panic_event (this=<optimized
out>, event=<optimized out>, ptr=0x0) at arch/powerpc/kernel/setup-common.c:712
8 CPU 7 0xc000000000025d00 in ppc_panic_event (this=<optimized
out>, event=<optimized out>, ptr=0x0) at arch/powerpc/kernel/setup-common.c:712
There are two issues here:
1. CPU 0 always shows '<unavailable>' frame:
This is due to 'thread 0' being initialised in 'crash_target_init',
but it's very early in crash init, so it could not get any registers
from the vmcore.
And, since GDB caches registers (whether it is there or now), it
keeps thinking that registers for this thread are unavailable, so all
future usage of thread 0 also shows unavailable registers & frame
Fix this by refreshing the register cache of all threads in
'gdb_refresh_regcache', at task_init
2. All other CPUs show the same frame:
For each thread, GDB depends on crash to give it registers, ie.
crash_target::fetch_registers, but this in turn depends on crash's
current context
GDB internally switches it's thread context internally to each thread
one by one, while printing the thread's frame, similarly switch
crash's context when the thread context is changed, so that it
matches the thread context in gdb (such as, CPU 3 in crash for thread
2(CPU 3) in gdb).
Also, since we are switching context multiple times, add an argument
to 'set_cpu' to not print the context everytime
With these fixed, it will show all threads, as well as correct registers
and frame. It might still show similar frame for many threads, due to
all of them being on same NMI exception crash path, but if checked with
'info registers', they will have different registers.
Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
---
crash_target.c | 37 ++++++++++++++++++++++++++++++++--
defs.h | 5 +++--
gdb-10.2.patch | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
kernel.c | 8 +++++---
task.c | 30 ++++++++++++++++++++--------
tools.c | 10 +++++-----
6 files changed, 124 insertions(+), 20 deletions(-)
diff --git a/crash_target.c b/crash_target.c
index a9d7eea8a80e..d06383f594aa 100644
--- a/crash_target.c
+++ b/crash_target.c
@@ -30,7 +30,8 @@ extern "C" int crash_get_nr_cpus(void);
extern "C" int crash_get_cpu_reg (int cpu, int regno, const char *regname,
int regsize, void *val);
extern "C" int gdb_change_cpu_context (unsigned int cpu);
-extern "C" int set_cpu (int cpu);
+extern "C" void gdb_refresh_regcache(unsigned int cpu);
+extern "C" int set_cpu(int cpu, int print_context);
/* The crash target. */
@@ -150,10 +151,42 @@ gdb_change_cpu_context(unsigned int cpu)
return FALSE;
/* Making sure that crash's context is same */
- set_cpu(cpu);
+ set_cpu(cpu, FALSE);
/* Switch to the thread */
switch_to_thread(tp);
+
+ /* Fetch/Refresh thread's registers */
+ gdb_refresh_regcache(cpu);
+
return TRUE;
}
+/* Refresh regcache of gdb thread on given CPU
+ *
+ * When gdb threads were initially added by 'crash_target_init', crash was not
+ * yet initialised, and hence crash_target::fetch_registers didn't give any
+ * registers to gdb.
+ *
+ * This is meant to be called after tasks in crash have been initialised, and
+ * possible machdep->get_cpu_reg is also set so architecture can give registers
+ */
+extern "C" void
+gdb_refresh_regcache(unsigned int cpu)
+{
+ int saved_cpu = inferior_thread()->ptid.tid();
+ ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu);
+ inferior *inf = current_inferior();
+ thread_info *tp = find_thread_ptid (inf, ptid);
+
+ if (tp == NULL) {
+ warning("gdb thread for cpu %d not found\n", cpu);
+ return;
+ }
+
+ /* temporarily switch to the cpu so we get correct registers */
+ set_cpu(cpu, FALSE);
+ target_fetch_registers(get_thread_regcache(tp), -1);
+
+ set_cpu(saved_cpu, FALSE);
+}
diff --git a/defs.h b/defs.h
index 98cc2ea58a46..2a6a6ab9f510 100644
--- a/defs.h
+++ b/defs.h
@@ -6012,7 +6012,7 @@ extern char *help_map[];
* task.c
*/
void task_init(void);
-int set_context(ulong, ulong);
+int set_context(ulong, ulong, uint);
void show_context(struct task_context *);
ulong pid_to_task(ulong);
ulong task_to_pid(ulong);
@@ -6117,7 +6117,7 @@ void parse_kernel_version(char *);
#define SHOW_LOG_CTIME (0x10)
#define SHOW_LOG_SAFE (0x20)
#define SHOW_LOG_CALLER (0x40)
-void set_cpu(int);
+void set_cpu(int cpu, int print_context);
void clear_machdep_cache(void);
struct stack_hook *gather_text_list(struct bt_info *);
int get_cpus_online(void);
@@ -8174,5 +8174,6 @@ enum ppc64_regnum {
/* crash_target.c */
extern int gdb_change_cpu_context (unsigned int cpu);
+extern void gdb_refresh_regcache (unsigned int cpu);
#endif /* !GDB_COMMON */
diff --git a/gdb-10.2.patch b/gdb-10.2.patch
index 1fb421286551..138413fa8a9c 100644
--- a/gdb-10.2.patch
+++ b/gdb-10.2.patch
@@ -16140,3 +16140,57 @@ exit 0
}
/*
+--- gdb-10.2/gdb/thread.c.orig
++++ gdb-10.2/gdb/thread.c
+@@ -60,7 +60,7 @@ static thread_info *current_thread_;
+
+ #ifdef CRASH_MERGE
+ /* Function to set cpu, defined by crash-utility */
+-extern "C" void set_cpu (int);
++extern "C" void set_cpu(int cpu, int print_context);
+ #endif
+
+ /* RAII type used to increase / decrease the refcount of each thread
+@@ -1098,6 +1098,9 @@ print_thread_info_1 (struct ui_out *uiout, const char
*requested_threads,
+ uiout->table_body ();
+ }
+
++#ifdef CRASH_MERGE
++ int current_cpu = current_thread ? current_thread->ptid.tid() : 0;
++#endif
+ for (inferior *inf : all_inferiors ())
+ for (thread_info *tp : inf->threads ())
+ {
+@@ -1113,6 +1116,11 @@ print_thread_info_1 (struct ui_out *uiout, const char
*requested_threads,
+
+ ui_out_emit_tuple tuple_emitter (uiout, NULL);
+
++ /* Switch to the thread's CPU in crash */
++#ifdef CRASH_MERGE
++ set_cpu(tp->ptid.tid(), FALSE);
++#endif
++
+ if (!uiout->is_mi_like_p ())
+ {
+ if (tp == current_thread)
+@@ -1185,6 +1193,11 @@ print_thread_info_1 (struct ui_out *uiout, const char
*requested_threads,
+ /* This end scope restores the current thread and the frame
+ selected before the "info threads" command, and it finishes the
+ ui-out list or table. */
++
++#ifdef CRASH_MERGE
++ /* Restore crash's context */
++ set_cpu(current_cpu, FALSE);
++#endif
+ }
+
+ if (pid == -1 && requested_threads == NULL)
+@@ -1904,7 +1917,7 @@ thread_command (const char *tidstr, int from_tty)
+ struct thread_info* thread_id = parse_thread_id (tidstr, NULL);
+
+ #ifdef CRASH_MERGE
+- set_cpu(thread_id->ptid.tid());
++ set_cpu(thread_id->ptid.tid(), FALSE);
+ #endif
+
+ thread_select (tidstr, thread_id);
diff --git a/kernel.c b/kernel.c
index 93af36fc0667..c7de73a3f615 100644
--- a/kernel.c
+++ b/kernel.c
@@ -6514,9 +6514,10 @@ no_cpu_flags:
/*
* Set the context to the active task on a given cpu -- dumpfiles only.
+ * Optionally show the context if print_context is TRUE
*/
void
-set_cpu(int cpu)
+set_cpu(int cpu, int print_context)
{
ulong task;
@@ -6534,11 +6535,12 @@ set_cpu(int cpu)
return;
if (task)
- set_context(task, NO_PID);
+ set_context(task, NO_PID, TRUE);
else
error(FATAL, "cannot determine active task on cpu %ld\n", cpu);
- show_context(CURRENT_CONTEXT());
+ if (print_context)
+ show_context(CURRENT_CONTEXT());
}
diff --git a/task.c b/task.c
index 3a190cafbacb..a405b05a47d1 100644
--- a/task.c
+++ b/task.c
@@ -672,7 +672,7 @@ task_init(void)
if (ACTIVE()) {
active_pid = REMOTE() ? pc->server_pid :
LOCAL_ACTIVE() ? pc->program_pid : 1;
- set_context(NO_TASK, active_pid);
+ set_context(NO_TASK, active_pid, FALSE);
tt->this_task = pid_to_task(active_pid);
}
else {
@@ -684,7 +684,7 @@ task_init(void)
else if (ELF_NOTES_VALID() && DISKDUMP_DUMPFILE())
map_cpus_to_prstatus_kdump_cmprs();
please_wait("determining panic task");
- set_context(get_panic_context(), NO_PID);
+ set_context(get_panic_context(), NO_PID, TRUE);
please_wait_done();
}
@@ -706,6 +706,17 @@ task_init(void)
kt->boot_date.tv_nsec = 0;
}
+ /*
+ * Refresh CPU 0's thread's regcache
+ *
+ * This is required since, it's registers were initialised in
+ * crash_target_init when crash was not initialised yet and hence could
+ * not pass registers to gdb when gdb requests via
+ * crash_target::fetch_registers, so CPU 0's registers are shown as
+ * <unavailable> in gdb mode
+ * */
+ gdb_refresh_regcache(0);
+
tt->flags |= TASK_INIT_DONE;
}
@@ -2985,9 +2996,9 @@ refresh_context(ulong curtask, ulong curpid)
struct task_context *tc;
if (task_exists(curtask) && pid_exists(curpid)) {
- set_context(curtask, NO_PID);
+ set_context(curtask, NO_PID, FALSE);
} else {
- set_context(tt->this_task, NO_PID);
+ set_context(tt->this_task, NO_PID, FALSE);
complain = TRUE;
if (STREQ(args[0], "set") && (argcnt == 2) &&
@@ -3053,7 +3064,7 @@ sort_context_array(void)
curtask = CURRENT_TASK();
qsort((void *)tt->context_array, (size_t)tt->running_tasks,
sizeof(struct task_context), sort_by_pid);
- set_context(curtask, NO_PID);
+ set_context(curtask, NO_PID, FALSE);
sort_context_by_task();
}
@@ -3100,7 +3111,7 @@ sort_context_array_by_last_run(void)
curtask = CURRENT_TASK();
qsort((void *)tt->context_array, (size_t)tt->running_tasks,
sizeof(struct task_context), sort_by_last_run);
- set_context(curtask, NO_PID);
+ set_context(curtask, NO_PID, FALSE);
sort_context_by_task();
}
@@ -5281,7 +5292,7 @@ comm_exists(char *s)
* that pid is selected.
*/
int
-set_context(ulong task, ulong pid)
+set_context(ulong task, ulong pid, uint update_gdb_thread)
{
int i;
struct task_context *tc;
@@ -5303,7 +5314,10 @@ set_context(ulong task, ulong pid)
CURRENT_CONTEXT() = tc;
/* change the selected thread in gdb, according to current context */
- return gdb_change_cpu_context(tc->processor);
+ if (update_gdb_thread)
+ return gdb_change_cpu_context(tc->processor);
+ else
+ return TRUE;
} else {
if (task)
error(INFO, "cannot set context for task: %lx\n", task);
diff --git a/tools.c b/tools.c
index 0f2db108838a..18d088ddb890 100644
--- a/tools.c
+++ b/tools.c
@@ -1860,7 +1860,7 @@ cmd_set(void)
break;
}
cpu = dtoi(optarg, FAULT_ON_ERROR, NULL);
- set_cpu(cpu);
+ set_cpu(cpu, TRUE);
return;
case 'p':
@@ -1871,7 +1871,7 @@ cmd_set(void)
return;
if (ACTIVE()) {
- set_context(tt->this_task, NO_PID);
+ set_context(tt->this_task, NO_PID, TRUE);
show_context(CURRENT_CONTEXT());
return;
}
@@ -1880,7 +1880,7 @@ cmd_set(void)
error(INFO, "no panic task found!\n");
return;
}
- set_context(tt->panic_task, NO_PID);
+ set_context(tt->panic_task, NO_PID, TRUE);
show_context(CURRENT_CONTEXT());
return;
@@ -2559,14 +2559,14 @@ cmd_set(void)
case STR_PID:
pid = value;
task = NO_TASK;
- if (set_context(task, pid))
+ if (set_context(task, pid, TRUE))
show_context(CURRENT_CONTEXT());
break;
case STR_TASK:
task = value;
pid = NO_PID;
- if (set_context(task, pid))
+ if (set_context(task, pid, TRUE))
show_context(CURRENT_CONTEXT());
break;
--
2.43.0
11:23 p.m.
New subject: [PATCH v9 6/6] ppc64 arbitrary task stack unwind support
From: Tao Liu <ltao(a)redhat.com>
Signed-off-by: Tao Liu <ltao(a)redhat.com>
Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
---
ppc64.c | 74 +++++++++++++++++++++++++++++++++------------------------
1 file changed, 43 insertions(+), 31 deletions(-)
diff --git a/ppc64.c b/ppc64.c
index 870b0fbb49ce..f86d951ddefe 100644
--- a/ppc64.c
+++ b/ppc64.c
@@ -2514,6 +2514,7 @@ ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
ulong task;
struct ppc64_pt_regs *pt_regs;
ulong ip, sp;
+ bool ret = FALSE;
if (LIVE()) {
/* doesn't support reading registers in live dump */
@@ -2545,14 +2546,17 @@ ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
return FALSE;
}
- task = get_active_task(cpu);
- tc = task_to_context(task);
+ tc = CURRENT_CONTEXT();
+ if (!tc)
+ return FALSE;
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);
pt_regs = (struct ppc64_pt_regs *)bt_info.machdep;
if (!pt_regs) {
@@ -2562,46 +2566,46 @@ ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
switch (regno) {
case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
- if (size != sizeof(pt_regs->gpr[regno]))
- return FALSE; // size mismatch
-
+ if (size != sizeof(pt_regs->gpr[regno])) {
+ ret = FALSE; break; // size mismatch
+ }
memcpy(value, &pt_regs->gpr[regno], size);
- break;
+ ret = TRUE; break;
case PPC64_PC_REGNUM:
- if (size != sizeof(pt_regs->nip))
- return FALSE; // size mismatch
-
+ if (size != sizeof(pt_regs->nip)) {
+ ret = FALSE; break; // size mismatch
+ }
memcpy(value, &pt_regs->nip, size);
- break;
+ ret = TRUE; break;
case PPC64_MSR_REGNUM:
- if (size != sizeof(pt_regs->msr))
- return FALSE; // size mismatch
-
+ if (size != sizeof(pt_regs->msr)) {
+ ret = FALSE; break; // size mismatch
+ }
memcpy(value, &pt_regs->msr, size);
- break;
+ ret = TRUE; break;
case PPC64_LR_REGNUM:
- if (size != sizeof(pt_regs->link))
- return FALSE; // size mismatch
-
+ if (size != sizeof(pt_regs->link)) {
+ ret = FALSE; break; // size mismatch
+ }
memcpy(value, &pt_regs->link, size);
- break;
+ ret = TRUE; break;
case PPC64_CTR_REGNUM:
- if (size != sizeof(pt_regs->ctr))
- return FALSE; // size mismatch
-
+ if (size != sizeof(pt_regs->ctr)) {
+ ret = FALSE; break; // size mismatch
+ }
memcpy(value, &pt_regs->ctr, size);
- break;
+ ret = TRUE; break;
+ }
+ if (bt_info.need_free) {
+ FREEBUF(pt_regs);
+ bt_info.need_free = FALSE;
}
- /* free buffer allocated by fill_stackbuf */
- if (bt_info.stackbuf)
- FREEBUF(bt_info.stackbuf);
-
- return TRUE;
+ return ret;
}
/*
@@ -2884,19 +2888,27 @@ static void
ppc64_get_stack_frame(struct bt_info *bt, ulong *pcp, ulong *spp)
{
ulong ksp, nip;
-
+ struct ppc64_pt_regs *regs;
+
nip = ksp = 0;
- if (DUMPFILE() && is_task_active(bt->task))
+ if (DUMPFILE() && is_task_active(bt->task)) {
ppc64_get_dumpfile_stack_frame(bt, &nip, &ksp);
- else
+ bt->need_free = FALSE;
+ } else {
get_ppc64_frame(bt, &nip, &ksp);
+ regs = (struct ppc64_pt_regs *)GETBUF(sizeof(struct ppc64_pt_regs));
+ memset(regs, 0, sizeof(struct ppc64_pt_regs));
+ regs->nip = nip;
+ regs->gpr[1] = ksp;
+ bt->machdep = regs;
+ bt->need_free = TRUE;
+ }
if (pcp)
*pcp = nip;
if (spp)
*spp = ksp;
-
}
static ulong
--
2.43.0
2:57 a.m.
Hi Aditya,
On Thu, Feb 22, 2024 at 1:23 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
The Problem:
============
Currently crash is unable to show function arguments and local variables, as
gdb can do. And functionality for moving between frames ('up'/'down') is
not
working in crash.
Crash has 'gdb passthroughs' for things gdb can do, but the gdb passthroughs
'bt', 'frame', 'info locals', 'up', 'down' are
not working either, due to
gdb not getting the register values from `crash_target::fetch_registers`,
which then uses `machdep->get_cpu_reg`, which is not implemented for PPC64
Proposed Solution:
==================
Fix the gdb passthroughs by implementing "machdep->get_cpu_reg" for PPC64.
This way, "gdb mode in crash" will support this feature for both ELF and
kdump-compressed vmcore formats, while "gdb" would only have supported ELF
format
This way other features of 'gdb', such as seeing
backtraces/registers/variables/arguments/local variables, moving up and
down stack frames, can be used with any ppc64 vmcore, irrespective of
being ELF format or kdump-compressed format.
Note: This doesn't support live debugging on ppc64, since registers are not
available to be read
Implications on Architectures:
====================================
No architecture other than PPC64 has been affected, other than in case of
'frame' command
As mentioned in patch #2, since frame will not be prohibited, so it will print:
crash> frame
#0 <unavailable> in ?? ()
Instead of before prohibited message:
crash> frame
crash: prohibited gdb command: frame
Major change will be in 'gdb mode' on PPC64, that it will print the frames, and
local variables, instead of failing with errors showing no frame, or showing
that couldn't get PC, it will be able to give all this information.
Testing:
========
Git tree with this patch series applied:
https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
I doubt the v9 patch will not work without my x86's trial patchset. I
see your repo, you directly applied my "ppc64 arbitrary task stack
unwind support" patch onto yours. However the patch has some
dependency on my x86 trial patchset.
E.g. I added a new member "bool need_free" for defs.h:struct bt_info
in the "x86 unwind support" patch of mine, however you didn't pick the
one. And in ppc64.c:ppc64_get_stack_frame() and
ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
if (bt_info.need_free) {
FREEBUF(pt_regs);
bt_info.need_free = FALSE;
}
So I guess (not tried yet) that your patchset v9 will not work.
Currently I'm still struggling with some failing cases of x86_64
unwinding. So I didn't arrange my patchsets, along with the patch
commit log well, since they are all "trial" patches.
I agree the patch "ppc64 arbitrary task stack unwind support" is
better to go with the ppc patch series. But I suggest we make some
modifications for it:
1) I'm OK with it being a stand alone patch, or merging the code
changes of this one into your previous patches, but I prefer the
latter one :)
2) If you'd like to go with a stand alone patch, could you please
rewrite a commit log and title for this one?
Thanks,
Tao Liu
To test various gdb passthroughs:
(crash) set
(crash) set gdb on
gdb> thread
gdb> bt
gdb> info threads
gdb> info threads
gdb> info locals
gdb> info variables irq_rover_lock
gdb> info args
gdb> thread 2
gdb> set gdb off
(crash) set
(crash) set -c 6
(crash) gdb thread
(crash) bt
(crash) gdb bt
(crash) frame
(crash) gdb up
(crash) gdb down
(crash) info locals
Known Issues:
=============
1. In gdb mode, 'bt' might fail to show backtrace in few vmcores collected
from older kernels. This is a known issue due to register mismatch, and
its fix has been merged upstream:
This can also cause some 'invalid kernel virtual address' errors during gdb
unwinding the stack registers
Commit:
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
Fixing GDB passthroughs on other architectures
==============================================
Much of the work for making gdb passthroughs like 'gdb bt', 'gdb
thread', 'gdb info locals' etc. has been done by the patches introducing
'machdep->get_cpu_reg' and this series fixing some issues in that.
Other architectures should be able to fix these gdb functionalities by
simply implementing 'machdep->get_cpu_reg (cpu, regno, ...)'.
The reasoning behind that has been explained with a diagram in commit
description of patch #1
I will assist with my findings/observations fixing it on ppc64 whenever needed.
Changelog:
==========
V9:
+ minor change in patch #5: sync gdb context on a 'set' and 'set -p'
+ add taoliu's patch for using current context, and fixes in ppc64_get_cpu_reg
V8:
+ use get_active_task instead of depending on CURRENT_CONTEXT in ppc64_get_cpu_reg
+ rebase to upstream/master (5977936c0a91)
V7:
+ move changes in gdb-10.2.patch to the end (minor change in patch #3,4,5)
+ fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
+ use ascii diagram in patch #1 description
V6:
+ changes in patch #5: fix bug introduced in v5 that caused initial gdb thread
to be thread 1
V5:
+ changes in patch #1: made ppc64_get_cpu_reg static, and remove unreachable
code
+ changes in patch #3: fixed typo 'ppc64_renum' instead of
'ppc64_regnum',
remove unneeded if condition
+ changes in patch #5: implement refresh regcache on per thread, instead of all
threads at once
V4:
+ fix segmentation fault in live debugging (change in patch #1)
+ mention live debugging not supported in cover letter and patch #1
+ fixed some checkpatch warnings (change in patch #5)
V3:
+ default gdb thread will be the crashing thread, instead of being
thread '0'
+ synchronise crash cpu and gdb thread context
+ fix bug in gdb_interface, that replaced gdb's output stream, losing
output in some cases, such as info threads and extra output in info
variables
+ fix 'info threads'
RFC V2:
- removed patch implementing 'frame', 'up', 'down' in crash
- updated the cover letter by removing the mention of those commands other
than the respective gdb passthrough
Aditya Gupta (5):
ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
remove 'frame' from prohibited commands list
synchronise cpu context changes between crash/gdb
fix gdb_interface: restore gdb's output streams at end of
gdb_interface
fix 'info threads' command
crash_target.c | 44 ++++++++++++++++
defs.h | 130 +++++++++++++++++++++++++++++++++++++++++++++++-
gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
gdb_interface.c | 2 +-
kernel.c | 47 +++++++++++++++--
ppc64.c | 95 +++++++++++++++++++++++++++++++++--
task.c | 14 ++++++
tools.c | 2 +-
8 files changed, 434 insertions(+), 10 deletions(-)
--
2.41.0
3:16 a.m.
Hi, Aditya and Tao
Thank you for working on this.
I would suggest that the current feature can be splitted into several
steps, for example:
[1] complete a basic functionality on ppc64
[2] improve it based on [1], such known issues
[3] after finishing the [1](and [2]), Tao will implement it on X86 64
That can avoid confusion(or dependency) between ppc64 and X86 64 patches,
it may save some time and make it easier to review patches.
Thanks.
Lianbo
On Thu, Feb 22, 2024 at 4:58 PM Tao Liu <ltao(a)redhat.com> wrote:
Hi Aditya,
On Thu, Feb 22, 2024 at 1:23 PM Aditya Gupta <adityag(a)linux.ibm.com>
wrote:
>
> The Problem:
> ============
>
> Currently crash is unable to show function arguments and local
variables, as
> gdb can do. And functionality for moving between frames
('up'/'down') is
not
> working in crash.
>
> Crash has 'gdb passthroughs' for things gdb can do, but the gdb
passthroughs
> 'bt', 'frame', 'info locals', 'up', 'down'
are not working either, due to
> gdb not getting the register values from `crash_target::fetch_registers`,
> which then uses `machdep->get_cpu_reg`, which is not implemented for
PPC64
>
> Proposed Solution:
> ==================
>
> Fix the gdb passthroughs by implementing "machdep->get_cpu_reg" for
PPC64.
> This way, "gdb mode in crash" will support this feature for both ELF and
> kdump-compressed vmcore formats, while "gdb" would only have supported
ELF
> format
>
> This way other features of 'gdb', such as seeing
> backtraces/registers/variables/arguments/local variables, moving up and
> down stack frames, can be used with any ppc64 vmcore, irrespective of
> being ELF format or kdump-compressed format.
>
> Note: This doesn't support live debugging on ppc64, since registers are
not
> available to be read
>
> Implications on Architectures:
> ====================================
>
> No architecture other than PPC64 has been affected, other than in case of
> 'frame' command
>
> As mentioned in patch #2, since frame will not be prohibited, so it will
print:
>
> crash> frame
> #0 <unavailable> in ?? ()
>
> Instead of before prohibited message:
>
> crash> frame
> crash: prohibited gdb command: frame
>
> Major change will be in 'gdb mode' on PPC64, that it will print the
frames, and
> local variables, instead of failing with errors showing no frame, or
showing
> that couldn't get PC, it will be able to give all this information.
>
> Testing:
> ========
>
> Git tree with this patch series applied:
> https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
I doubt the v9 patch will not work without my x86's trial patchset. I
see your repo, you directly applied my "ppc64 arbitrary task stack
unwind support" patch onto yours. However the patch has some
dependency on my x86 trial patchset.
E.g. I added a new member "bool need_free" for defs.h:struct bt_info
in the "x86 unwind support" patch of mine, however you didn't pick the
one. And in ppc64.c:ppc64_get_stack_frame() and
ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
if (bt_info.need_free) {
FREEBUF(pt_regs);
bt_info.need_free = FALSE;
}
So I guess (not tried yet) that your patchset v9 will not work.
Currently I'm still struggling with some failing cases of x86_64
unwinding. So I didn't arrange my patchsets, along with the patch
commit log well, since they are all "trial" patches.
I agree the patch "ppc64 arbitrary task stack unwind support" is
better to go with the ppc patch series. But I suggest we make some
modifications for it:
1) I'm OK with it being a stand alone patch, or merging the code
changes of this one into your previous patches, but I prefer the
latter one :)
2) If you'd like to go with a stand alone patch, could you please
rewrite a commit log and title for this one?
Thanks,
Tao Liu
>
> To test various gdb passthroughs:
>
> (crash) set
> (crash) set gdb on
> gdb> thread
> gdb> bt
> gdb> info threads
> gdb> info threads
> gdb> info locals
> gdb> info variables irq_rover_lock
> gdb> info args
> gdb> thread 2
> gdb> set gdb off
> (crash) set
> (crash) set -c 6
> (crash) gdb thread
> (crash) bt
> (crash) gdb bt
> (crash) frame
> (crash) gdb up
> (crash) gdb down
> (crash) info locals
>
> Known Issues:
> =============
>
> 1. In gdb mode, 'bt' might fail to show backtrace in few vmcores
collected
> from older kernels. This is a known issue due to register mismatch,
and
> its fix has been merged upstream:
>
> This can also cause some 'invalid kernel virtual address' errors
during gdb
> unwinding the stack registers
>
> Commit:
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
>
> Fixing GDB passthroughs on other architectures
> ==============================================
>
> Much of the work for making gdb passthroughs like 'gdb bt', 'gdb
> thread', 'gdb info locals' etc. has been done by the patches
introducing
> 'machdep->get_cpu_reg' and this series fixing some issues in that.
>
> Other architectures should be able to fix these gdb functionalities by
> simply implementing 'machdep->get_cpu_reg (cpu, regno, ...)'.
>
> The reasoning behind that has been explained with a diagram in commit
> description of patch #1
>
> I will assist with my findings/observations fixing it on ppc64 whenever
needed.
>
> Changelog:
> ==========
>
> V9:
> + minor change in patch #5: sync gdb context on a 'set' and 'set
-p'
> + add taoliu's patch for using current context, and fixes in
ppc64_get_cpu_reg
>
> V8:
> + use get_active_task instead of depending on CURRENT_CONTEXT in
ppc64_get_cpu_reg
> + rebase to upstream/master (5977936c0a91)
>
> V7:
> + move changes in gdb-10.2.patch to the end (minor change in patch
#3,4,5)
> + fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
> + use ascii diagram in patch #1 description
>
> V6:
> + changes in patch #5: fix bug introduced in v5 that caused initial gdb
thread
> to be thread 1
>
> V5:
> + changes in patch #1: made ppc64_get_cpu_reg static, and remove
unreachable
> code
> + changes in patch #3: fixed typo 'ppc64_renum' instead of
'ppc64_regnum',
> remove unneeded if condition
> + changes in patch #5: implement refresh regcache on per thread, instead
of all
> threads at once
>
> V4:
> + fix segmentation fault in live debugging (change in patch #1)
> + mention live debugging not supported in cover letter and patch #1
> + fixed some checkpatch warnings (change in patch #5)
>
> V3:
> + default gdb thread will be the crashing thread, instead of being
> thread '0'
> + synchronise crash cpu and gdb thread context
> + fix bug in gdb_interface, that replaced gdb's output stream, losing
> output in some cases, such as info threads and extra output in info
> variables
> + fix 'info threads'
>
> RFC V2:
> - removed patch implementing 'frame', 'up', 'down' in
crash
> - updated the cover letter by removing the mention of those commands
other
> than the respective gdb passthrough
>
> Aditya Gupta (5):
> ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
> remove 'frame' from prohibited commands list
> synchronise cpu context changes between crash/gdb
> fix gdb_interface: restore gdb's output streams at end of
> gdb_interface
> fix 'info threads' command
>
> crash_target.c | 44 ++++++++++++++++
> defs.h | 130 +++++++++++++++++++++++++++++++++++++++++++++++-
> gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
> gdb_interface.c | 2 +-
> kernel.c | 47 +++++++++++++++--
> ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> task.c | 14 ++++++
> tools.c | 2 +-
> 8 files changed, 434 insertions(+), 10 deletions(-)
>
> --
> 2.41.0
>
4:29 a.m.
Hi Lianbo,
On Thu, Feb 22, 2024 at 05:16:31PM +0800, lijiang wrote:
Hi, Aditya and Tao
Thank you for working on this.
I would suggest that the current feature can be splitted into several
steps, for example:
[1] complete a basic functionality on ppc64
[2] improve it based on [1], such known issues
[3] after finishing the [1](and [2]), Tao will implement it on X86 64
That can avoid confusion(or dependency) between ppc64 and X86 64 patches,
it may save some time and make it easier to review patches.
Sure, actually I made a mistake in v9, other than that there should be
no dependency in this patch series.
This is the summary:
Patch #1: PPC64 specific
Patch #2 - #5: Architecture Independent, with patch #4 fixing an
existing bug in gdb_interface, this can be an independent patch.
So based on your suggestions, let's split into it as:
1. This patch series having Patch #1,2,3,5 (removed 4)
2. Fix for existing bug in gdb_interface: Patch #4, It will be good if
this can be merged soon, as testing 'info threads' will not work due to
the bug
3. Tao's patches for x86_64
Also, even though i have kept #2-#5 as architecture-independent, but
then they should be merged after the ppc patches, for their description
and usage to make sense, so I still kept them in the series.
Do let me know, I will send a v10 accordingly :)
Thanks,
Aditya Gupta
>
> Thanks.
> Lianbo
>
> On Thu, Feb 22, 2024 at 4:58 PM Tao Liu <ltao(a)redhat.com> wrote:
>
> > Hi Aditya,
> >
> > On Thu, Feb 22, 2024 at 1:23 PM Aditya Gupta <adityag(a)linux.ibm.com>
> > wrote:
> > >
> > > The Problem:
> > > ============
> > >
> > > Currently crash is unable to show function arguments and local
> > variables, as
> > > gdb can do. And functionality for moving between frames
('up'/'down') is
> > not
> > > working in crash.
> > >
> > > Crash has 'gdb passthroughs' for things gdb can do, but the gdb
> > passthroughs
> > > 'bt', 'frame', 'info locals', 'up',
'down' are not working either, due to
> > > gdb not getting the register values from `crash_target::fetch_registers`,
> > > which then uses `machdep->get_cpu_reg`, which is not implemented for
> > PPC64
> > >
> > > Proposed Solution:
> > > ==================
> > >
> > > Fix the gdb passthroughs by implementing
"machdep->get_cpu_reg" for
> > PPC64.
> > > This way, "gdb mode in crash" will support this feature for both
ELF and
> > > kdump-compressed vmcore formats, while "gdb" would only have
supported
> > ELF
> > > format
> > >
> > > This way other features of 'gdb', such as seeing
> > > backtraces/registers/variables/arguments/local variables, moving up and
> > > down stack frames, can be used with any ppc64 vmcore, irrespective of
> > > being ELF format or kdump-compressed format.
> > >
> > > Note: This doesn't support live debugging on ppc64, since registers
are
> > not
> > > available to be read
> > >
> > > Implications on Architectures:
> > > ====================================
> > >
> > > No architecture other than PPC64 has been affected, other than in case of
> > > 'frame' command
> > >
> > > As mentioned in patch #2, since frame will not be prohibited, so it will
> > print:
> > >
> > > crash> frame
> > > #0 <unavailable> in ?? ()
> > >
> > > Instead of before prohibited message:
> > >
> > > crash> frame
> > > crash: prohibited gdb command: frame
> > >
> > > Major change will be in 'gdb mode' on PPC64, that it will print
the
> > frames, and
> > > local variables, instead of failing with errors showing no frame, or
> > showing
> > > that couldn't get PC, it will be able to give all this information.
> > >
> > > Testing:
> > > ========
> > >
> > > Git tree with this patch series applied:
> > > https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
> >
> > I doubt the v9 patch will not work without my x86's trial patchset. I
> > see your repo, you directly applied my "ppc64 arbitrary task stack
> > unwind support" patch onto yours. However the patch has some
> > dependency on my x86 trial patchset.
> >
> > E.g. I added a new member "bool need_free" for defs.h:struct bt_info
> > in the "x86 unwind support" patch of mine, however you didn't pick
the
> > one. And in ppc64.c:ppc64_get_stack_frame() and
> > ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
> >
> > if (bt_info.need_free) {
> > FREEBUF(pt_regs);
> > bt_info.need_free = FALSE;
> > }
> >
> > So I guess (not tried yet) that your patchset v9 will not work.
> >
> > Currently I'm still struggling with some failing cases of x86_64
> > unwinding. So I didn't arrange my patchsets, along with the patch
> > commit log well, since they are all "trial" patches.
> >
> > I agree the patch "ppc64 arbitrary task stack unwind support" is
> > better to go with the ppc patch series. But I suggest we make some
> > modifications for it:
> >
> > 1) I'm OK with it being a stand alone patch, or merging the code
> > changes of this one into your previous patches, but I prefer the
> > latter one :)
> >
> > 2) If you'd like to go with a stand alone patch, could you please
> > rewrite a commit log and title for this one?
> >
> > Thanks,
> > Tao Liu
> >
> >
> > >
> > > To test various gdb passthroughs:
> > >
> > > (crash) set
> > > (crash) set gdb on
> > > gdb> thread
> > > gdb> bt
> > > gdb> info threads
> > > gdb> info threads
> > > gdb> info locals
> > > gdb> info variables irq_rover_lock
> > > gdb> info args
> > > gdb> thread 2
> > > gdb> set gdb off
> > > (crash) set
> > > (crash) set -c 6
> > > (crash) gdb thread
> > > (crash) bt
> > > (crash) gdb bt
> > > (crash) frame
> > > (crash) gdb up
> > > (crash) gdb down
> > > (crash) info locals
> > >
> > > Known Issues:
> > > =============
> > >
> > > 1. In gdb mode, 'bt' might fail to show backtrace in few vmcores
> > collected
> > > from older kernels. This is a known issue due to register mismatch,
> > and
> > > its fix has been merged upstream:
> > >
> > > This can also cause some 'invalid kernel virtual address'
errors
> > during gdb
> > > unwinding the stack registers
> > >
> > > Commit:
> >
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
> > >
> > > Fixing GDB passthroughs on other architectures
> > > ==============================================
> > >
> > > Much of the work for making gdb passthroughs like 'gdb bt',
'gdb
> > > thread', 'gdb info locals' etc. has been done by the patches
introducing
> > > 'machdep->get_cpu_reg' and this series fixing some issues in
that.
> > >
> > > Other architectures should be able to fix these gdb functionalities by
> > > simply implementing 'machdep->get_cpu_reg (cpu, regno, ...)'.
> > >
> > > The reasoning behind that has been explained with a diagram in commit
> > > description of patch #1
> > >
> > > I will assist with my findings/observations fixing it on ppc64 whenever
> > needed.
> > >
> > > Changelog:
> > > ==========
> > >
> > > V9:
> > > + minor change in patch #5: sync gdb context on a 'set' and
'set -p'
> > > + add taoliu's patch for using current context, and fixes in
> > ppc64_get_cpu_reg
> > >
> > > V8:
> > > + use get_active_task instead of depending on CURRENT_CONTEXT in
> > ppc64_get_cpu_reg
> > > + rebase to upstream/master (5977936c0a91)
> > >
> > > V7:
> > > + move changes in gdb-10.2.patch to the end (minor change in patch
> > #3,4,5)
> > > + fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
> > > + use ascii diagram in patch #1 description
> > >
> > > V6:
> > > + changes in patch #5: fix bug introduced in v5 that caused initial gdb
> > thread
> > > to be thread 1
> > >
> > > V5:
> > > + changes in patch #1: made ppc64_get_cpu_reg static, and remove
> > unreachable
> > > code
> > > + changes in patch #3: fixed typo 'ppc64_renum' instead of
> > 'ppc64_regnum',
> > > remove unneeded if condition
> > > + changes in patch #5: implement refresh regcache on per thread, instead
> > of all
> > > threads at once
> > >
> > > V4:
> > > + fix segmentation fault in live debugging (change in patch #1)
> > > + mention live debugging not supported in cover letter and patch #1
> > > + fixed some checkpatch warnings (change in patch #5)
> > >
> > > V3:
> > > + default gdb thread will be the crashing thread, instead of being
> > > thread '0'
> > > + synchronise crash cpu and gdb thread context
> > > + fix bug in gdb_interface, that replaced gdb's output stream, losing
> > > output in some cases, such as info threads and extra output in info
> > > variables
> > > + fix 'info threads'
> > >
> > > RFC V2:
> > > - removed patch implementing 'frame', 'up',
'down' in crash
> > > - updated the cover letter by removing the mention of those commands
> > other
> > > than the respective gdb passthrough
> > >
> > > Aditya Gupta (5):
> > > ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
> > > remove 'frame' from prohibited commands list
> > > synchronise cpu context changes between crash/gdb
> > > fix gdb_interface: restore gdb's output streams at end of
> > > gdb_interface
> > > fix 'info threads' command
> > >
> > > crash_target.c | 44 ++++++++++++++++
> > > defs.h | 130 +++++++++++++++++++++++++++++++++++++++++++++++-
> > > gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
> > > gdb_interface.c | 2 +-
> > > kernel.c | 47 +++++++++++++++--
> > > ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> > > task.c | 14 ++++++
> > > tools.c | 2 +-
> > > 8 files changed, 434 insertions(+), 10 deletions(-)
> > >
> > > --
> > > 2.41.0
> > >
> >
> >
5:12 a.m.
Hi Lianbo,
On Thu, Feb 22, 2024 at 03:59:36PM +0530, Aditya Gupta wrote:
Hi Lianbo,
On Thu, Feb 22, 2024 at 05:16:31PM +0800, lijiang wrote:
> Hi, Aditya and Tao
>
> Thank you for working on this.
>
> I would suggest that the current feature can be splitted into several
> steps, for example:
> [1] complete a basic functionality on ppc64
> [2] improve it based on [1], such known issues
> [3] after finishing the [1](and [2]), Tao will implement it on X86 64
>
> That can avoid confusion(or dependency) between ppc64 and X86 64 patches,
> it may save some time and make it easier to review patches.
Sure, actually I made a mistake in v9, other than that there should be
no dependency in this patch series.
This is the summary:
Patch #1: PPC64 specific
Patch #2 - #5: Architecture Independent, with patch #4 fixing an
existing bug in gdb_interface, this can be an independent patch.
So based on your suggestions, let's split into it as:
1. This patch series having Patch #1,2,3,5 (removed 4)
2. Fix for existing bug in gdb_interface: Patch #4, It will be good if
this can be merged soon, as testing 'info threads' will not work due to
the bug
3. Tao's patches for x86_64
Also, even though i have kept #2-#5 as architecture-independent, but
then they should be merged after the ppc patches, for their description
and usage to make sense, so I still kept them in the series.
Any comments on this ?
I am still of the view it makes sense for all 5 patches to go in as a
single series, since independently they might not make much sense (such
as, even though patch #4 fixes an existing bug, but info threads will
not hit the bug unless the architecture supports gdb mode unwinding).
But if splitting it makes more sense, I am okay with it.
Thanks,
Aditya Gupta
>
> Do let me know, I will send a v10 accordingly :)
>
> Thanks,
> Aditya Gupta
>
> >
> > Thanks.
> > Lianbo
> >
> > On Thu, Feb 22, 2024 at 4:58 PM Tao Liu <ltao(a)redhat.com> wrote:
> >
> > > Hi Aditya,
> > >
> > > On Thu, Feb 22, 2024 at 1:23 PM Aditya Gupta <adityag(a)linux.ibm.com>
> > > wrote:
> > > >
> > > > The Problem:
> > > > ============
> > > >
> > > > Currently crash is unable to show function arguments and local
> > > variables, as
> > > > gdb can do. And functionality for moving between frames
('up'/'down') is
> > > not
> > > > working in crash.
> > > >
> > > > Crash has 'gdb passthroughs' for things gdb can do, but the
gdb
> > > passthroughs
> > > > 'bt', 'frame', 'info locals', 'up',
'down' are not working either, due to
> > > > gdb not getting the register values from
`crash_target::fetch_registers`,
> > > > which then uses `machdep->get_cpu_reg`, which is not implemented
for
> > > PPC64
> > > >
> > > > Proposed Solution:
> > > > ==================
> > > >
> > > > Fix the gdb passthroughs by implementing
"machdep->get_cpu_reg" for
> > > PPC64.
> > > > This way, "gdb mode in crash" will support this feature for
both ELF and
> > > > kdump-compressed vmcore formats, while "gdb" would only have
supported
> > > ELF
> > > > format
> > > >
> > > > This way other features of 'gdb', such as seeing
> > > > backtraces/registers/variables/arguments/local variables, moving up
and
> > > > down stack frames, can be used with any ppc64 vmcore, irrespective of
> > > > being ELF format or kdump-compressed format.
> > > >
> > > > Note: This doesn't support live debugging on ppc64, since
registers are
> > > not
> > > > available to be read
> > > >
> > > > Implications on Architectures:
> > > > ====================================
> > > >
> > > > No architecture other than PPC64 has been affected, other than in case
of
> > > > 'frame' command
> > > >
> > > > As mentioned in patch #2, since frame will not be prohibited, so it
will
> > > print:
> > > >
> > > > crash> frame
> > > > #0 <unavailable> in ?? ()
> > > >
> > > > Instead of before prohibited message:
> > > >
> > > > crash> frame
> > > > crash: prohibited gdb command: frame
> > > >
> > > > Major change will be in 'gdb mode' on PPC64, that it will
print the
> > > frames, and
> > > > local variables, instead of failing with errors showing no frame, or
> > > showing
> > > > that couldn't get PC, it will be able to give all this
information.
> > > >
> > > > Testing:
> > > > ========
> > > >
> > > > Git tree with this patch series applied:
> > > > https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
> > >
> > > I doubt the v9 patch will not work without my x86's trial patchset. I
> > > see your repo, you directly applied my "ppc64 arbitrary task stack
> > > unwind support" patch onto yours. However the patch has some
> > > dependency on my x86 trial patchset.
> > >
> > > E.g. I added a new member "bool need_free" for defs.h:struct
bt_info
> > > in the "x86 unwind support" patch of mine, however you didn't
pick the
> > > one. And in ppc64.c:ppc64_get_stack_frame() and
> > > ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
> > >
> > > if (bt_info.need_free) {
> > > FREEBUF(pt_regs);
> > > bt_info.need_free = FALSE;
> > > }
> > >
> > > So I guess (not tried yet) that your patchset v9 will not work.
> > >
> > > Currently I'm still struggling with some failing cases of x86_64
> > > unwinding. So I didn't arrange my patchsets, along with the patch
> > > commit log well, since they are all "trial" patches.
> > >
> > > I agree the patch "ppc64 arbitrary task stack unwind support" is
> > > better to go with the ppc patch series. But I suggest we make some
> > > modifications for it:
> > >
> > > 1) I'm OK with it being a stand alone patch, or merging the code
> > > changes of this one into your previous patches, but I prefer the
> > > latter one :)
> > >
> > > 2) If you'd like to go with a stand alone patch, could you please
> > > rewrite a commit log and title for this one?
> > >
> > > Thanks,
> > > Tao Liu
> > >
> > >
> > > >
> > > > To test various gdb passthroughs:
> > > >
> > > > (crash) set
> > > > (crash) set gdb on
> > > > gdb> thread
> > > > gdb> bt
> > > > gdb> info threads
> > > > gdb> info threads
> > > > gdb> info locals
> > > > gdb> info variables irq_rover_lock
> > > > gdb> info args
> > > > gdb> thread 2
> > > > gdb> set gdb off
> > > > (crash) set
> > > > (crash) set -c 6
> > > > (crash) gdb thread
> > > > (crash) bt
> > > > (crash) gdb bt
> > > > (crash) frame
> > > > (crash) gdb up
> > > > (crash) gdb down
> > > > (crash) info locals
> > > >
> > > > Known Issues:
> > > > =============
> > > >
> > > > 1. In gdb mode, 'bt' might fail to show backtrace in few
vmcores
> > > collected
> > > > from older kernels. This is a known issue due to register
mismatch,
> > > and
> > > > its fix has been merged upstream:
> > > >
> > > > This can also cause some 'invalid kernel virtual address'
errors
> > > during gdb
> > > > unwinding the stack registers
> > > >
> > > > Commit:
> > >
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
> > > >
> > > > Fixing GDB passthroughs on other architectures
> > > > ==============================================
> > > >
> > > > Much of the work for making gdb passthroughs like 'gdb bt',
'gdb
> > > > thread', 'gdb info locals' etc. has been done by the
patches introducing
> > > > 'machdep->get_cpu_reg' and this series fixing some issues
in that.
> > > >
> > > > Other architectures should be able to fix these gdb functionalities
by
> > > > simply implementing 'machdep->get_cpu_reg (cpu, regno,
...)'.
> > > >
> > > > The reasoning behind that has been explained with a diagram in commit
> > > > description of patch #1
> > > >
> > > > I will assist with my findings/observations fixing it on ppc64
whenever
> > > needed.
> > > >
> > > > Changelog:
> > > > ==========
> > > >
> > > > V9:
> > > > + minor change in patch #5: sync gdb context on a 'set' and
'set -p'
> > > > + add taoliu's patch for using current context, and fixes in
> > > ppc64_get_cpu_reg
> > > >
> > > > V8:
> > > > + use get_active_task instead of depending on CURRENT_CONTEXT in
> > > ppc64_get_cpu_reg
> > > > + rebase to upstream/master (5977936c0a91)
> > > >
> > > > V7:
> > > > + move changes in gdb-10.2.patch to the end (minor change in patch
> > > #3,4,5)
> > > > + fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
> > > > + use ascii diagram in patch #1 description
> > > >
> > > > V6:
> > > > + changes in patch #5: fix bug introduced in v5 that caused initial
gdb
> > > thread
> > > > to be thread 1
> > > >
> > > > V5:
> > > > + changes in patch #1: made ppc64_get_cpu_reg static, and remove
> > > unreachable
> > > > code
> > > > + changes in patch #3: fixed typo 'ppc64_renum' instead of
> > > 'ppc64_regnum',
> > > > remove unneeded if condition
> > > > + changes in patch #5: implement refresh regcache on per thread,
instead
> > > of all
> > > > threads at once
> > > >
> > > > V4:
> > > > + fix segmentation fault in live debugging (change in patch #1)
> > > > + mention live debugging not supported in cover letter and patch #1
> > > > + fixed some checkpatch warnings (change in patch #5)
> > > >
> > > > V3:
> > > > + default gdb thread will be the crashing thread, instead of being
> > > > thread '0'
> > > > + synchronise crash cpu and gdb thread context
> > > > + fix bug in gdb_interface, that replaced gdb's output stream,
losing
> > > > output in some cases, such as info threads and extra output in info
> > > > variables
> > > > + fix 'info threads'
> > > >
> > > > RFC V2:
> > > > - removed patch implementing 'frame', 'up',
'down' in crash
> > > > - updated the cover letter by removing the mention of those
commands
> > > other
> > > > than the respective gdb passthrough
> > > >
> > > > Aditya Gupta (5):
> > > > ppc64: correct gdb passthroughs by implementing
machdep->get_cpu_reg
> > > > remove 'frame' from prohibited commands list
> > > > synchronise cpu context changes between crash/gdb
> > > > fix gdb_interface: restore gdb's output streams at end of
> > > > gdb_interface
> > > > fix 'info threads' command
> > > >
> > > > crash_target.c | 44 ++++++++++++++++
> > > > defs.h | 130
+++++++++++++++++++++++++++++++++++++++++++++++-
> > > > gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
> > > > gdb_interface.c | 2 +-
> > > > kernel.c | 47 +++++++++++++++--
> > > > ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> > > > task.c | 14 ++++++
> > > > tools.c | 2 +-
> > > > 8 files changed, 434 insertions(+), 10 deletions(-)
> > > >
> > > > --
> > > > 2.41.0
> > > >
> > >
> > >
9:27 p.m.
Hi, Aditya
On Mon, Feb 26, 2024 at 7:12 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
Hi Lianbo,
On Thu, Feb 22, 2024 at 03:59:36PM +0530, Aditya Gupta wrote:
> Hi Lianbo,
>
> On Thu, Feb 22, 2024 at 05:16:31PM +0800, lijiang wrote:
> > Hi, Aditya and Tao
> >
> > Thank you for working on this.
> >
> > I would suggest that the current feature can be splitted into several
> > steps, for example:
> > [1] complete a basic functionality on ppc64
> > [2] improve it based on [1], such known issues
> > [3] after finishing the [1](and [2]), Tao will implement it on X86 64
> >
> > That can avoid confusion(or dependency) between ppc64 and X86 64
patches,
> > it may save some time and make it easier to review patches.
>
> Sure, actually I made a mistake in v9, other than that there should be
> no dependency in this patch series.
> This is the summary:
>
> Patch #1: PPC64 specific
> Patch #2 - #5: Architecture Independent, with patch #4 fixing an
> existing bug in gdb_interface, this can be an independent patch.
>
> So based on your suggestions, let's split into it as:
>
> 1. This patch series having Patch #1,2,3,5 (removed 4)
> 2. Fix for existing bug in gdb_interface: Patch #4, It will be good if
> this can be merged soon, as testing 'info threads' will not work due to
> the bug
> 3. Tao's patches for x86_64
>
> Also, even though i have kept #2-#5 as architecture-independent, but
> then they should be merged after the ppc patches, for their description
> and usage to make sense, so I still kept them in the series.
Any comments on this ?
I am still of the view it makes sense for all 5 patches to go in as a
single series, since independently they might not make much sense (such
as, even though patch #4 fixes an existing bug, but info threads will
not hit the bug unless the architecture supports gdb mode unwinding).
But if splitting it makes more sense, I am okay with it.
Sorry about this, I should explain it in detail.
Earlier I saw you were discussing with Tao about the following changes:
"E.g. I added a new member "bool need_free" for defs.h:struct bt_info
in the "x86 unwind support" patch of mine, however you didn't pick the
one. And in ppc64.c:ppc64_get_stack_frame() and
ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
if (bt_info.need_free) {
FREEBUF(pt_regs);
bt_info.need_free = FALSE;
}
"
So I would suggest avoiding confusion between ppc64 and x86 64 patches, and
split into several steps. Also do not put X86 64 changes into ppc64 series.
Thanks.
Lianbo
Thanks,
Aditya Gupta
>
> Do let me know, I will send a v10 accordingly :)
>
> Thanks,
> Aditya Gupta
>
> >
> > Thanks.
> > Lianbo
> >
> > On Thu, Feb 22, 2024 at 4:58 PM Tao Liu <ltao(a)redhat.com> wrote:
> >
> > > Hi Aditya,
> > >
> > > On Thu, Feb 22, 2024 at 1:23 PM Aditya Gupta
<adityag(a)linux.ibm.com>
> > > wrote:
> > > >
> > > > The Problem:
> > > > ============
> > > >
> > > > Currently crash is unable to show function arguments and local
> > > variables, as
> > > > gdb can do. And functionality for moving between frames
('up'/'down') is
> > > not
> > > > working in crash.
> > > >
> > > > Crash has 'gdb passthroughs' for things gdb can do, but the
gdb
> > > passthroughs
> > > > 'bt', 'frame', 'info locals', 'up',
'down' are not working either,
due to
> > > > gdb not getting the register values from
`crash_target::fetch_registers`,
> > > > which then uses `machdep->get_cpu_reg`, which is not implemented
for
> > > PPC64
> > > >
> > > > Proposed Solution:
> > > > ==================
> > > >
> > > > Fix the gdb passthroughs by implementing
"machdep->get_cpu_reg" for
> > > PPC64.
> > > > This way, "gdb mode in crash" will support this feature for
both
ELF and
> > > > kdump-compressed vmcore formats, while "gdb" would only
have
supported
> > > ELF
> > > > format
> > > >
> > > > This way other features of 'gdb', such as seeing
> > > > backtraces/registers/variables/arguments/local variables, moving
up and
> > > > down stack frames, can be used with any ppc64 vmcore, irrespective
of
> > > > being ELF format or kdump-compressed format.
> > > >
> > > > Note: This doesn't support live debugging on ppc64, since
registers are
> > > not
> > > > available to be read
> > > >
> > > > Implications on Architectures:
> > > > ====================================
> > > >
> > > > No architecture other than PPC64 has been affected, other than in
case of
> > > > 'frame' command
> > > >
> > > > As mentioned in patch #2, since frame will not be prohibited, so
it will
> > > print:
> > > >
> > > > crash> frame
> > > > #0 <unavailable> in ?? ()
> > > >
> > > > Instead of before prohibited message:
> > > >
> > > > crash> frame
> > > > crash: prohibited gdb command: frame
> > > >
> > > > Major change will be in 'gdb mode' on PPC64, that it will
print the
> > > frames, and
> > > > local variables, instead of failing with errors showing no frame,
or
> > > showing
> > > > that couldn't get PC, it will be able to give all this
information.
> > > >
> > > > Testing:
> > > > ========
> > > >
> > > > Git tree with this patch series applied:
> > > > https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
> > >
> > > I doubt the v9 patch will not work without my x86's trial patchset. I
> > > see your repo, you directly applied my "ppc64 arbitrary task stack
> > > unwind support" patch onto yours. However the patch has some
> > > dependency on my x86 trial patchset.
> > >
> > > E.g. I added a new member "bool need_free" for defs.h:struct
bt_info
> > > in the "x86 unwind support" patch of mine, however you
didn't pick
the
> > > one. And in ppc64.c:ppc64_get_stack_frame() and
> > > ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
> > >
> > > if (bt_info.need_free) {
> > > FREEBUF(pt_regs);
> > > bt_info.need_free = FALSE;
> > > }
> > >
> > > So I guess (not tried yet) that your patchset v9 will not work.
> > >
> > > Currently I'm still struggling with some failing cases of x86_64
> > > unwinding. So I didn't arrange my patchsets, along with the patch
> > > commit log well, since they are all "trial" patches.
> > >
> > > I agree the patch "ppc64 arbitrary task stack unwind support"
is
> > > better to go with the ppc patch series. But I suggest we make some
> > > modifications for it:
> > >
> > > 1) I'm OK with it being a stand alone patch, or merging the code
> > > changes of this one into your previous patches, but I prefer the
> > > latter one :)
> > >
> > > 2) If you'd like to go with a stand alone patch, could you please
> > > rewrite a commit log and title for this one?
> > >
> > > Thanks,
> > > Tao Liu
> > >
> > >
> > > >
> > > > To test various gdb passthroughs:
> > > >
> > > > (crash) set
> > > > (crash) set gdb on
> > > > gdb> thread
> > > > gdb> bt
> > > > gdb> info threads
> > > > gdb> info threads
> > > > gdb> info locals
> > > > gdb> info variables irq_rover_lock
> > > > gdb> info args
> > > > gdb> thread 2
> > > > gdb> set gdb off
> > > > (crash) set
> > > > (crash) set -c 6
> > > > (crash) gdb thread
> > > > (crash) bt
> > > > (crash) gdb bt
> > > > (crash) frame
> > > > (crash) gdb up
> > > > (crash) gdb down
> > > > (crash) info locals
> > > >
> > > > Known Issues:
> > > > =============
> > > >
> > > > 1. In gdb mode, 'bt' might fail to show backtrace in few
vmcores
> > > collected
> > > > from older kernels. This is a known issue due to register
mismatch,
> > > and
> > > > its fix has been merged upstream:
> > > >
> > > > This can also cause some 'invalid kernel virtual address'
errors
> > > during gdb
> > > > unwinding the stack registers
> > > >
> > > > Commit:
> > >
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
> > > >
> > > > Fixing GDB passthroughs on other architectures
> > > > ==============================================
> > > >
> > > > Much of the work for making gdb passthroughs like 'gdb bt',
'gdb
> > > > thread', 'gdb info locals' etc. has been done by the
patches
introducing
> > > > 'machdep->get_cpu_reg' and this series fixing some issues
in that.
> > > >
> > > > Other architectures should be able to fix these gdb
functionalities by
> > > > simply implementing 'machdep->get_cpu_reg (cpu, regno,
...)'.
> > > >
> > > > The reasoning behind that has been explained with a diagram in
commit
> > > > description of patch #1
> > > >
> > > > I will assist with my findings/observations fixing it on ppc64
whenever
> > > needed.
> > > >
> > > > Changelog:
> > > > ==========
> > > >
> > > > V9:
> > > > + minor change in patch #5: sync gdb context on a 'set' and
'set
-p'
> > > > + add taoliu's patch for using current context, and fixes in
> > > ppc64_get_cpu_reg
> > > >
> > > > V8:
> > > > + use get_active_task instead of depending on CURRENT_CONTEXT in
> > > ppc64_get_cpu_reg
> > > > + rebase to upstream/master (5977936c0a91)
> > > >
> > > > V7:
> > > > + move changes in gdb-10.2.patch to the end (minor change in patch
> > > #3,4,5)
> > > > + fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
> > > > + use ascii diagram in patch #1 description
> > > >
> > > > V6:
> > > > + changes in patch #5: fix bug introduced in v5 that caused
initial gdb
> > > thread
> > > > to be thread 1
> > > >
> > > > V5:
> > > > + changes in patch #1: made ppc64_get_cpu_reg static, and remove
> > > unreachable
> > > > code
> > > > + changes in patch #3: fixed typo 'ppc64_renum' instead of
> > > 'ppc64_regnum',
> > > > remove unneeded if condition
> > > > + changes in patch #5: implement refresh regcache on per thread,
instead
> > > of all
> > > > threads at once
> > > >
> > > > V4:
> > > > + fix segmentation fault in live debugging (change in patch #1)
> > > > + mention live debugging not supported in cover letter and patch #1
> > > > + fixed some checkpatch warnings (change in patch #5)
> > > >
> > > > V3:
> > > > + default gdb thread will be the crashing thread, instead of being
> > > > thread '0'
> > > > + synchronise crash cpu and gdb thread context
> > > > + fix bug in gdb_interface, that replaced gdb's output stream,
losing
> > > > output in some cases, such as info threads and extra output in
info
> > > > variables
> > > > + fix 'info threads'
> > > >
> > > > RFC V2:
> > > > - removed patch implementing 'frame', 'up',
'down' in crash
> > > > - updated the cover letter by removing the mention of those
commands
> > > other
> > > > than the respective gdb passthrough
> > > >
> > > > Aditya Gupta (5):
> > > > ppc64: correct gdb passthroughs by implementing
machdep->get_cpu_reg
> > > > remove 'frame' from prohibited commands list
> > > > synchronise cpu context changes between crash/gdb
> > > > fix gdb_interface: restore gdb's output streams at end of
> > > > gdb_interface
> > > > fix 'info threads' command
> > > >
> > > > crash_target.c | 44 ++++++++++++++++
> > > > defs.h | 130
+++++++++++++++++++++++++++++++++++++++++++++++-
> > > > gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
> > > > gdb_interface.c | 2 +-
> > > > kernel.c | 47 +++++++++++++++--
> > > > ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> > > > task.c | 14 ++++++
> > > > tools.c | 2 +-
> > > > 8 files changed, 434 insertions(+), 10 deletions(-)
> > > >
> > > > --
> > > > 2.41.0
> > > >
> > >
> > >
3:53 a.m.
Hi Lianbo and Tao,
On Tue, Feb 27, 2024 at 11:27:32AM +0800, lijiang wrote:
Hi, Aditya
On Mon, Feb 26, 2024 at 7:12 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
> Hi Lianbo,
> On Thu, Feb 22, 2024 at 03:59:36PM +0530, Aditya Gupta wrote:
> > Hi Lianbo,
> > On Thu, Feb 22, 2024 at 05:16:31PM +0800, lijiang wrote:
> > > Hi, Aditya and Tao
> > > Thank you for working on this.
> > >
> > > I would suggest that the current feature can be splitted into several
> > > steps, for example:
> > > [1] complete a basic functionality on ppc64
> > > [2] improve it based on [1], such known issues
> > > [3] after finishing the [1](and [2]), Tao will implement it on X86 64
> > >
> > > That can avoid confusion(or dependency) between ppc64 and X86 64
> patches,
> > > it may save some time and make it easier to review patches.
> >
> > Sure, actually I made a mistake in v9, other than that there should be
> > no dependency in this patch series.
> > This is the summary:
> >
> > Patch #1: PPC64 specific
> > Patch #2 - #5: Architecture Independent, with patch #4 fixing an
> > existing bug in gdb_interface, this can be an independent patch.
> >
> > So based on your suggestions, let's split into it as:
> >
> > 1. This patch series having Patch #1,2,3,5 (removed 4)
> > 2. Fix for existing bug in gdb_interface: Patch #4, It will be good if
> > this can be merged soon, as testing 'info threads' will not work due
to
> > the bug
> > 3. Tao's patches for x86_64
> >
> > Also, even though i have kept #2-#5 as architecture-independent, but
> > then they should be merged after the ppc patches, for their description
> > and usage to make sense, so I still kept them in the series.
>
> Any comments on this ?
>
> I am still of the view it makes sense for all 5 patches to go in as a
> single series, since independently they might not make much sense (such
> as, even though patch #4 fixes an existing bug, but info threads will
> not hit the bug unless the architecture supports gdb mode unwinding).
>
> But if splitting it makes more sense, I am okay with it.
>
>
Sorry about this, I should explain it in detail.
Earlier I saw you were discussing with Tao about the following changes:
"E.g. I added a new member "bool need_free" for defs.h:struct bt_info
in the "x86 unwind support" patch of mine, however you didn't pick the
one. And in ppc64.c:ppc64_get_stack_frame() and
ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
if (bt_info.need_free) {
FREEBUF(pt_regs);
bt_info.need_free = FALSE;
}
"
So I would suggest avoiding confusion between ppc64 and x86 64 patches, and
split into several steps. Also do not put X86 64 changes into ppc64 series.
Sorry for the confusion, I will keep the patch series to how it was at
v8, plus fix tao did in that. That will require 'need_free' part tao
added in patch #1, other than that I will not add any x86 changes into
ppc64.
Thanks,
Aditya Gupta
>
> Thanks.
> Lianbo
>
> Thanks,
> > Aditya Gupta
> >
> > >
> > > Do let me know, I will send a v10 accordingly :)
> > >
> > > Thanks,
> > > Aditya Gupta
> > >
> > > >
> > > > Thanks.
> > > > Lianbo
> > > >
> > > > On Thu, Feb 22, 2024 at 4:58 PM Tao Liu <ltao(a)redhat.com>
wrote:
> > > >
> > > > > Hi Aditya,
> > > > >
> > > > > On Thu, Feb 22, 2024 at 1:23 PM Aditya Gupta
<adityag(a)linux.ibm.com>
> > > > > wrote:
> > > > > >
> > > > > > The Problem:
> > > > > > ============
> > > > > >
> > > > > > Currently crash is unable to show function arguments and
local
> > > > > variables, as
> > > > > > gdb can do. And functionality for moving between frames
> > ('up'/'down') is
> > > > > not
> > > > > > working in crash.
> > > > > >
> > > > > > Crash has 'gdb passthroughs' for things gdb can do,
but the gdb
> > > > > passthroughs
> > > > > > 'bt', 'frame', 'info locals',
'up', 'down' are not working either,
> > due to
> > > > > > gdb not getting the register values from
> > `crash_target::fetch_registers`,
> > > > > > which then uses `machdep->get_cpu_reg`, which is not
implemented
> > for
> > > > > PPC64
> > > > > >
> > > > > > Proposed Solution:
> > > > > > ==================
> > > > > >
> > > > > > Fix the gdb passthroughs by implementing
"machdep->get_cpu_reg" for
> > > > > PPC64.
> > > > > > This way, "gdb mode in crash" will support this
feature for both
> > ELF and
> > > > > > kdump-compressed vmcore formats, while "gdb" would
only have
> > supported
> > > > > ELF
> > > > > > format
> > > > > >
> > > > > > This way other features of 'gdb', such as seeing
> > > > > > backtraces/registers/variables/arguments/local variables,
moving
> > up and
> > > > > > down stack frames, can be used with any ppc64 vmcore,
irrespective
> > of
> > > > > > being ELF format or kdump-compressed format.
> > > > > >
> > > > > > Note: This doesn't support live debugging on ppc64,
since
> > registers are
> > > > > not
> > > > > > available to be read
> > > > > >
> > > > > > Implications on Architectures:
> > > > > > ====================================
> > > > > >
> > > > > > No architecture other than PPC64 has been affected, other
than in
> > case of
> > > > > > 'frame' command
> > > > > >
> > > > > > As mentioned in patch #2, since frame will not be
prohibited, so
> > it will
> > > > > print:
> > > > > >
> > > > > > crash> frame
> > > > > > #0 <unavailable> in ?? ()
> > > > > >
> > > > > > Instead of before prohibited message:
> > > > > >
> > > > > > crash> frame
> > > > > > crash: prohibited gdb command: frame
> > > > > >
> > > > > > Major change will be in 'gdb mode' on PPC64, that it
will print the
> > > > > frames, and
> > > > > > local variables, instead of failing with errors showing no
frame,
> > or
> > > > > showing
> > > > > > that couldn't get PC, it will be able to give all this
information.
> > > > > >
> > > > > > Testing:
> > > > > > ========
> > > > > >
> > > > > > Git tree with this patch series applied:
> > > > > > https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
> > > > >
> > > > > I doubt the v9 patch will not work without my x86's trial
patchset. I
> > > > > see your repo, you directly applied my "ppc64 arbitrary task
stack
> > > > > unwind support" patch onto yours. However the patch has
some
> > > > > dependency on my x86 trial patchset.
> > > > >
> > > > > E.g. I added a new member "bool need_free" for
defs.h:struct bt_info
> > > > > in the "x86 unwind support" patch of mine, however you
didn't pick
> > the
> > > > > one. And in ppc64.c:ppc64_get_stack_frame() and
> > > > > ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
> > > > >
> > > > > if (bt_info.need_free) {
> > > > > FREEBUF(pt_regs);
> > > > > bt_info.need_free = FALSE;
> > > > > }
> > > > >
> > > > > So I guess (not tried yet) that your patchset v9 will not work.
> > > > >
> > > > > Currently I'm still struggling with some failing cases of
x86_64
> > > > > unwinding. So I didn't arrange my patchsets, along with the
patch
> > > > > commit log well, since they are all "trial" patches.
> > > > >
> > > > > I agree the patch "ppc64 arbitrary task stack unwind
support" is
> > > > > better to go with the ppc patch series. But I suggest we make
some
> > > > > modifications for it:
> > > > >
> > > > > 1) I'm OK with it being a stand alone patch, or merging the
code
> > > > > changes of this one into your previous patches, but I prefer the
> > > > > latter one :)
> > > > >
> > > > > 2) If you'd like to go with a stand alone patch, could you
please
> > > > > rewrite a commit log and title for this one?
> > > > >
> > > > > Thanks,
> > > > > Tao Liu
> > > > >
> > > > >
> > > > > >
> > > > > > To test various gdb passthroughs:
> > > > > >
> > > > > > (crash) set
> > > > > > (crash) set gdb on
> > > > > > gdb> thread
> > > > > > gdb> bt
> > > > > > gdb> info threads
> > > > > > gdb> info threads
> > > > > > gdb> info locals
> > > > > > gdb> info variables irq_rover_lock
> > > > > > gdb> info args
> > > > > > gdb> thread 2
> > > > > > gdb> set gdb off
> > > > > > (crash) set
> > > > > > (crash) set -c 6
> > > > > > (crash) gdb thread
> > > > > > (crash) bt
> > > > > > (crash) gdb bt
> > > > > > (crash) frame
> > > > > > (crash) gdb up
> > > > > > (crash) gdb down
> > > > > > (crash) info locals
> > > > > >
> > > > > > Known Issues:
> > > > > > =============
> > > > > >
> > > > > > 1. In gdb mode, 'bt' might fail to show backtrace in
few vmcores
> > > > > collected
> > > > > > from older kernels. This is a known issue due to
register
> > mismatch,
> > > > > and
> > > > > > its fix has been merged upstream:
> > > > > >
> > > > > > This can also cause some 'invalid kernel virtual
address' errors
> > > > > during gdb
> > > > > > unwinding the stack registers
> > > > > >
> > > > > > Commit:
> > > > >
> >
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
> > > > > >
> > > > > > Fixing GDB passthroughs on other architectures
> > > > > > ==============================================
> > > > > >
> > > > > > Much of the work for making gdb passthroughs like 'gdb
bt', 'gdb
> > > > > > thread', 'gdb info locals' etc. has been done by
the patches
> > introducing
> > > > > > 'machdep->get_cpu_reg' and this series fixing
some issues in that.
> > > > > >
> > > > > > Other architectures should be able to fix these gdb
> > functionalities by
> > > > > > simply implementing 'machdep->get_cpu_reg (cpu,
regno, ...)'.
> > > > > >
> > > > > > The reasoning behind that has been explained with a diagram
in
> > commit
> > > > > > description of patch #1
> > > > > >
> > > > > > I will assist with my findings/observations fixing it on
ppc64
> > whenever
> > > > > needed.
> > > > > >
> > > > > > Changelog:
> > > > > > ==========
> > > > > >
> > > > > > V9:
> > > > > > + minor change in patch #5: sync gdb context on a
'set' and 'set
> > -p'
> > > > > > + add taoliu's patch for using current context, and
fixes in
> > > > > ppc64_get_cpu_reg
> > > > > >
> > > > > > V8:
> > > > > > + use get_active_task instead of depending on
CURRENT_CONTEXT in
> > > > > ppc64_get_cpu_reg
> > > > > > + rebase to upstream/master (5977936c0a91)
> > > > > >
> > > > > > V7:
> > > > > > + move changes in gdb-10.2.patch to the end (minor change in
patch
> > > > > #3,4,5)
> > > > > > + fix a memory leak in ppc64_get_cpu_reg (minor change in
patch #1)
> > > > > > + use ascii diagram in patch #1 description
> > > > > >
> > > > > > V6:
> > > > > > + changes in patch #5: fix bug introduced in v5 that caused
> > initial gdb
> > > > > thread
> > > > > > to be thread 1
> > > > > >
> > > > > > V5:
> > > > > > + changes in patch #1: made ppc64_get_cpu_reg static, and
remove
> > > > > unreachable
> > > > > > code
> > > > > > + changes in patch #3: fixed typo 'ppc64_renum'
instead of
> > > > > 'ppc64_regnum',
> > > > > > remove unneeded if condition
> > > > > > + changes in patch #5: implement refresh regcache on per
thread,
> > instead
> > > > > of all
> > > > > > threads at once
> > > > > >
> > > > > > V4:
> > > > > > + fix segmentation fault in live debugging (change in patch
#1)
> > > > > > + mention live debugging not supported in cover letter and
patch #1
> > > > > > + fixed some checkpatch warnings (change in patch #5)
> > > > > >
> > > > > > V3:
> > > > > > + default gdb thread will be the crashing thread, instead of
being
> > > > > > thread '0'
> > > > > > + synchronise crash cpu and gdb thread context
> > > > > > + fix bug in gdb_interface, that replaced gdb's output
stream,
> > losing
> > > > > > output in some cases, such as info threads and extra
output in
> > info
> > > > > > variables
> > > > > > + fix 'info threads'
> > > > > >
> > > > > > RFC V2:
> > > > > > - removed patch implementing 'frame',
'up', 'down' in crash
> > > > > > - updated the cover letter by removing the mention of
those
> > commands
> > > > > other
> > > > > > than the respective gdb passthrough
> > > > > >
> > > > > > Aditya Gupta (5):
> > > > > > ppc64: correct gdb passthroughs by implementing
> > machdep->get_cpu_reg
> > > > > > remove 'frame' from prohibited commands list
> > > > > > synchronise cpu context changes between crash/gdb
> > > > > > fix gdb_interface: restore gdb's output streams at end
of
> > > > > > gdb_interface
> > > > > > fix 'info threads' command
> > > > > >
> > > > > > crash_target.c | 44 ++++++++++++++++
> > > > > > defs.h | 130
> > +++++++++++++++++++++++++++++++++++++++++++++++-
> > > > > > gdb-10.2.patch | 110
+++++++++++++++++++++++++++++++++++++++-
> > > > > > gdb_interface.c | 2 +-
> > > > > > kernel.c | 47 +++++++++++++++--
> > > > > > ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> > > > > > task.c | 14 ++++++
> > > > > > tools.c | 2 +-
> > > > > > 8 files changed, 434 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > --
> > > > > > 2.41.0
> > > > > >
> > > > >
> > > > >
> >
> >
10:19 p.m.
Hi Aditya,
On Fri, Mar 1, 2024 at 5:53 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
Hi Lianbo and Tao,
On Tue, Feb 27, 2024 at 11:27:32AM +0800, lijiang wrote:
> Hi, Aditya
> On Mon, Feb 26, 2024 at 7:12 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
> > Hi Lianbo,
> > On Thu, Feb 22, 2024 at 03:59:36PM +0530, Aditya Gupta wrote:
> > > Hi Lianbo,
> > > On Thu, Feb 22, 2024 at 05:16:31PM +0800, lijiang wrote:
> > > > Hi, Aditya and Tao
> > > > Thank you for working on this.
> > > >
> > > > I would suggest that the current feature can be splitted into
several
> > > > steps, for example:
> > > > [1] complete a basic functionality on ppc64
> > > > [2] improve it based on [1], such known issues
> > > > [3] after finishing the [1](and [2]), Tao will implement it on X86
64
> > > >
> > > > That can avoid confusion(or dependency) between ppc64 and X86 64
> > patches,
> > > > it may save some time and make it easier to review patches.
> > >
> > > Sure, actually I made a mistake in v9, other than that there should be
> > > no dependency in this patch series.
> > > This is the summary:
> > >
> > > Patch #1: PPC64 specific
> > > Patch #2 - #5: Architecture Independent, with patch #4 fixing an
> > > existing bug in gdb_interface, this can be an independent patch.
> > >
> > > So based on your suggestions, let's split into it as:
> > >
> > > 1. This patch series having Patch #1,2,3,5 (removed 4)
> > > 2. Fix for existing bug in gdb_interface: Patch #4, It will be good if
> > > this can be merged soon, as testing 'info threads' will not work
due to
> > > the bug
> > > 3. Tao's patches for x86_64
> > >
> > > Also, even though i have kept #2-#5 as architecture-independent, but
> > > then they should be merged after the ppc patches, for their description
> > > and usage to make sense, so I still kept them in the series.
> >
> > Any comments on this ?
> >
> > I am still of the view it makes sense for all 5 patches to go in as a
> > single series, since independently they might not make much sense (such
> > as, even though patch #4 fixes an existing bug, but info threads will
> > not hit the bug unless the architecture supports gdb mode unwinding).
> >
> > But if splitting it makes more sense, I am okay with it.
> >
> >
> Sorry about this, I should explain it in detail.
>
> Earlier I saw you were discussing with Tao about the following changes:
>
> "E.g. I added a new member "bool need_free" for defs.h:struct
bt_info
> in the "x86 unwind support" patch of mine, however you didn't pick
the
> one. And in ppc64.c:ppc64_get_stack_frame() and
> ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
>
> if (bt_info.need_free) {
> FREEBUF(pt_regs);
> bt_info.need_free = FALSE;
> }
> "
>
> So I would suggest avoiding confusion between ppc64 and x86 64 patches, and
> split into several steps. Also do not put X86 64 changes into ppc64 series.
Sorry for the confusion, I will keep the patch series to how it was at
v8, plus fix tao did in that. That will require 'need_free' part tao
added in patch #1, other than that I will not add any x86 changes into
ppc64.
Will you post your ppc series v10? Because I think my x86 series is
almost ready, so I plan to rebase mine onto your v10, and post it
upstream for discussion. Please let me know your plans. Thanks in
advance!
Thanks,
Tao Liu
Thanks,
Aditya Gupta
>
> Thanks.
> Lianbo
>
> Thanks,
> > Aditya Gupta
> >
> > >
> > > Do let me know, I will send a v10 accordingly :)
> > >
> > > Thanks,
> > > Aditya Gupta
> > >
> > > >
> > > > Thanks.
> > > > Lianbo
> > > >
> > > > On Thu, Feb 22, 2024 at 4:58 PM Tao Liu <ltao(a)redhat.com>
wrote:
> > > >
> > > > > Hi Aditya,
> > > > >
> > > > > On Thu, Feb 22, 2024 at 1:23 PM Aditya Gupta
<adityag(a)linux.ibm.com>
> > > > > wrote:
> > > > > >
> > > > > > The Problem:
> > > > > > ============
> > > > > >
> > > > > > Currently crash is unable to show function arguments and
local
> > > > > variables, as
> > > > > > gdb can do. And functionality for moving between frames
> > ('up'/'down') is
> > > > > not
> > > > > > working in crash.
> > > > > >
> > > > > > Crash has 'gdb passthroughs' for things gdb can do,
but the gdb
> > > > > passthroughs
> > > > > > 'bt', 'frame', 'info locals',
'up', 'down' are not working either,
> > due to
> > > > > > gdb not getting the register values from
> > `crash_target::fetch_registers`,
> > > > > > which then uses `machdep->get_cpu_reg`, which is not
implemented
> > for
> > > > > PPC64
> > > > > >
> > > > > > Proposed Solution:
> > > > > > ==================
> > > > > >
> > > > > > Fix the gdb passthroughs by implementing
"machdep->get_cpu_reg" for
> > > > > PPC64.
> > > > > > This way, "gdb mode in crash" will support this
feature for both
> > ELF and
> > > > > > kdump-compressed vmcore formats, while "gdb"
would only have
> > supported
> > > > > ELF
> > > > > > format
> > > > > >
> > > > > > This way other features of 'gdb', such as seeing
> > > > > > backtraces/registers/variables/arguments/local variables,
moving
> > up and
> > > > > > down stack frames, can be used with any ppc64 vmcore,
irrespective
> > of
> > > > > > being ELF format or kdump-compressed format.
> > > > > >
> > > > > > Note: This doesn't support live debugging on ppc64,
since
> > registers are
> > > > > not
> > > > > > available to be read
> > > > > >
> > > > > > Implications on Architectures:
> > > > > > ====================================
> > > > > >
> > > > > > No architecture other than PPC64 has been affected, other
than in
> > case of
> > > > > > 'frame' command
> > > > > >
> > > > > > As mentioned in patch #2, since frame will not be
prohibited, so
> > it will
> > > > > print:
> > > > > >
> > > > > > crash> frame
> > > > > > #0 <unavailable> in ?? ()
> > > > > >
> > > > > > Instead of before prohibited message:
> > > > > >
> > > > > > crash> frame
> > > > > > crash: prohibited gdb command: frame
> > > > > >
> > > > > > Major change will be in 'gdb mode' on PPC64, that
it will print the
> > > > > frames, and
> > > > > > local variables, instead of failing with errors showing no
frame,
> > or
> > > > > showing
> > > > > > that couldn't get PC, it will be able to give all this
information.
> > > > > >
> > > > > > Testing:
> > > > > > ========
> > > > > >
> > > > > > Git tree with this patch series applied:
> > > > > > https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
> > > > >
> > > > > I doubt the v9 patch will not work without my x86's trial
patchset. I
> > > > > see your repo, you directly applied my "ppc64 arbitrary
task stack
> > > > > unwind support" patch onto yours. However the patch has
some
> > > > > dependency on my x86 trial patchset.
> > > > >
> > > > > E.g. I added a new member "bool need_free" for
defs.h:struct bt_info
> > > > > in the "x86 unwind support" patch of mine, however you
didn't pick
> > the
> > > > > one. And in ppc64.c:ppc64_get_stack_frame() and
> > > > > ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
> > > > >
> > > > > if (bt_info.need_free) {
> > > > > FREEBUF(pt_regs);
> > > > > bt_info.need_free = FALSE;
> > > > > }
> > > > >
> > > > > So I guess (not tried yet) that your patchset v9 will not work.
> > > > >
> > > > > Currently I'm still struggling with some failing cases of
x86_64
> > > > > unwinding. So I didn't arrange my patchsets, along with the
patch
> > > > > commit log well, since they are all "trial" patches.
> > > > >
> > > > > I agree the patch "ppc64 arbitrary task stack unwind
support" is
> > > > > better to go with the ppc patch series. But I suggest we make
some
> > > > > modifications for it:
> > > > >
> > > > > 1) I'm OK with it being a stand alone patch, or merging the
code
> > > > > changes of this one into your previous patches, but I prefer
the
> > > > > latter one :)
> > > > >
> > > > > 2) If you'd like to go with a stand alone patch, could you
please
> > > > > rewrite a commit log and title for this one?
> > > > >
> > > > > Thanks,
> > > > > Tao Liu
> > > > >
> > > > >
> > > > > >
> > > > > > To test various gdb passthroughs:
> > > > > >
> > > > > > (crash) set
> > > > > > (crash) set gdb on
> > > > > > gdb> thread
> > > > > > gdb> bt
> > > > > > gdb> info threads
> > > > > > gdb> info threads
> > > > > > gdb> info locals
> > > > > > gdb> info variables irq_rover_lock
> > > > > > gdb> info args
> > > > > > gdb> thread 2
> > > > > > gdb> set gdb off
> > > > > > (crash) set
> > > > > > (crash) set -c 6
> > > > > > (crash) gdb thread
> > > > > > (crash) bt
> > > > > > (crash) gdb bt
> > > > > > (crash) frame
> > > > > > (crash) gdb up
> > > > > > (crash) gdb down
> > > > > > (crash) info locals
> > > > > >
> > > > > > Known Issues:
> > > > > > =============
> > > > > >
> > > > > > 1. In gdb mode, 'bt' might fail to show backtrace
in few vmcores
> > > > > collected
> > > > > > from older kernels. This is a known issue due to
register
> > mismatch,
> > > > > and
> > > > > > its fix has been merged upstream:
> > > > > >
> > > > > > This can also cause some 'invalid kernel virtual
address' errors
> > > > > during gdb
> > > > > > unwinding the stack registers
> > > > > >
> > > > > > Commit:
> > > > >
> >
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
> > > > > >
> > > > > > Fixing GDB passthroughs on other architectures
> > > > > > ==============================================
> > > > > >
> > > > > > Much of the work for making gdb passthroughs like 'gdb
bt', 'gdb
> > > > > > thread', 'gdb info locals' etc. has been done
by the patches
> > introducing
> > > > > > 'machdep->get_cpu_reg' and this series fixing
some issues in that.
> > > > > >
> > > > > > Other architectures should be able to fix these gdb
> > functionalities by
> > > > > > simply implementing 'machdep->get_cpu_reg (cpu,
regno, ...)'.
> > > > > >
> > > > > > The reasoning behind that has been explained with a diagram
in
> > commit
> > > > > > description of patch #1
> > > > > >
> > > > > > I will assist with my findings/observations fixing it on
ppc64
> > whenever
> > > > > needed.
> > > > > >
> > > > > > Changelog:
> > > > > > ==========
> > > > > >
> > > > > > V9:
> > > > > > + minor change in patch #5: sync gdb context on a
'set' and 'set
> > -p'
> > > > > > + add taoliu's patch for using current context, and
fixes in
> > > > > ppc64_get_cpu_reg
> > > > > >
> > > > > > V8:
> > > > > > + use get_active_task instead of depending on
CURRENT_CONTEXT in
> > > > > ppc64_get_cpu_reg
> > > > > > + rebase to upstream/master (5977936c0a91)
> > > > > >
> > > > > > V7:
> > > > > > + move changes in gdb-10.2.patch to the end (minor change
in patch
> > > > > #3,4,5)
> > > > > > + fix a memory leak in ppc64_get_cpu_reg (minor change in
patch #1)
> > > > > > + use ascii diagram in patch #1 description
> > > > > >
> > > > > > V6:
> > > > > > + changes in patch #5: fix bug introduced in v5 that
caused
> > initial gdb
> > > > > thread
> > > > > > to be thread 1
> > > > > >
> > > > > > V5:
> > > > > > + changes in patch #1: made ppc64_get_cpu_reg static, and
remove
> > > > > unreachable
> > > > > > code
> > > > > > + changes in patch #3: fixed typo 'ppc64_renum'
instead of
> > > > > 'ppc64_regnum',
> > > > > > remove unneeded if condition
> > > > > > + changes in patch #5: implement refresh regcache on per
thread,
> > instead
> > > > > of all
> > > > > > threads at once
> > > > > >
> > > > > > V4:
> > > > > > + fix segmentation fault in live debugging (change in patch
#1)
> > > > > > + mention live debugging not supported in cover letter and
patch #1
> > > > > > + fixed some checkpatch warnings (change in patch #5)
> > > > > >
> > > > > > V3:
> > > > > > + default gdb thread will be the crashing thread, instead
of being
> > > > > > thread '0'
> > > > > > + synchronise crash cpu and gdb thread context
> > > > > > + fix bug in gdb_interface, that replaced gdb's output
stream,
> > losing
> > > > > > output in some cases, such as info threads and extra
output in
> > info
> > > > > > variables
> > > > > > + fix 'info threads'
> > > > > >
> > > > > > RFC V2:
> > > > > > - removed patch implementing 'frame',
'up', 'down' in crash
> > > > > > - updated the cover letter by removing the mention of
those
> > commands
> > > > > other
> > > > > > than the respective gdb passthrough
> > > > > >
> > > > > > Aditya Gupta (5):
> > > > > > ppc64: correct gdb passthroughs by implementing
> > machdep->get_cpu_reg
> > > > > > remove 'frame' from prohibited commands list
> > > > > > synchronise cpu context changes between crash/gdb
> > > > > > fix gdb_interface: restore gdb's output streams at
end of
> > > > > > gdb_interface
> > > > > > fix 'info threads' command
> > > > > >
> > > > > > crash_target.c | 44 ++++++++++++++++
> > > > > > defs.h | 130
> > +++++++++++++++++++++++++++++++++++++++++++++++-
> > > > > > gdb-10.2.patch | 110
+++++++++++++++++++++++++++++++++++++++-
> > > > > > gdb_interface.c | 2 +-
> > > > > > kernel.c | 47 +++++++++++++++--
> > > > > > ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> > > > > > task.c | 14 ++++++
> > > > > > tools.c | 2 +-
> > > > > > 8 files changed, 434 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > --
> > > > > > 2.41.0
> > > > > >
> > > > >
> > > > >
> >
> >
2:08 a.m.
Hi Tao,
On 04/03/24 09:49, Tao Liu wrote:
Hi Aditya,
<...snip...>
Will you post your ppc series v10? Because I think my x86 series is
almost ready, so I plan to rebase mine onto your v10, and post it
upstream for discussion. Please let me know your plans. Thanks in
advance!
Yes Tao, I will post it soon today.
Thanks,
Aditya Gupta
Thanks,
Tao Liu
> Thanks,
> Aditya Gupta
>
>> Thanks.
>> Lianbo
>>
>> Thanks,
>>> Aditya Gupta
>>>
>>>> Do let me know, I will send a v10 accordingly :)
>>>>
>>>> Thanks,
>>>> Aditya Gupta
>>>>
>>>>> Thanks.
>>>>> Lianbo
>>>>>
>>>>> On Thu, Feb 22, 2024 at 4:58 PM Tao Liu<ltao(a)redhat.com>
wrote:
>>>>>
>>>>>> Hi Aditya,
>>>>>>
>>>>>> On Thu, Feb 22, 2024 at 1:23 PM Aditya
Gupta<adityag(a)linux.ibm.com>
>>>>>> wrote:
>>>>>>> The Problem:
>>>>>>> ============
>>>>>>>
>>>>>>> Currently crash is unable to show function arguments and
local
>>>>>> variables, as
>>>>>>> gdb can do. And functionality for moving between frames
>>> ('up'/'down') is
>>>>>> not
>>>>>>> working in crash.
>>>>>>>
>>>>>>> Crash has 'gdb passthroughs' for things gdb can do,
but the gdb
>>>>>> passthroughs
>>>>>>> 'bt', 'frame', 'info locals',
'up', 'down' are not working either,
>>> due to
>>>>>>> gdb not getting the register values from
>>> `crash_target::fetch_registers`,
>>>>>>> which then uses `machdep->get_cpu_reg`, which is not
implemented
>>> for
>>>>>> PPC64
>>>>>>> Proposed Solution:
>>>>>>> ==================
>>>>>>>
>>>>>>> Fix the gdb passthroughs by implementing
"machdep->get_cpu_reg" for
>>>>>> PPC64.
>>>>>>> This way, "gdb mode in crash" will support this
feature for both
>>> ELF and
>>>>>>> kdump-compressed vmcore formats, while "gdb" would
only have
>>> supported
>>>>>> ELF
>>>>>>> format
>>>>>>>
>>>>>>> This way other features of 'gdb', such as seeing
>>>>>>> backtraces/registers/variables/arguments/local variables,
moving
>>> up and
>>>>>>> down stack frames, can be used with any ppc64 vmcore,
irrespective
>>> of
>>>>>>> being ELF format or kdump-compressed format.
>>>>>>>
>>>>>>> Note: This doesn't support live debugging on ppc64,
since
>>> registers are
>>>>>> not
>>>>>>> available to be read
>>>>>>>
>>>>>>> Implications on Architectures:
>>>>>>> ====================================
>>>>>>>
>>>>>>> No architecture other than PPC64 has been affected, other
than in
>>> case of
>>>>>>> 'frame' command
>>>>>>>
>>>>>>> As mentioned in patch #2, since frame will not be prohibited,
so
>>> it will
>>>>>> print:
>>>>>>> crash> frame
>>>>>>> #0 <unavailable> in ?? ()
>>>>>>>
>>>>>>> Instead of before prohibited message:
>>>>>>>
>>>>>>> crash> frame
>>>>>>> crash: prohibited gdb command: frame
>>>>>>>
>>>>>>> Major change will be in 'gdb mode' on PPC64, that it
will print the
>>>>>> frames, and
>>>>>>> local variables, instead of failing with errors showing no
frame,
>>> or
>>>>>> showing
>>>>>>> that couldn't get PC, it will be able to give all this
information.
>>>>>>>
>>>>>>> Testing:
>>>>>>> ========
>>>>>>>
>>>>>>> Git tree with this patch series applied:
>>>>>>> https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
>>>>>> I doubt the v9 patch will not work without my x86's trial
patchset. I
>>>>>> see your repo, you directly applied my "ppc64 arbitrary task
stack
>>>>>> unwind support" patch onto yours. However the patch has
some
>>>>>> dependency on my x86 trial patchset.
>>>>>>
>>>>>> E.g. I added a new member "bool need_free" for
defs.h:struct bt_info
>>>>>> in the "x86 unwind support" patch of mine, however you
didn't pick
>>> the
>>>>>> one. And in ppc64.c:ppc64_get_stack_frame() and
>>>>>> ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
>>>>>>
>>>>>> if (bt_info.need_free) {
>>>>>> FREEBUF(pt_regs);
>>>>>> bt_info.need_free = FALSE;
>>>>>> }
>>>>>>
>>>>>> So I guess (not tried yet) that your patchset v9 will not work.
>>>>>>
>>>>>> Currently I'm still struggling with some failing cases of
x86_64
>>>>>> unwinding. So I didn't arrange my patchsets, along with the
patch
>>>>>> commit log well, since they are all "trial" patches.
>>>>>>
>>>>>> I agree the patch "ppc64 arbitrary task stack unwind
support" is
>>>>>> better to go with the ppc patch series. But I suggest we make
some
>>>>>> modifications for it:
>>>>>>
>>>>>> 1) I'm OK with it being a stand alone patch, or merging the
code
>>>>>> changes of this one into your previous patches, but I prefer the
>>>>>> latter one :)
>>>>>>
>>>>>> 2) If you'd like to go with a stand alone patch, could you
please
>>>>>> rewrite a commit log and title for this one?
>>>>>>
>>>>>> Thanks,
>>>>>> Tao Liu
>>>>>>
>>>>>>
>>>>>>> To test various gdb passthroughs:
>>>>>>>
>>>>>>> (crash) set
>>>>>>> (crash) set gdb on
>>>>>>> gdb> thread
>>>>>>> gdb> bt
>>>>>>> gdb> info threads
>>>>>>> gdb> info threads
>>>>>>> gdb> info locals
>>>>>>> gdb> info variables irq_rover_lock
>>>>>>> gdb> info args
>>>>>>> gdb> thread 2
>>>>>>> gdb> set gdb off
>>>>>>> (crash) set
>>>>>>> (crash) set -c 6
>>>>>>> (crash) gdb thread
>>>>>>> (crash) bt
>>>>>>> (crash) gdb bt
>>>>>>> (crash) frame
>>>>>>> (crash) gdb up
>>>>>>> (crash) gdb down
>>>>>>> (crash) info locals
>>>>>>>
>>>>>>> Known Issues:
>>>>>>> =============
>>>>>>>
>>>>>>> 1. In gdb mode, 'bt' might fail to show backtrace in
few vmcores
>>>>>> collected
>>>>>>> from older kernels. This is a known issue due to
register
>>> mismatch,
>>>>>> and
>>>>>>> its fix has been merged upstream:
>>>>>>>
>>>>>>> This can also cause some 'invalid kernel virtual
address' errors
>>>>>> during gdb
>>>>>>> unwinding the stack registers
>>>>>>>
>>>>>>> Commit:
>>>
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
>>>>>>> Fixing GDB passthroughs on other architectures
>>>>>>> ==============================================
>>>>>>>
>>>>>>> Much of the work for making gdb passthroughs like 'gdb
bt', 'gdb
>>>>>>> thread', 'gdb info locals' etc. has been done by
the patches
>>> introducing
>>>>>>> 'machdep->get_cpu_reg' and this series fixing some
issues in that.
>>>>>>>
>>>>>>> Other architectures should be able to fix these gdb
>>> functionalities by
>>>>>>> simply implementing 'machdep->get_cpu_reg (cpu, regno,
...)'.
>>>>>>>
>>>>>>> The reasoning behind that has been explained with a diagram
in
>>> commit
>>>>>>> description of patch #1
>>>>>>>
>>>>>>> I will assist with my findings/observations fixing it on
ppc64
>>> whenever
>>>>>> needed.
>>>>>>> Changelog:
>>>>>>> ==========
>>>>>>>
>>>>>>> V9:
>>>>>>> + minor change in patch #5: sync gdb context on a
'set' and 'set
>>> -p'
>>>>>>> + add taoliu's patch for using current context, and fixes
in
>>>>>> ppc64_get_cpu_reg
>>>>>>> V8:
>>>>>>> + use get_active_task instead of depending on CURRENT_CONTEXT
in
>>>>>> ppc64_get_cpu_reg
>>>>>>> + rebase to upstream/master (5977936c0a91)
>>>>>>>
>>>>>>> V7:
>>>>>>> + move changes in gdb-10.2.patch to the end (minor change in
patch
>>>>>> #3,4,5)
>>>>>>> + fix a memory leak in ppc64_get_cpu_reg (minor change in
patch #1)
>>>>>>> + use ascii diagram in patch #1 description
>>>>>>>
>>>>>>> V6:
>>>>>>> + changes in patch #5: fix bug introduced in v5 that caused
>>> initial gdb
>>>>>> thread
>>>>>>> to be thread 1
>>>>>>>
>>>>>>> V5:
>>>>>>> + changes in patch #1: made ppc64_get_cpu_reg static, and
remove
>>>>>> unreachable
>>>>>>> code
>>>>>>> + changes in patch #3: fixed typo 'ppc64_renum'
instead of
>>>>>> 'ppc64_regnum',
>>>>>>> remove unneeded if condition
>>>>>>> + changes in patch #5: implement refresh regcache on per
thread,
>>> instead
>>>>>> of all
>>>>>>> threads at once
>>>>>>>
>>>>>>> V4:
>>>>>>> + fix segmentation fault in live debugging (change in patch
#1)
>>>>>>> + mention live debugging not supported in cover letter and
patch #1
>>>>>>> + fixed some checkpatch warnings (change in patch #5)
>>>>>>>
>>>>>>> V3:
>>>>>>> + default gdb thread will be the crashing thread, instead of
being
>>>>>>> thread '0'
>>>>>>> + synchronise crash cpu and gdb thread context
>>>>>>> + fix bug in gdb_interface, that replaced gdb's output
stream,
>>> losing
>>>>>>> output in some cases, such as info threads and extra
output in
>>> info
>>>>>>> variables
>>>>>>> + fix 'info threads'
>>>>>>>
>>>>>>> RFC V2:
>>>>>>> - removed patch implementing 'frame',
'up', 'down' in crash
>>>>>>> - updated the cover letter by removing the mention of
those
>>> commands
>>>>>> other
>>>>>>> than the respective gdb passthrough
>>>>>>>
>>>>>>> Aditya Gupta (5):
>>>>>>> ppc64: correct gdb passthroughs by implementing
>>> machdep->get_cpu_reg
>>>>>>> remove 'frame' from prohibited commands list
>>>>>>> synchronise cpu context changes between crash/gdb
>>>>>>> fix gdb_interface: restore gdb's output streams at end
of
>>>>>>> gdb_interface
>>>>>>> fix 'info threads' command
>>>>>>>
>>>>>>> crash_target.c | 44 ++++++++++++++++
>>>>>>> defs.h | 130
>>> +++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>> gdb-10.2.patch | 110
+++++++++++++++++++++++++++++++++++++++-
>>>>>>> gdb_interface.c | 2 +-
>>>>>>> kernel.c | 47 +++++++++++++++--
>>>>>>> ppc64.c | 95 +++++++++++++++++++++++++++++++++--
>>>>>>> task.c | 14 ++++++
>>>>>>> tools.c | 2 +-
>>>>>>> 8 files changed, 434 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> --
>>>>>>> 2.41.0
>>>>>>>
>>>>>>
>>>
4:17 a.m.
Hi Tao,
On Thu, Feb 22, 2024 at 04:57:23PM +0800, Tao Liu wrote:
Hi Aditya,
On Thu, Feb 22, 2024 at 1:23 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
>
> The Problem:
> ============
>
> Currently crash is unable to show function arguments and local variables, as
> gdb can do. And functionality for moving between frames
('up'/'down') is not
> working in crash.
>
> Crash has 'gdb passthroughs' for things gdb can do, but the gdb
passthroughs
> 'bt', 'frame', 'info locals', 'up', 'down'
are not working either, due to
> gdb not getting the register values from `crash_target::fetch_registers`,
> which then uses `machdep->get_cpu_reg`, which is not implemented for PPC64
>
> Proposed Solution:
> ==================
>
> Fix the gdb passthroughs by implementing "machdep->get_cpu_reg" for
PPC64.
> This way, "gdb mode in crash" will support this feature for both ELF and
> kdump-compressed vmcore formats, while "gdb" would only have supported
ELF
> format
>
> This way other features of 'gdb', such as seeing
> backtraces/registers/variables/arguments/local variables, moving up and
> down stack frames, can be used with any ppc64 vmcore, irrespective of
> being ELF format or kdump-compressed format.
>
> Note: This doesn't support live debugging on ppc64, since registers are not
> available to be read
>
> Implications on Architectures:
> ====================================
>
> No architecture other than PPC64 has been affected, other than in case of
> 'frame' command
>
> As mentioned in patch #2, since frame will not be prohibited, so it will print:
>
> crash> frame
> #0 <unavailable> in ?? ()
>
> Instead of before prohibited message:
>
> crash> frame
> crash: prohibited gdb command: frame
>
> Major change will be in 'gdb mode' on PPC64, that it will print the frames,
and
> local variables, instead of failing with errors showing no frame, or showing
> that couldn't get PC, it will be able to give all this information.
>
> Testing:
> ========
>
> Git tree with this patch series applied:
> https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
I doubt the v9 patch will not work without my x86's trial patchset. I
see your repo, you directly applied my "ppc64 arbitrary task stack
unwind support" patch onto yours. However the patch has some
dependency on my x86 trial patchset.
Sorry, and thanks for the quick mail.
E.g. I added a new member "bool need_free" for defs.h:struct bt_info
in the "x86 unwind support" patch of mine, however you didn't pick the
one. And in ppc64.c:ppc64_get_stack_frame() and
ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
if (bt_info.need_free) {
FREEBUF(pt_regs);
bt_info.need_free = FALSE;
}
So I guess (not tried yet) that your patchset v9 will not work.
True, it will not work. Just checked, I had already applied your patches
also and then that patch, where I tested this series. My mistake, please
skip testing this one.
Currently I'm still struggling with some failing cases of x86_64
unwinding. So I didn't arrange my patchsets, along with the patch
commit log well, since they are all "trial" patches.
I agree the patch "ppc64 arbitrary task stack unwind support" is
better to go with the ppc patch series. But I suggest we make some
modifications for it:
1) I'm OK with it being a stand alone patch, or merging the code
changes of this one into your previous patches, but I prefer the
latter one :)
2) If you'd like to go with a stand alone patch, could you please
rewrite a commit log and title for this one?
Sure, I am okay with either. Let's go with the latter one you suggested,
in that case I will add the line adding 'need_free' in 'bt_info'
structure also, to make that work. By the way, shouldn't it be
'FREEBUF(bt_info.stackbuf)' instead of 'FREEBUF(pt_regs)' ?
Also, should I add you as a co-author of the patch ?
Thanks,
Aditya Gupta
>
> Thanks,
> Tao Liu
>
>
> >
> > To test various gdb passthroughs:
> >
> > (crash) set
> > (crash) set gdb on
> > gdb> thread
> > gdb> bt
> > gdb> info threads
> > gdb> info threads
> > gdb> info locals
> > gdb> info variables irq_rover_lock
> > gdb> info args
> > gdb> thread 2
> > gdb> set gdb off
> > (crash) set
> > (crash) set -c 6
> > (crash) gdb thread
> > (crash) bt
> > (crash) gdb bt
> > (crash) frame
> > (crash) gdb up
> > (crash) gdb down
> > (crash) info locals
> >
> > Known Issues:
> > =============
> >
> > 1. In gdb mode, 'bt' might fail to show backtrace in few vmcores
collected
> > from older kernels. This is a known issue due to register mismatch, and
> > its fix has been merged upstream:
> >
> > This can also cause some 'invalid kernel virtual address' errors
during gdb
> > unwinding the stack registers
> >
> > Commit:
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
> >
> > Fixing GDB passthroughs on other architectures
> > ==============================================
> >
> > Much of the work for making gdb passthroughs like 'gdb bt', 'gdb
> > thread', 'gdb info locals' etc. has been done by the patches
introducing
> > 'machdep->get_cpu_reg' and this series fixing some issues in that.
> >
> > Other architectures should be able to fix these gdb functionalities by
> > simply implementing 'machdep->get_cpu_reg (cpu, regno, ...)'.
> >
> > The reasoning behind that has been explained with a diagram in commit
> > description of patch #1
> >
> > I will assist with my findings/observations fixing it on ppc64 whenever needed.
> >
> > Changelog:
> > ==========
> >
> > V9:
> > + minor change in patch #5: sync gdb context on a 'set' and 'set
-p'
> > + add taoliu's patch for using current context, and fixes in
ppc64_get_cpu_reg
> >
> > V8:
> > + use get_active_task instead of depending on CURRENT_CONTEXT in
ppc64_get_cpu_reg
> > + rebase to upstream/master (5977936c0a91)
> >
> > V7:
> > + move changes in gdb-10.2.patch to the end (minor change in patch #3,4,5)
> > + fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
> > + use ascii diagram in patch #1 description
> >
> > V6:
> > + changes in patch #5: fix bug introduced in v5 that caused initial gdb thread
> > to be thread 1
> >
> > V5:
> > + changes in patch #1: made ppc64_get_cpu_reg static, and remove unreachable
> > code
> > + changes in patch #3: fixed typo 'ppc64_renum' instead of
'ppc64_regnum',
> > remove unneeded if condition
> > + changes in patch #5: implement refresh regcache on per thread, instead of all
> > threads at once
> >
> > V4:
> > + fix segmentation fault in live debugging (change in patch #1)
> > + mention live debugging not supported in cover letter and patch #1
> > + fixed some checkpatch warnings (change in patch #5)
> >
> > V3:
> > + default gdb thread will be the crashing thread, instead of being
> > thread '0'
> > + synchronise crash cpu and gdb thread context
> > + fix bug in gdb_interface, that replaced gdb's output stream, losing
> > output in some cases, such as info threads and extra output in info
> > variables
> > + fix 'info threads'
> >
> > RFC V2:
> > - removed patch implementing 'frame', 'up', 'down' in
crash
> > - updated the cover letter by removing the mention of those commands other
> > than the respective gdb passthrough
> >
> > Aditya Gupta (5):
> > ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
> > remove 'frame' from prohibited commands list
> > synchronise cpu context changes between crash/gdb
> > fix gdb_interface: restore gdb's output streams at end of
> > gdb_interface
> > fix 'info threads' command
> >
> > crash_target.c | 44 ++++++++++++++++
> > defs.h | 130 +++++++++++++++++++++++++++++++++++++++++++++++-
> > gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
> > gdb_interface.c | 2 +-
> > kernel.c | 47 +++++++++++++++--
> > ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> > task.c | 14 ++++++
> > tools.c | 2 +-
> > 8 files changed, 434 insertions(+), 10 deletions(-)
> >
> > --
> > 2.41.0
> >
>
2:26 a.m.
Hi Aditya,
On Thu, Feb 22, 2024 at 6:17 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
Hi Tao,
On Thu, Feb 22, 2024 at 04:57:23PM +0800, Tao Liu wrote:
> Hi Aditya,
>
> On Thu, Feb 22, 2024 at 1:23 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
> >
> > The Problem:
> > ============
> >
> > Currently crash is unable to show function arguments and local variables, as
> > gdb can do. And functionality for moving between frames
('up'/'down') is not
> > working in crash.
> >
> > Crash has 'gdb passthroughs' for things gdb can do, but the gdb
passthroughs
> > 'bt', 'frame', 'info locals', 'up',
'down' are not working either, due to
> > gdb not getting the register values from `crash_target::fetch_registers`,
> > which then uses `machdep->get_cpu_reg`, which is not implemented for PPC64
> >
> > Proposed Solution:
> > ==================
> >
> > Fix the gdb passthroughs by implementing "machdep->get_cpu_reg"
for PPC64.
> > This way, "gdb mode in crash" will support this feature for both ELF
and
> > kdump-compressed vmcore formats, while "gdb" would only have
supported ELF
> > format
> >
> > This way other features of 'gdb', such as seeing
> > backtraces/registers/variables/arguments/local variables, moving up and
> > down stack frames, can be used with any ppc64 vmcore, irrespective of
> > being ELF format or kdump-compressed format.
> >
> > Note: This doesn't support live debugging on ppc64, since registers are
not
> > available to be read
> >
> > Implications on Architectures:
> > ====================================
> >
> > No architecture other than PPC64 has been affected, other than in case of
> > 'frame' command
> >
> > As mentioned in patch #2, since frame will not be prohibited, so it will
print:
> >
> > crash> frame
> > #0 <unavailable> in ?? ()
> >
> > Instead of before prohibited message:
> >
> > crash> frame
> > crash: prohibited gdb command: frame
> >
> > Major change will be in 'gdb mode' on PPC64, that it will print the
frames, and
> > local variables, instead of failing with errors showing no frame, or showing
> > that couldn't get PC, it will be able to give all this information.
> >
> > Testing:
> > ========
> >
> > Git tree with this patch series applied:
> > https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
>
> I doubt the v9 patch will not work without my x86's trial patchset. I
> see your repo, you directly applied my "ppc64 arbitrary task stack
> unwind support" patch onto yours. However the patch has some
> dependency on my x86 trial patchset.
Sorry, and thanks for the quick mail.
>
> E.g. I added a new member "bool need_free" for defs.h:struct bt_info
> in the "x86 unwind support" patch of mine, however you didn't pick
the
> one. And in ppc64.c:ppc64_get_stack_frame() and
> ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
>
> if (bt_info.need_free) {
> FREEBUF(pt_regs);
> bt_info.need_free = FALSE;
> }
>
> So I guess (not tried yet) that your patchset v9 will not work.
True, it will not work. Just checked, I had already applied your patches
also and then that patch, where I tested this series. My mistake, please
skip testing this one.
>
> Currently I'm still struggling with some failing cases of x86_64
> unwinding. So I didn't arrange my patchsets, along with the patch
> commit log well, since they are all "trial" patches.
>
> I agree the patch "ppc64 arbitrary task stack unwind support" is
> better to go with the ppc patch series. But I suggest we make some
> modifications for it:
>
> 1) I'm OK with it being a stand alone patch, or merging the code
> changes of this one into your previous patches, but I prefer the
> latter one :)
>
> 2) If you'd like to go with a stand alone patch, could you please
> rewrite a commit log and title for this one?
Sure, I am okay with either. Let's go with the latter one you suggested,
in that case I will add the line adding 'need_free' in 'bt_info'
structure also, to make that work. By the way, shouldn't it be
'FREEBUF(bt_info.stackbuf)' instead of 'FREEBUF(pt_regs)' ?
No it isn't. Let's take a look at the code in
https://github.com/liutgnu/crash-dev/blob/master/x86_64.c#L5006:
x86_64_get_stack_frame():
For non active tasks, bt->machdep = user_regs, user_regs is allocated
here and get the regs value from stack frame. However for active
tasks, bt->machdep is get from get_netdump_regs_x86_64(), the values
are get from vmcore's elf note section.
Then in x86_64_get_cpu_reg(), pt_regs = (struct
x86_64_user_regs_struct *)bt_info.machdep, so the regs value will be
get from pt_regs. And if the pt_regs is allocated in
x86_64_get_stack_frame(), it should be freed. Else if pt_regs are
allocated from get_netdump_regs_x86_64(), then we don't need to do
anything.
So back to your question, ppc64 case is the same as x86_64, so we
should use FREEBUF(pt_regs) here.
In additiion, in
https://github.com/liutgnu/crash-dev/blob/master/ppc64.c#L2509:
ppc64_get_cpu_reg(), there is both 'FREEBUF(bt_info.stackbuf)' and
'FREEBUF(pt_regs)', they deal with different buffer release.
Thanks,
Tao Liu
Also, should I add you as a co-author of the patch ?
Thanks,
Aditya Gupta
>
> Thanks,
> Tao Liu
>
>
> >
> > To test various gdb passthroughs:
> >
> > (crash) set
> > (crash) set gdb on
> > gdb> thread
> > gdb> bt
> > gdb> info threads
> > gdb> info threads
> > gdb> info locals
> > gdb> info variables irq_rover_lock
> > gdb> info args
> > gdb> thread 2
> > gdb> set gdb off
> > (crash) set
> > (crash) set -c 6
> > (crash) gdb thread
> > (crash) bt
> > (crash) gdb bt
> > (crash) frame
> > (crash) gdb up
> > (crash) gdb down
> > (crash) info locals
> >
> > Known Issues:
> > =============
> >
> > 1. In gdb mode, 'bt' might fail to show backtrace in few vmcores
collected
> > from older kernels. This is a known issue due to register mismatch, and
> > its fix has been merged upstream:
> >
> > This can also cause some 'invalid kernel virtual address' errors
during gdb
> > unwinding the stack registers
> >
> > Commit:
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
> >
> > Fixing GDB passthroughs on other architectures
> > ==============================================
> >
> > Much of the work for making gdb passthroughs like 'gdb bt', 'gdb
> > thread', 'gdb info locals' etc. has been done by the patches
introducing
> > 'machdep->get_cpu_reg' and this series fixing some issues in that.
> >
> > Other architectures should be able to fix these gdb functionalities by
> > simply implementing 'machdep->get_cpu_reg (cpu, regno, ...)'.
> >
> > The reasoning behind that has been explained with a diagram in commit
> > description of patch #1
> >
> > I will assist with my findings/observations fixing it on ppc64 whenever
needed.
> >
> > Changelog:
> > ==========
> >
> > V9:
> > + minor change in patch #5: sync gdb context on a 'set' and 'set
-p'
> > + add taoliu's patch for using current context, and fixes in
ppc64_get_cpu_reg
> >
> > V8:
> > + use get_active_task instead of depending on CURRENT_CONTEXT in
ppc64_get_cpu_reg
> > + rebase to upstream/master (5977936c0a91)
> >
> > V7:
> > + move changes in gdb-10.2.patch to the end (minor change in patch #3,4,5)
> > + fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
> > + use ascii diagram in patch #1 description
> >
> > V6:
> > + changes in patch #5: fix bug introduced in v5 that caused initial gdb thread
> > to be thread 1
> >
> > V5:
> > + changes in patch #1: made ppc64_get_cpu_reg static, and remove unreachable
> > code
> > + changes in patch #3: fixed typo 'ppc64_renum' instead of
'ppc64_regnum',
> > remove unneeded if condition
> > + changes in patch #5: implement refresh regcache on per thread, instead of
all
> > threads at once
> >
> > V4:
> > + fix segmentation fault in live debugging (change in patch #1)
> > + mention live debugging not supported in cover letter and patch #1
> > + fixed some checkpatch warnings (change in patch #5)
> >
> > V3:
> > + default gdb thread will be the crashing thread, instead of being
> > thread '0'
> > + synchronise crash cpu and gdb thread context
> > + fix bug in gdb_interface, that replaced gdb's output stream, losing
> > output in some cases, such as info threads and extra output in info
> > variables
> > + fix 'info threads'
> >
> > RFC V2:
> > - removed patch implementing 'frame', 'up', 'down' in
crash
> > - updated the cover letter by removing the mention of those commands other
> > than the respective gdb passthrough
> >
> > Aditya Gupta (5):
> > ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
> > remove 'frame' from prohibited commands list
> > synchronise cpu context changes between crash/gdb
> > fix gdb_interface: restore gdb's output streams at end of
> > gdb_interface
> > fix 'info threads' command
> >
> > crash_target.c | 44 ++++++++++++++++
> > defs.h | 130 +++++++++++++++++++++++++++++++++++++++++++++++-
> > gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
> > gdb_interface.c | 2 +-
> > kernel.c | 47 +++++++++++++++--
> > ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> > task.c | 14 ++++++
> > tools.c | 2 +-
> > 8 files changed, 434 insertions(+), 10 deletions(-)
> >
> > --
> > 2.41.0
> >
>
3:40 a.m.
Hi Tao,
> > <...snip...>
> >
> > Currently I'm still struggling with some failing cases of x86_64
> > unwinding. So I didn't arrange my patchsets, along with the patch
> > commit log well, since they are all "trial" patches.
> >
> > I agree the patch "ppc64 arbitrary task stack unwind support" is
> > better to go with the ppc patch series. But I suggest we make some
> > modifications for it:
> >
> > 1) I'm OK with it being a stand alone patch, or merging the code
> > changes of this one into your previous patches, but I prefer the
> > latter one :)
> >
> > 2) If you'd like to go with a stand alone patch, could you please
> > rewrite a commit log and title for this one?
>
> Sure, I am okay with either. Let's go with the latter one you suggested,
> in that case I will add the line adding 'need_free' in 'bt_info'
> structure also, to make that work. By the way, shouldn't it be
> 'FREEBUF(bt_info.stackbuf)' instead of 'FREEBUF(pt_regs)' ?
No it isn't. Let's take a look at the code in
https://github.com/liutgnu/crash-dev/blob/master/x86_64.c#L5006:
x86_64_get_stack_frame():
For non active tasks, bt->machdep = user_regs, user_regs is allocated
here and get the regs value from stack frame. However for active
tasks, bt->machdep is get from get_netdump_regs_x86_64(), the values
are get from vmcore's elf note section.
Then in x86_64_get_cpu_reg(), pt_regs = (struct
x86_64_user_regs_struct *)bt_info.machdep, so the regs value will be
get from pt_regs. And if the pt_regs is allocated in
x86_64_get_stack_frame(), it should be freed. Else if pt_regs are
allocated from get_netdump_regs_x86_64(), then we don't need to do
anything.
So back to your question, ppc64 case is the same as x86_64, so we
should use FREEBUF(pt_regs) here.
In additiion, in
https://github.com/liutgnu/crash-dev/blob/master/ppc64.c#L2509:
ppc64_get_cpu_reg(), there is both 'FREEBUF(bt_info.stackbuf)' and
'FREEBUF(pt_regs)', they deal with different buffer release.
Thank you for the detailed explanation !
Understood. After reading, went through the code again, it all makes
sense.
Thanks,
Aditya Gupta
>
> Thanks,
> Tao Liu
>
>
>
>
>
> >
> > Also, should I add you as a co-author of the patch ?
> >
> > Thanks,
> > Aditya Gupta
> >
> > >
> > > Thanks,
> > > Tao Liu
> > >
> > >
> > > >
> > > > To test various gdb passthroughs:
> > > >
> > > > (crash) set
> > > > (crash) set gdb on
> > > > gdb> thread
> > > > gdb> bt
> > > > gdb> info threads
> > > > gdb> info threads
> > > > gdb> info locals
> > > > gdb> info variables irq_rover_lock
> > > > gdb> info args
> > > > gdb> thread 2
> > > > gdb> set gdb off
> > > > (crash) set
> > > > (crash) set -c 6
> > > > (crash) gdb thread
> > > > (crash) bt
> > > > (crash) gdb bt
> > > > (crash) frame
> > > > (crash) gdb up
> > > > (crash) gdb down
> > > > (crash) info locals
> > > >
> > > > Known Issues:
> > > > =============
> > > >
> > > > 1. In gdb mode, 'bt' might fail to show backtrace in few
vmcores collected
> > > > from older kernels. This is a known issue due to register mismatch,
and
> > > > its fix has been merged upstream:
> > > >
> > > > This can also cause some 'invalid kernel virtual address'
errors during gdb
> > > > unwinding the stack registers
> > > >
> > > > Commit:
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
> > > >
> > > > Fixing GDB passthroughs on other architectures
> > > > ==============================================
> > > >
> > > > Much of the work for making gdb passthroughs like 'gdb bt',
'gdb
> > > > thread', 'gdb info locals' etc. has been done by the
patches introducing
> > > > 'machdep->get_cpu_reg' and this series fixing some issues
in that.
> > > >
> > > > Other architectures should be able to fix these gdb functionalities
by
> > > > simply implementing 'machdep->get_cpu_reg (cpu, regno,
...)'.
> > > >
> > > > The reasoning behind that has been explained with a diagram in commit
> > > > description of patch #1
> > > >
> > > > I will assist with my findings/observations fixing it on ppc64
whenever needed.
> > > >
> > > > Changelog:
> > > > ==========
> > > >
> > > > V9:
> > > > + minor change in patch #5: sync gdb context on a 'set' and
'set -p'
> > > > + add taoliu's patch for using current context, and fixes in
ppc64_get_cpu_reg
> > > >
> > > > V8:
> > > > + use get_active_task instead of depending on CURRENT_CONTEXT in
ppc64_get_cpu_reg
> > > > + rebase to upstream/master (5977936c0a91)
> > > >
> > > > V7:
> > > > + move changes in gdb-10.2.patch to the end (minor change in patch
#3,4,5)
> > > > + fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
> > > > + use ascii diagram in patch #1 description
> > > >
> > > > V6:
> > > > + changes in patch #5: fix bug introduced in v5 that caused initial
gdb thread
> > > > to be thread 1
> > > >
> > > > V5:
> > > > + changes in patch #1: made ppc64_get_cpu_reg static, and remove
unreachable
> > > > code
> > > > + changes in patch #3: fixed typo 'ppc64_renum' instead of
'ppc64_regnum',
> > > > remove unneeded if condition
> > > > + changes in patch #5: implement refresh regcache on per thread,
instead of all
> > > > threads at once
> > > >
> > > > V4:
> > > > + fix segmentation fault in live debugging (change in patch #1)
> > > > + mention live debugging not supported in cover letter and patch #1
> > > > + fixed some checkpatch warnings (change in patch #5)
> > > >
> > > > V3:
> > > > + default gdb thread will be the crashing thread, instead of being
> > > > thread '0'
> > > > + synchronise crash cpu and gdb thread context
> > > > + fix bug in gdb_interface, that replaced gdb's output stream,
losing
> > > > output in some cases, such as info threads and extra output in info
> > > > variables
> > > > + fix 'info threads'
> > > >
> > > > RFC V2:
> > > > - removed patch implementing 'frame', 'up',
'down' in crash
> > > > - updated the cover letter by removing the mention of those commands
other
> > > > than the respective gdb passthrough
> > > >
> > > > Aditya Gupta (5):
> > > > ppc64: correct gdb passthroughs by implementing
machdep->get_cpu_reg
> > > > remove 'frame' from prohibited commands list
> > > > synchronise cpu context changes between crash/gdb
> > > > fix gdb_interface: restore gdb's output streams at end of
> > > > gdb_interface
> > > > fix 'info threads' command
> > > >
> > > > crash_target.c | 44 ++++++++++++++++
> > > > defs.h | 130
+++++++++++++++++++++++++++++++++++++++++++++++-
> > > > gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
> > > > gdb_interface.c | 2 +-
> > > > kernel.c | 47 +++++++++++++++--
> > > > ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> > > > task.c | 14 ++++++
> > > > tools.c | 2 +-
> > > > 8 files changed, 434 insertions(+), 10 deletions(-)
> > > >
> > > > --
> > > > 2.41.0
> > > >
> > >
> >
>
2:36 a.m.
Hi Aditya,
On Thu, Feb 22, 2024 at 6:17 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
Hi Tao,
On Thu, Feb 22, 2024 at 04:57:23PM +0800, Tao Liu wrote:
> Hi Aditya,
>
> On Thu, Feb 22, 2024 at 1:23 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
> >
> > The Problem:
> > ============
> >
> > Currently crash is unable to show function arguments and local variables, as
> > gdb can do. And functionality for moving between frames
('up'/'down') is not
> > working in crash.
> >
> > Crash has 'gdb passthroughs' for things gdb can do, but the gdb
passthroughs
> > 'bt', 'frame', 'info locals', 'up',
'down' are not working either, due to
> > gdb not getting the register values from `crash_target::fetch_registers`,
> > which then uses `machdep->get_cpu_reg`, which is not implemented for PPC64
> >
> > Proposed Solution:
> > ==================
> >
> > Fix the gdb passthroughs by implementing "machdep->get_cpu_reg"
for PPC64.
> > This way, "gdb mode in crash" will support this feature for both ELF
and
> > kdump-compressed vmcore formats, while "gdb" would only have
supported ELF
> > format
> >
> > This way other features of 'gdb', such as seeing
> > backtraces/registers/variables/arguments/local variables, moving up and
> > down stack frames, can be used with any ppc64 vmcore, irrespective of
> > being ELF format or kdump-compressed format.
> >
> > Note: This doesn't support live debugging on ppc64, since registers are
not
> > available to be read
> >
> > Implications on Architectures:
> > ====================================
> >
> > No architecture other than PPC64 has been affected, other than in case of
> > 'frame' command
> >
> > As mentioned in patch #2, since frame will not be prohibited, so it will
print:
> >
> > crash> frame
> > #0 <unavailable> in ?? ()
> >
> > Instead of before prohibited message:
> >
> > crash> frame
> > crash: prohibited gdb command: frame
> >
> > Major change will be in 'gdb mode' on PPC64, that it will print the
frames, and
> > local variables, instead of failing with errors showing no frame, or showing
> > that couldn't get PC, it will be able to give all this information.
> >
> > Testing:
> > ========
> >
> > Git tree with this patch series applied:
> > https://github.com/adi-g15-ibm/crash/tree/stack-unwind-v9
>
> I doubt the v9 patch will not work without my x86's trial patchset. I
> see your repo, you directly applied my "ppc64 arbitrary task stack
> unwind support" patch onto yours. However the patch has some
> dependency on my x86 trial patchset.
Sorry, and thanks for the quick mail.
>
> E.g. I added a new member "bool need_free" for defs.h:struct bt_info
> in the "x86 unwind support" patch of mine, however you didn't pick
the
> one. And in ppc64.c:ppc64_get_stack_frame() and
> ppc64.c:ppc64_get_cpu_reg(), the member will be used as:
>
> if (bt_info.need_free) {
> FREEBUF(pt_regs);
> bt_info.need_free = FALSE;
> }
>
> So I guess (not tried yet) that your patchset v9 will not work.
True, it will not work. Just checked, I had already applied your patches
also and then that patch, where I tested this series. My mistake, please
skip testing this one.
>
> Currently I'm still struggling with some failing cases of x86_64
> unwinding. So I didn't arrange my patchsets, along with the patch
> commit log well, since they are all "trial" patches.
>
> I agree the patch "ppc64 arbitrary task stack unwind support" is
> better to go with the ppc patch series. But I suggest we make some
> modifications for it:
>
> 1) I'm OK with it being a stand alone patch, or merging the code
> changes of this one into your previous patches, but I prefer the
> latter one :)
>
> 2) If you'd like to go with a stand alone patch, could you please
> rewrite a commit log and title for this one?
Sure, I am okay with either. Let's go with the latter one you suggested,
in that case I will add the line adding 'need_free' in 'bt_info'
structure also, to make that work. By the way, shouldn't it be
'FREEBUF(bt_info.stackbuf)' instead of 'FREEBUF(pt_regs)' ?
Also, should I add you as a co-author of the patch ?
Thanks for the reminder! I just found some bugs and made a little
improvement for the patch. So I guess "Suggested-by:" is better than
"co-author", what do you think?
Thanks,
Tao Liu
Thanks,
Aditya Gupta
>
> Thanks,
> Tao Liu
>
>
> >
> > To test various gdb passthroughs:
> >
> > (crash) set
> > (crash) set gdb on
> > gdb> thread
> > gdb> bt
> > gdb> info threads
> > gdb> info threads
> > gdb> info locals
> > gdb> info variables irq_rover_lock
> > gdb> info args
> > gdb> thread 2
> > gdb> set gdb off
> > (crash) set
> > (crash) set -c 6
> > (crash) gdb thread
> > (crash) bt
> > (crash) gdb bt
> > (crash) frame
> > (crash) gdb up
> > (crash) gdb down
> > (crash) info locals
> >
> > Known Issues:
> > =============
> >
> > 1. In gdb mode, 'bt' might fail to show backtrace in few vmcores
collected
> > from older kernels. This is a known issue due to register mismatch, and
> > its fix has been merged upstream:
> >
> > This can also cause some 'invalid kernel virtual address' errors
during gdb
> > unwinding the stack registers
> >
> > Commit:
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
> >
> > Fixing GDB passthroughs on other architectures
> > ==============================================
> >
> > Much of the work for making gdb passthroughs like 'gdb bt', 'gdb
> > thread', 'gdb info locals' etc. has been done by the patches
introducing
> > 'machdep->get_cpu_reg' and this series fixing some issues in that.
> >
> > Other architectures should be able to fix these gdb functionalities by
> > simply implementing 'machdep->get_cpu_reg (cpu, regno, ...)'.
> >
> > The reasoning behind that has been explained with a diagram in commit
> > description of patch #1
> >
> > I will assist with my findings/observations fixing it on ppc64 whenever
needed.
> >
> > Changelog:
> > ==========
> >
> > V9:
> > + minor change in patch #5: sync gdb context on a 'set' and 'set
-p'
> > + add taoliu's patch for using current context, and fixes in
ppc64_get_cpu_reg
> >
> > V8:
> > + use get_active_task instead of depending on CURRENT_CONTEXT in
ppc64_get_cpu_reg
> > + rebase to upstream/master (5977936c0a91)
> >
> > V7:
> > + move changes in gdb-10.2.patch to the end (minor change in patch #3,4,5)
> > + fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
> > + use ascii diagram in patch #1 description
> >
> > V6:
> > + changes in patch #5: fix bug introduced in v5 that caused initial gdb thread
> > to be thread 1
> >
> > V5:
> > + changes in patch #1: made ppc64_get_cpu_reg static, and remove unreachable
> > code
> > + changes in patch #3: fixed typo 'ppc64_renum' instead of
'ppc64_regnum',
> > remove unneeded if condition
> > + changes in patch #5: implement refresh regcache on per thread, instead of
all
> > threads at once
> >
> > V4:
> > + fix segmentation fault in live debugging (change in patch #1)
> > + mention live debugging not supported in cover letter and patch #1
> > + fixed some checkpatch warnings (change in patch #5)
> >
> > V3:
> > + default gdb thread will be the crashing thread, instead of being
> > thread '0'
> > + synchronise crash cpu and gdb thread context
> > + fix bug in gdb_interface, that replaced gdb's output stream, losing
> > output in some cases, such as info threads and extra output in info
> > variables
> > + fix 'info threads'
> >
> > RFC V2:
> > - removed patch implementing 'frame', 'up', 'down' in
crash
> > - updated the cover letter by removing the mention of those commands other
> > than the respective gdb passthrough
> >
> > Aditya Gupta (5):
> > ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
> > remove 'frame' from prohibited commands list
> > synchronise cpu context changes between crash/gdb
> > fix gdb_interface: restore gdb's output streams at end of
> > gdb_interface
> > fix 'info threads' command
> >
> > crash_target.c | 44 ++++++++++++++++
> > defs.h | 130 +++++++++++++++++++++++++++++++++++++++++++++++-
> > gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
> > gdb_interface.c | 2 +-
> > kernel.c | 47 +++++++++++++++--
> > ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> > task.c | 14 ++++++
> > tools.c | 2 +-
> > 8 files changed, 434 insertions(+), 10 deletions(-)
> >
> > --
> > 2.41.0
> >
>
3:53 a.m.
Hi Tao Liu,
> > <...snip...>
> >
> > Currently I'm still struggling with some failing cases of x86_64
> > unwinding. So I didn't arrange my patchsets, along with the patch
> > commit log well, since they are all "trial" patches.
> >
> > I agree the patch "ppc64 arbitrary task stack unwind support" is
> > better to go with the ppc patch series. But I suggest we make some
> > modifications for it:
> >
> > 1) I'm OK with it being a stand alone patch, or merging the code
> > changes of this one into your previous patches, but I prefer the
> > latter one :)
> >
> > 2) If you'd like to go with a stand alone patch, could you please
> > rewrite a commit log and title for this one?
>
> Sure, I am okay with either. Let's go with the latter one you suggested,
> in that case I will add the line adding 'need_free' in 'bt_info'
> structure also, to make that work. By the way, shouldn't it be
> 'FREEBUF(bt_info.stackbuf)' instead of 'FREEBUF(pt_regs)' ?
>
> Also, should I add you as a co-author of the patch ?
Thanks for the reminder! I just found some bugs and made a little
improvement for the patch. So I guess "Suggested-by:" is better than
"co-author", what do you think?
Sure, I will pull your latest changes. Regarding the tag,
"improved-by" okay with you ? Also, I am thinking to add the Cc: list in
my patch itself, that seems good idea for a discussion this long, and
easier to manage with git-send-email.
Btw, I will post the next version, after Lianbo replies to the splitting
the series suggestion.
Thanks,
Aditya Gupta
>
> Thanks,
> Tao Liu
>
> >
> > Thanks,
> > Aditya Gupta
> >
> > >
> > > Thanks,
> > > Tao Liu
> > >
> > >
> > > >
> > > > To test various gdb passthroughs:
> > > >
> > > > (crash) set
> > > > (crash) set gdb on
> > > > gdb> thread
> > > > gdb> bt
> > > > gdb> info threads
> > > > gdb> info threads
> > > > gdb> info locals
> > > > gdb> info variables irq_rover_lock
> > > > gdb> info args
> > > > gdb> thread 2
> > > > gdb> set gdb off
> > > > (crash) set
> > > > (crash) set -c 6
> > > > (crash) gdb thread
> > > > (crash) bt
> > > > (crash) gdb bt
> > > > (crash) frame
> > > > (crash) gdb up
> > > > (crash) gdb down
> > > > (crash) info locals
> > > >
> > > > Known Issues:
> > > > =============
> > > >
> > > > 1. In gdb mode, 'bt' might fail to show backtrace in few
vmcores collected
> > > > from older kernels. This is a known issue due to register mismatch,
and
> > > > its fix has been merged upstream:
> > > >
> > > > This can also cause some 'invalid kernel virtual address'
errors during gdb
> > > > unwinding the stack registers
> > > >
> > > > Commit:
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
> > > >
> > > > Fixing GDB passthroughs on other architectures
> > > > ==============================================
> > > >
> > > > Much of the work for making gdb passthroughs like 'gdb bt',
'gdb
> > > > thread', 'gdb info locals' etc. has been done by the
patches introducing
> > > > 'machdep->get_cpu_reg' and this series fixing some issues
in that.
> > > >
> > > > Other architectures should be able to fix these gdb functionalities
by
> > > > simply implementing 'machdep->get_cpu_reg (cpu, regno,
...)'.
> > > >
> > > > The reasoning behind that has been explained with a diagram in commit
> > > > description of patch #1
> > > >
> > > > I will assist with my findings/observations fixing it on ppc64
whenever needed.
> > > >
> > > > Changelog:
> > > > ==========
> > > >
> > > > V9:
> > > > + minor change in patch #5: sync gdb context on a 'set' and
'set -p'
> > > > + add taoliu's patch for using current context, and fixes in
ppc64_get_cpu_reg
> > > >
> > > > V8:
> > > > + use get_active_task instead of depending on CURRENT_CONTEXT in
ppc64_get_cpu_reg
> > > > + rebase to upstream/master (5977936c0a91)
> > > >
> > > > V7:
> > > > + move changes in gdb-10.2.patch to the end (minor change in patch
#3,4,5)
> > > > + fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
> > > > + use ascii diagram in patch #1 description
> > > >
> > > > V6:
> > > > + changes in patch #5: fix bug introduced in v5 that caused initial
gdb thread
> > > > to be thread 1
> > > >
> > > > V5:
> > > > + changes in patch #1: made ppc64_get_cpu_reg static, and remove
unreachable
> > > > code
> > > > + changes in patch #3: fixed typo 'ppc64_renum' instead of
'ppc64_regnum',
> > > > remove unneeded if condition
> > > > + changes in patch #5: implement refresh regcache on per thread,
instead of all
> > > > threads at once
> > > >
> > > > V4:
> > > > + fix segmentation fault in live debugging (change in patch #1)
> > > > + mention live debugging not supported in cover letter and patch #1
> > > > + fixed some checkpatch warnings (change in patch #5)
> > > >
> > > > V3:
> > > > + default gdb thread will be the crashing thread, instead of being
> > > > thread '0'
> > > > + synchronise crash cpu and gdb thread context
> > > > + fix bug in gdb_interface, that replaced gdb's output stream,
losing
> > > > output in some cases, such as info threads and extra output in info
> > > > variables
> > > > + fix 'info threads'
> > > >
> > > > RFC V2:
> > > > - removed patch implementing 'frame', 'up',
'down' in crash
> > > > - updated the cover letter by removing the mention of those commands
other
> > > > than the respective gdb passthrough
> > > >
> > > > Aditya Gupta (5):
> > > > ppc64: correct gdb passthroughs by implementing
machdep->get_cpu_reg
> > > > remove 'frame' from prohibited commands list
> > > > synchronise cpu context changes between crash/gdb
> > > > fix gdb_interface: restore gdb's output streams at end of
> > > > gdb_interface
> > > > fix 'info threads' command
> > > >
> > > > crash_target.c | 44 ++++++++++++++++
> > > > defs.h | 130
+++++++++++++++++++++++++++++++++++++++++++++++-
> > > > gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
> > > > gdb_interface.c | 2 +-
> > > > kernel.c | 47 +++++++++++++++--
> > > > ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> > > > task.c | 14 ++++++
> > > > tools.c | 2 +-
> > > > 8 files changed, 434 insertions(+), 10 deletions(-)
> > > >
> > > > --
> > > > 2.41.0
> > > >
> > >
> >
>
9:10 p.m.
Hi Aditya,
On Fri, Feb 23, 2024 at 5:53 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
Hi Tao Liu,
> > > <...snip...>
> > >
> > > Currently I'm still struggling with some failing cases of x86_64
> > > unwinding. So I didn't arrange my patchsets, along with the patch
> > > commit log well, since they are all "trial" patches.
> > >
> > > I agree the patch "ppc64 arbitrary task stack unwind support"
is
> > > better to go with the ppc patch series. But I suggest we make some
> > > modifications for it:
> > >
> > > 1) I'm OK with it being a stand alone patch, or merging the code
> > > changes of this one into your previous patches, but I prefer the
> > > latter one :)
> > >
> > > 2) If you'd like to go with a stand alone patch, could you please
> > > rewrite a commit log and title for this one?
> >
> > Sure, I am okay with either. Let's go with the latter one you suggested,
> > in that case I will add the line adding 'need_free' in
'bt_info'
> > structure also, to make that work. By the way, shouldn't it be
> > 'FREEBUF(bt_info.stackbuf)' instead of 'FREEBUF(pt_regs)' ?
> >
> > Also, should I add you as a co-author of the patch ?
>
> Thanks for the reminder! I just found some bugs and made a little
> improvement for the patch. So I guess "Suggested-by:" is better than
> "co-author", what do you think?
Sure, I will pull your latest changes. Regarding the tag,
"improved-by" okay with you ? Also, I am thinking to add the Cc: list in
my patch itself, that seems good idea for a discussion this long, and
easier to manage with git-send-email.
OK, it is OK with me, thanks!
Thanks,
Tao Liu
Btw, I will post the next version, after Lianbo replies to the splitting
the series suggestion.
Thanks,
Aditya Gupta
>
> Thanks,
> Tao Liu
>
> >
> > Thanks,
> > Aditya Gupta
> >
> > >
> > > Thanks,
> > > Tao Liu
> > >
> > >
> > > >
> > > > To test various gdb passthroughs:
> > > >
> > > > (crash) set
> > > > (crash) set gdb on
> > > > gdb> thread
> > > > gdb> bt
> > > > gdb> info threads
> > > > gdb> info threads
> > > > gdb> info locals
> > > > gdb> info variables irq_rover_lock
> > > > gdb> info args
> > > > gdb> thread 2
> > > > gdb> set gdb off
> > > > (crash) set
> > > > (crash) set -c 6
> > > > (crash) gdb thread
> > > > (crash) bt
> > > > (crash) gdb bt
> > > > (crash) frame
> > > > (crash) gdb up
> > > > (crash) gdb down
> > > > (crash) info locals
> > > >
> > > > Known Issues:
> > > > =============
> > > >
> > > > 1. In gdb mode, 'bt' might fail to show backtrace in few
vmcores collected
> > > > from older kernels. This is a known issue due to register
mismatch, and
> > > > its fix has been merged upstream:
> > > >
> > > > This can also cause some 'invalid kernel virtual address'
errors during gdb
> > > > unwinding the stack registers
> > > >
> > > > Commit:
https://github.com/torvalds/linux/commit/b684c09f09e7a6af3794d4233ef78581...
> > > >
> > > > Fixing GDB passthroughs on other architectures
> > > > ==============================================
> > > >
> > > > Much of the work for making gdb passthroughs like 'gdb bt',
'gdb
> > > > thread', 'gdb info locals' etc. has been done by the
patches introducing
> > > > 'machdep->get_cpu_reg' and this series fixing some issues
in that.
> > > >
> > > > Other architectures should be able to fix these gdb functionalities
by
> > > > simply implementing 'machdep->get_cpu_reg (cpu, regno,
...)'.
> > > >
> > > > The reasoning behind that has been explained with a diagram in
commit
> > > > description of patch #1
> > > >
> > > > I will assist with my findings/observations fixing it on ppc64
whenever needed.
> > > >
> > > > Changelog:
> > > > ==========
> > > >
> > > > V9:
> > > > + minor change in patch #5: sync gdb context on a 'set' and
'set -p'
> > > > + add taoliu's patch for using current context, and fixes in
ppc64_get_cpu_reg
> > > >
> > > > V8:
> > > > + use get_active_task instead of depending on CURRENT_CONTEXT in
ppc64_get_cpu_reg
> > > > + rebase to upstream/master (5977936c0a91)
> > > >
> > > > V7:
> > > > + move changes in gdb-10.2.patch to the end (minor change in patch
#3,4,5)
> > > > + fix a memory leak in ppc64_get_cpu_reg (minor change in patch #1)
> > > > + use ascii diagram in patch #1 description
> > > >
> > > > V6:
> > > > + changes in patch #5: fix bug introduced in v5 that caused initial
gdb thread
> > > > to be thread 1
> > > >
> > > > V5:
> > > > + changes in patch #1: made ppc64_get_cpu_reg static, and remove
unreachable
> > > > code
> > > > + changes in patch #3: fixed typo 'ppc64_renum' instead of
'ppc64_regnum',
> > > > remove unneeded if condition
> > > > + changes in patch #5: implement refresh regcache on per thread,
instead of all
> > > > threads at once
> > > >
> > > > V4:
> > > > + fix segmentation fault in live debugging (change in patch #1)
> > > > + mention live debugging not supported in cover letter and patch #1
> > > > + fixed some checkpatch warnings (change in patch #5)
> > > >
> > > > V3:
> > > > + default gdb thread will be the crashing thread, instead of being
> > > > thread '0'
> > > > + synchronise crash cpu and gdb thread context
> > > > + fix bug in gdb_interface, that replaced gdb's output stream,
losing
> > > > output in some cases, such as info threads and extra output in
info
> > > > variables
> > > > + fix 'info threads'
> > > >
> > > > RFC V2:
> > > > - removed patch implementing 'frame', 'up',
'down' in crash
> > > > - updated the cover letter by removing the mention of those
commands other
> > > > than the respective gdb passthrough
> > > >
> > > > Aditya Gupta (5):
> > > > ppc64: correct gdb passthroughs by implementing
machdep->get_cpu_reg
> > > > remove 'frame' from prohibited commands list
> > > > synchronise cpu context changes between crash/gdb
> > > > fix gdb_interface: restore gdb's output streams at end of
> > > > gdb_interface
> > > > fix 'info threads' command
> > > >
> > > > crash_target.c | 44 ++++++++++++++++
> > > > defs.h | 130
+++++++++++++++++++++++++++++++++++++++++++++++-
> > > > gdb-10.2.patch | 110 +++++++++++++++++++++++++++++++++++++++-
> > > > gdb_interface.c | 2 +-
> > > > kernel.c | 47 +++++++++++++++--
> > > > ppc64.c | 95 +++++++++++++++++++++++++++++++++--
> > > > task.c | 14 ++++++
> > > > tools.c | 2 +-
> > > > 8 files changed, 434 insertions(+), 10 deletions(-)
> > > >
> > > > --
> > > > 2.41.0
> > > >
> > >
> >
>
118
days inactive
130
days old
devel@lists.crash-utility.osci.io
24 comments
3 participants
participants (3)
-
Aditya Gupta -
lijiang -
Tao Liu