1:59 a.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-v4
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) up
(crash) 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.
Additional Notes:
=================
Sorry, it took a long time to send this version. Tried fixing 'info
threads' but wasn't able to. Gave it time again, and was able to fix it
this time after multiple days of debugging.
Some other things from last version review:
* 'info rv' not working:
It's not supported in gdb, instead we need to use 'info locals rv' or
'info variables rv'
* 'info variables' command hangs... and prints nothing after hanging for long
It likely hangs due to a lot of symbols being there, and it's trying to
get all gdb's output and page it, so Control+C messes it up, but if we pass
a regex filter to limit the output, eg. info variables rq, then it doesn't
hang, and prints the variables/symbols.
Even with gdb, ie. simply running 'gdb vmlinux vmcore' also hangs due
to the lot of symbols
* making crashing thread as default in gdb:
This is implemented now, along with synchronising crash & gdb contexts, in
patch #3
* 'info threads' not working:
This turned to be due to a bug in gdb_interface. I fixed 'info
threads' in 2 patches, to simplify it, first for the gdb_interface,
and another patch for setting the context correctly in crash
* other info commands:
I tested all the info commands, in crash along with this patch.
Most of those that fail in crash are due to gdb itself not supporting
them with vmcores, and other than that is the 'info pretty' command,
which might not be needed in crash anyways
* live debugging showing only one thread:
I tried it with crash, crash shows only the current thread, ie.
itself, so it does not have information of registers for the other
CPUs. Similarly gdb does not support live kernel debugging (without
connecting to a gdbstub/QEMU etc.).
If you need I can make it show the current thread id correctly for
the one thread, but I don't think it might help much with live
debugging
Hope, I set the context, thanks for the reviews, I replied and worked
on your suggestions, but got stuck there due to 'info threads'
Changelog:
==========
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
1:59 a.m.
New subject: [PATCH v4 1/5] 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 | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
kernel.c | 33 +++++++++++++++
ppc64.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 262 insertions(+), 3 deletions(-)
diff --git a/defs.h b/defs.h
index 5218a94fe4a4..0c80de72e89e 100644
--- a/defs.h
+++ b/defs.h
@@ -6023,6 +6023,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 *);
@@ -6121,6 +6122,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)
@@ -6662,6 +6664,8 @@ struct machine_specific {
void ppc64_init(int);
void ppc64_dump_machdep_table(ulong);
+int ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
+ void *value);
#define display_idt_table() \
error(FATAL, "-d option is not applicable to PowerPC architecture\n")
#define KSYMS_START (0x1)
@@ -7854,4 +7858,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_renum {
+ 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 6dcf414693e6..52b7ba09f390 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..f5b0b7241ea4 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 *);
+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,102 @@ ppc64_print_eframe(char *efrm_str, struct ppc64_pt_regs *regs,
ppc64_print_nip_lr(regs, 1);
}
+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;
+ 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 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
+ return FALSE;
+ }
+
+ /* FIXME: Always setting the context to CURRENT_CONTEXT irrespective of whicher
+ * thread we switched to, in gdb
+ */
+ tc = CURRENT_CONTEXT();
+ 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);
+ return TRUE;
+
+ case PPC64_PC_REGNUM:
+ if (size != sizeof(pt_regs->nip))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->nip, size);
+ return TRUE;
+
+ case PPC64_MSR_REGNUM:
+ if (size != sizeof(pt_regs->msr))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->msr, size);
+ return TRUE;
+
+ case PPC64_LR_REGNUM:
+ if (size != sizeof(pt_regs->link))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->link, size);
+ return TRUE;
+
+ case PPC64_CTR_REGNUM:
+ if (size != sizeof(pt_regs->ctr))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->ctr, size);
+ return TRUE;
+ }
+
+ printf("error: %s: statement after switch case should be unreachable"
+ , __func__);
+ return FALSE;
+}
+
/*
* For vmcore typically saved with KDump or FADump, get SP and IP values
* from the saved ptregs.
@@ -2613,9 +2713,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:43 a.m.
New subject: [PATCH v4 1/5] ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
Thank you for the update, Aditya.
On 12/15/23 15:59, 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 | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
kernel.c | 33 +++++++++++++++
ppc64.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 262 insertions(+), 3 deletions(-)
diff --git a/defs.h b/defs.h
index 5218a94fe4a4..0c80de72e89e 100644
--- a/defs.h
+++ b/defs.h
@@ -6023,6 +6023,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 *);
@@ -6121,6 +6122,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)
@@ -6662,6 +6664,8 @@ struct machine_specific {
void ppc64_init(int);
void ppc64_dump_machdep_table(ulong);
+int ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
+ void *value);
This is only invoked in ppc64 module, so it should be good to define
this function in the ppc64.c, and use the keywork 'static'.
Given that, no need to add it in defs.h.
#define display_idt_table() \
error(FATAL, "-d option is not applicable to PowerPC
architecture\n")
#define KSYMS_START (0x1)
@@ -7854,4 +7858,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
+ */
Some register number constants are defined in the gdb/ppc-tdep.h:
enum {
PPC_R0_REGNUM = 0,
PPC_F0_REGNUM = 32,
PPC_PC_REGNUM = 64,
...
}
Do you mean that other undefined registers won't be used in this case?
Just a confirmation.
+enum ppc64_renum {
+ 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 6dcf414693e6..52b7ba09f390 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..f5b0b7241ea4 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 *);
+int ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
+ void *value);
As I mentioned before, we can add a 'static' keyword.
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,102 @@ ppc64_print_eframe(char *efrm_str, struct ppc64_pt_regs *regs,
ppc64_print_nip_lr(regs, 1);
}
+int
+ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
+ void *value)
Ditto.
+{
+ struct bt_info bt_info, bt_setup;
+ struct task_context *tc;
+ 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 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
Could you please print the 'regno' value if the given register number is
not handled for now?
if (CRASHDEBUG(1))
error(WARNING, "unsupported register, regno=%d\n", regno);
+ return FALSE;
+ }
+
+ /* FIXME: Always setting the context to CURRENT_CONTEXT irrespective of whicher
+ * thread we switched to, in gdb
+ */
The above code comment can be removed, because the latter patch will
change this behavior.
Or you could explain a little bit why to do this change in the code
comment if needed.
+ tc = CURRENT_CONTEXT();
+ 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);
+ return TRUE;
+
+ case PPC64_PC_REGNUM:
+ if (size != sizeof(pt_regs->nip))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->nip, size);
+ return TRUE;
+
+ case PPC64_MSR_REGNUM:
+ if (size != sizeof(pt_regs->msr))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->msr, size);
+ return TRUE;
+
+ case PPC64_LR_REGNUM:
+ if (size != sizeof(pt_regs->link))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->link, size);
+ return TRUE;
+
+ case PPC64_CTR_REGNUM:
+ if (size != sizeof(pt_regs->ctr))
+ return FALSE; // size mismatch
+
+ memcpy(value, &pt_regs->ctr, size);
+ return TRUE;
+ }
+
+ printf("error: %s: statement after switch case should be unreachable"
+ , __func__);
+ return FALSE;
I would suggest the following changes:
...
switch (regno) {
+ case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
+ if (size != sizeof(pt_regs->gpr[regno]))
+ return FALSE;
+
+ memcpy(value, &pt_regs->gpr[regno], size);
+ break;
+ case...
+}
+return TRUE;
+}
The 'printf' and 'return FALSE' should not be reached. I guess this is
debugging information, if 'yes', we can remove it this time.
Thanks
Lianbo
+}
+
/*
* For vmcore typically saved with KDump or FADump, get SP and IP values
* from the saved ptregs.
@@ -2613,9 +2713,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))) {
12:31 a.m.
New subject: [PATCH v4 1/5] ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
Hi Lianbo,
On Mon, Dec 18, 2023 at 07:43:27PM +0800, Lianbo Jiang wrote:
Thank you for the update, Aditya.
On 12/15/23 15:59, 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 | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> kernel.c | 33 +++++++++++++++
> ppc64.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 262 insertions(+), 3 deletions(-)
>
> diff --git a/defs.h b/defs.h
> index 5218a94fe4a4..0c80de72e89e 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -6023,6 +6023,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 *);
> @@ -6121,6 +6122,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)
> @@ -6662,6 +6664,8 @@ struct machine_specific {
> void ppc64_init(int);
> void ppc64_dump_machdep_table(ulong);
> +int ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
> + void *value);
This is only invoked in ppc64 module, so it should be good to define this
function in the ppc64.c, and use the keywork 'static'.
Given that, no need to add it in defs.h.
Sure, I will make it static, and remove it from defs.h.
> #define display_idt_table() \
> error(FATAL, "-d option is not applicable to PowerPC
architecture\n")
> #define KSYMS_START (0x1)
> @@ -7854,4 +7858,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
> + */
Some register number constants are defined in the gdb/ppc-tdep.h:
enum {
PPC_R0_REGNUM = 0,
PPC_F0_REGNUM = 32,
PPC_PC_REGNUM = 64,
...
Thanks for the reference.
}
Do you mean that other undefined registers won't be used in this case? Just
a confirmation.
Yes, other registers won't be handled, since they are not in the stack frame
according to the ABI at
https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#STACK
In 'ppc64_get_cpu_reg' we simply return from the function for any undefined
register number.
> +enum ppc64_renum {
> + 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 6dcf414693e6..52b7ba09f390 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..f5b0b7241ea4 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 *);
> +int ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
> + void *value);
As I mentioned before, we can add a 'static' keyword.
Sure. Will add it.
> 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,102 @@ ppc64_print_eframe(char *efrm_str, struct ppc64_pt_regs
*regs,
> ppc64_print_nip_lr(regs, 1);
> }
> +int
> +ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
> + void *value)
Ditto.
> +{
> + struct bt_info bt_info, bt_setup;
> + struct task_context *tc;
> + 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 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
Could you please print the 'regno' value if the given register number is not
handled for now?
if (CRASHDEBUG(1))
error(WARNING, "unsupported register, regno=%d\n", regno);
Sure I will add that message.
> + return FALSE;
> + }
> +
> + /* FIXME: Always setting the context to CURRENT_CONTEXT irrespective of whicher
> + * thread we switched to, in gdb
> + */
The above code comment can be removed, because the latter patch will change
this behavior.
Or you could explain a little bit why to do this change in the code comment
if needed.
Okay, sure I will remove it. I kept it signifying that it's a issue in this
patch that will be fixed later.
> + tc = CURRENT_CONTEXT();
> + 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);
> + return TRUE;
> +
> + case PPC64_PC_REGNUM:
> + if (size != sizeof(pt_regs->nip))
> + return FALSE; // size mismatch
> +
> + memcpy(value, &pt_regs->nip, size);
> + return TRUE;
> +
> + case PPC64_MSR_REGNUM:
> + if (size != sizeof(pt_regs->msr))
> + return FALSE; // size mismatch
> +
> + memcpy(value, &pt_regs->msr, size);
> + return TRUE;
> +
> + case PPC64_LR_REGNUM:
> + if (size != sizeof(pt_regs->link))
> + return FALSE; // size mismatch
> +
> + memcpy(value, &pt_regs->link, size);
> + return TRUE;
> +
> + case PPC64_CTR_REGNUM:
> + if (size != sizeof(pt_regs->ctr))
> + return FALSE; // size mismatch
> +
> + memcpy(value, &pt_regs->ctr, size);
> + return TRUE;
> + }
> +
> + printf("error: %s: statement after switch case should be unreachable"
> + , __func__);
> + return FALSE;
I would suggest the following changes:
...
switch (regno) {
+ case PPC64_R0_REGNUM ... PPC64_R31_REGNUM:
+ if (size != sizeof(pt_regs->gpr[regno]))
+ return FALSE;
+
+ memcpy(value, &pt_regs->gpr[regno], size);
+ break;
+ case...
+}
+return TRUE;
+}
Makes sense, sure I will move the returns to end, and break instead.
The 'printf' and 'return FALSE' should not be reached. I guess this is
debugging information, if 'yes', we can remove it this time.
Sure, I will remove it.
Thanks,
Aditya Gupta
>
> Thanks
> Lianbo
>
> > +}
> > +
> > /*
> > * For vmcore typically saved with KDump or FADump, get SP and IP values
> > * from the saved ptregs.
> > @@ -2613,9 +2713,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))) {
>
1:59 a.m.
New subject: [PATCH v4 2/5] 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
1:59 a.m.
New subject: [PATCH v4 3/5] 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 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 for gdb passthroughs
to work correctly, gdb needs register values from crash, which depend on
current context in crash.
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 | 22 ++++++++++++++++++++++
defs.h | 3 +++
gdb-10.2.patch | 30 ++++++++++++++++++++++++++++++
kernel.c | 8 +++++++-
ppc64.c | 3 ---
task.c | 5 +++++
6 files changed, 67 insertions(+), 4 deletions(-)
diff --git a/crash_target.c b/crash_target.c
index 455480679741..223a51acb1e1 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,23 @@ 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 0c80de72e89e..5e6df14e947d 100644
--- a/defs.h
+++ b/defs.h
@@ -7978,4 +7978,7 @@ enum ppc64_renum {
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 2f7d585105aa..8c1b43eb07b7 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 \
@@ -485,6 +486,35 @@ exit 0
return best_pst;
}
+--- 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. */
--- gdb-10.2/gdb/symfile.c.orig
+++ gdb-10.2/gdb/symfile.c
@@ -652,7 +652,26 @@ default_symfile_offsets (struct objfile *objfile,
diff --git a/kernel.c b/kernel.c
index 52b7ba09f390..a8d60507dd95 100644
--- a/kernel.c
+++ b/kernel.c
@@ -6504,7 +6504,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/ppc64.c b/ppc64.c
index f5b0b7241ea4..1816972fcf2c 100644
--- a/ppc64.c
+++ b/ppc64.c
@@ -2542,9 +2542,6 @@ ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
return FALSE;
}
- /* FIXME: Always setting the context to CURRENT_CONTEXT irrespective of whicher
- * thread we switched to, in gdb
- */
tc = CURRENT_CONTEXT();
BZERO(&bt_setup, sizeof(struct bt_info));
clone_bt_info(&bt_setup, &bt_info, tc);
diff --git a/task.c b/task.c
index ebdb5be3786f..64088eca29a1 100644
--- a/task.c
+++ b/task.c
@@ -5301,6 +5301,11 @@ set_context(ulong task, ulong pid)
if (found) {
CURRENT_CONTEXT() = tc;
+
+ /* change the selected thread in gdb, according to current context */
+ if ( gdb_change_cpu_context(tc->processor) == FALSE )
+ return FALSE;
+
return TRUE;
} else {
if (task)
--
2.43.0
6:33 a.m.
New subject: [PATCH v4 3/5] synchronise cpu context changes between crash/gdb
On 12/15/23 15:59, Aditya Gupta wrote:
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 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 for gdb passthroughs
to work correctly, gdb needs register values from crash, which depend on
current context in crash.
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 | 22 ++++++++++++++++++++++
defs.h | 3 +++
gdb-10.2.patch | 30 ++++++++++++++++++++++++++++++
kernel.c | 8 +++++++-
ppc64.c | 3 ---
task.c | 5 +++++
6 files changed, 67 insertions(+), 4 deletions(-)
diff --git a/crash_target.c b/crash_target.c
index 455480679741..223a51acb1e1 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,23 @@ 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 0c80de72e89e..5e6df14e947d 100644
--- a/defs.h
+++ b/defs.h
@@ -7978,4 +7978,7 @@ enum ppc64_renum {
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 2f7d585105aa..8c1b43eb07b7 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 \
@@ -485,6 +486,35 @@ exit 0
return best_pst;
}
+--- 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. */
--- gdb-10.2/gdb/symfile.c.orig
+++ gdb-10.2/gdb/symfile.c
@@ -652,7 +652,26 @@ default_symfile_offsets (struct objfile *objfile,
diff --git a/kernel.c b/kernel.c
index 52b7ba09f390..a8d60507dd95 100644
--- a/kernel.c
+++ b/kernel.c
@@ -6504,7 +6504,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);
Good changes.
else
error(FATAL, "cannot determine active task on cpu %ld\n", cpu);
diff --git a/ppc64.c b/ppc64.c
index f5b0b7241ea4..1816972fcf2c 100644
--- a/ppc64.c
+++ b/ppc64.c
@@ -2542,9 +2542,6 @@ ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
return FALSE;
}
- /* FIXME: Always setting the context to CURRENT_CONTEXT irrespective of whicher
- * thread we switched to, in gdb
- */
tc = CURRENT_CONTEXT();
BZERO(&bt_setup, sizeof(struct bt_info));
clone_bt_info(&bt_setup, &bt_info, tc);
diff --git a/task.c b/task.c
index ebdb5be3786f..64088eca29a1 100644
--- a/task.c
+++ b/task.c
@@ -5301,6 +5301,11 @@ set_context(ulong task, ulong pid)
if (found) {
CURRENT_CONTEXT() = tc;
+
+ /* change the selected thread in gdb, according to current context */
+ if ( gdb_change_cpu_context(tc->processor) == FALSE )
+ return FALSE;
+
return TRUE;
Because the gdb_change_cpu_context() always returns 'TRUE' or 'FALSE',
we can do this changes as below:
return gdb_change_cpu_context(tc->processor);
And remove the redundant 'return TRUE'.
Thanks.
Lianbo
} else {
if (task)
12:36 a.m.
New subject: [PATCH v4 3/5] synchronise cpu context changes between crash/gdb
Hello Lianbo,
On Mon, Dec 18, 2023 at 08:33:59PM +0800, Lianbo Jiang wrote:
On 12/15/23 15:59, Aditya Gupta wrote:
> 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 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 for gdb passthroughs
> to work correctly, gdb needs register values from crash, which depend on
> current context in crash.
>
> 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 | 22 ++++++++++++++++++++++
> defs.h | 3 +++
> gdb-10.2.patch | 30 ++++++++++++++++++++++++++++++
> kernel.c | 8 +++++++-
> ppc64.c | 3 ---
> task.c | 5 +++++
> 6 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/crash_target.c b/crash_target.c
> index 455480679741..223a51acb1e1 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,23 @@ 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 0c80de72e89e..5e6df14e947d 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -7978,4 +7978,7 @@ enum ppc64_renum {
> 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 2f7d585105aa..8c1b43eb07b7 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 \
> @@ -485,6 +486,35 @@ exit 0
> return best_pst;
> }
> +--- 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. */
> --- gdb-10.2/gdb/symfile.c.orig
> +++ gdb-10.2/gdb/symfile.c
> @@ -652,7 +652,26 @@ default_symfile_offsets (struct objfile *objfile,
> diff --git a/kernel.c b/kernel.c
> index 52b7ba09f390..a8d60507dd95 100644
> --- a/kernel.c
> +++ b/kernel.c
> @@ -6504,7 +6504,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);
Good changes.
> else
> error(FATAL, "cannot determine active task on cpu %ld\n", cpu);
> diff --git a/ppc64.c b/ppc64.c
> index f5b0b7241ea4..1816972fcf2c 100644
> --- a/ppc64.c
> +++ b/ppc64.c
> @@ -2542,9 +2542,6 @@ ppc64_get_cpu_reg(int cpu, int regno, const char *name, int
size,
> return FALSE;
> }
> - /* FIXME: Always setting the context to CURRENT_CONTEXT irrespective of whicher
> - * thread we switched to, in gdb
> - */
> tc = CURRENT_CONTEXT();
> BZERO(&bt_setup, sizeof(struct bt_info));
> clone_bt_info(&bt_setup, &bt_info, tc);
> diff --git a/task.c b/task.c
> index ebdb5be3786f..64088eca29a1 100644
> --- a/task.c
> +++ b/task.c
> @@ -5301,6 +5301,11 @@ set_context(ulong task, ulong pid)
> if (found) {
> CURRENT_CONTEXT() = tc;
> +
> + /* change the selected thread in gdb, according to current context */
> + if ( gdb_change_cpu_context(tc->processor) == FALSE )
> + return FALSE;
> +
> return TRUE;
Because the gdb_change_cpu_context() always returns 'TRUE' or 'FALSE',
we
can do this changes as below:
return gdb_change_cpu_context(tc->processor);
And remove the redundant 'return TRUE'.
Sure, I will do it.
Thanks,
Aditya Gupta
>
>
> Thanks.
>
> Lianbo
>
> > } else {
> > if (task)
>
1:59 a.m.
New subject: [PATCH v4 4/5] 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 | 46 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 45 insertions(+), 1 deletion(-)
diff --git a/gdb-10.2.patch b/gdb-10.2.patch
index 8c1b43eb07b7..a4e45fa0e6fa 100644
--- a/gdb-10.2.patch
+++ b/gdb-10.2.patch
@@ -1484,13 +1484,29 @@ exit 0
+ }
+}
+#endif
+--- 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/ui-file.h.orig
+++ gdb-10.2/gdb/ui-file.h
-@@ -195,10 +195,10 @@ class stdio_file : public ui_file
+@@ -195,10 +195,11 @@ class stdio_file : public ui_file
bool can_emit_style_escape () override;
-private:
++ 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);
@@ -3267,3 +3283,31 @@ exit 0
for (compunit_symtab *cust : objfile->compunits ())
{
+--- 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
1:59 a.m.
New subject: [PATCH v4 5/5] 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 | 36 +++++++++++++++++++++++++++++++-----
defs.h | 3 ++-
gdb-10.2.patch | 40 +++++++++++++++++++++++++++++++++++++---
kernel.c | 6 ++++--
task.c | 9 +++++++++
tools.c | 2 +-
6 files changed, 84 insertions(+), 12 deletions(-)
diff --git a/crash_target.c b/crash_target.c
index 223a51acb1e1..96a65a1fa55c 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(void);
+extern "C" int set_cpu(int cpu, int print_context);
/* The crash target. */
@@ -139,19 +140,44 @@ crash_target_init (void)
/*
* Change gdb's thread context to the thread on given CPU
* */
-extern "C" int gdb_change_cpu_context (unsigned int 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 ();
+ inferior *inf = current_inferior ();
thread_info *tp = find_thread_ptid (inf, ptid);
if (tp == nullptr)
- return FALSE;
+ 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);
return TRUE;
}
+/* Refresh regcache of all gdb threads
+ *
+ * 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(void)
+{
+ inferior *inf = current_inferior();
+ int saved_cpu = inferior_thread()->ptid.tid();
+
+ for (thread_info * tp : inf->threads()) {
+ /* temporarily switch to the cpu so we get correct registers */
+ set_cpu(tp->ptid.tid(), FALSE);
+ target_fetch_registers(get_thread_regcache(tp), -1);
+ }
+
+ set_cpu(saved_cpu, FALSE);
+}
diff --git a/defs.h b/defs.h
index 5e6df14e947d..240e5c27b223 100644
--- a/defs.h
+++ b/defs.h
@@ -6044,7 +6044,7 @@ void parse_kernel_version(char *);
#define SHOW_LOG_AUDIT (0x8)
#define SHOW_LOG_CTIME (0x10)
#define SHOW_LOG_SAFE (0x20)
-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);
@@ -7980,5 +7980,6 @@ enum ppc64_renum {
/* crash_target.c */
extern int gdb_change_cpu_context (unsigned int cpu);
+extern void gdb_refresh_regcache (void);
#endif /* !GDB_COMMON */
diff --git a/gdb-10.2.patch b/gdb-10.2.patch
index a4e45fa0e6fa..9051c83ac784 100644
--- a/gdb-10.2.patch
+++ b/gdb-10.2.patch
@@ -494,13 +494,47 @@ exit 0
+#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
in a given list of threads. */
-@@ -1896,7 +1901,13 @@ thread_command (const char *tidstr, int from_tty)
+@@ -1093,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 ())
+ {
+@@ -1108,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)
+@@ -1180,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)
+@@ -1896,7 +1914,13 @@ thread_command (const char *tidstr, int from_tty)
{
ptid_t previous_ptid = inferior_ptid;
@@ -508,7 +542,7 @@ exit 0
+ 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 a8d60507dd95..edb728aab06d 100644
--- a/kernel.c
+++ b/kernel.c
@@ -6491,9 +6491,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;
@@ -6515,7 +6516,8 @@ set_cpu(int cpu)
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 64088eca29a1..19894ae3a266 100644
--- a/task.c
+++ b/task.c
@@ -706,6 +706,15 @@ task_init(void)
kt->boot_date.tv_nsec = 0;
}
+ /*
+ * Refresh gdb thread's regcache
+ *
+ * This is required since, threads 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.
+ * */
+ gdb_refresh_regcache();
+
tt->flags |= TASK_INIT_DONE;
}
diff --git a/tools.c b/tools.c
index 0f2db108838a..40a78cb392cc 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':
--
2.43.0
7 a.m.
New subject: [PATCH v4 5/5] fix 'info threads' command
On 12/15/23 15:59, Aditya Gupta wrote:
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 | 36 +++++++++++++++++++++++++++++++-----
defs.h | 3 ++-
gdb-10.2.patch | 40 +++++++++++++++++++++++++++++++++++++---
kernel.c | 6 ++++--
task.c | 9 +++++++++
tools.c | 2 +-
6 files changed, 84 insertions(+), 12 deletions(-)
diff --git a/crash_target.c b/crash_target.c
index 223a51acb1e1..96a65a1fa55c 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(void);
+extern "C" int set_cpu(int cpu, int print_context);
/* The crash target. */
@@ -139,19 +140,44 @@ crash_target_init (void)
/*
* Change gdb's thread context to the thread on given CPU
* */
-extern "C" int gdb_change_cpu_context (unsigned int 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 ();
+ inferior *inf = current_inferior ();
thread_info *tp = find_thread_ptid (inf, ptid);
if (tp == nullptr)
- return FALSE;
+ 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);
return TRUE;
}
+/* Refresh regcache of all gdb threads
+ *
+ * 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(void)
+{
+ inferior *inf = current_inferior();
+ int saved_cpu = inferior_thread()->ptid.tid();
+
+ for (thread_info * tp : inf->threads()) {
+ /* temporarily switch to the cpu so we get correct registers */
+ set_cpu(tp->ptid.tid(), FALSE);
+ target_fetch_registers(get_thread_regcache(tp), -1);
+ }
+
+ set_cpu(saved_cpu, FALSE);
+}
The 'saved_cpu' will overwrite the value of set_cpu(tp->ptid.tid(),
FALSE) in the for-loop, is it expected behavior?
In addition, I haven't tested how far it takes the time in the
multi-core environment, is it possible to implement a smaller
granularity refresh operation? For example: When we need switch to
another cpu, execute the refresh operation for corresponding regcache.
Just my thoughts, I did not try it, not sure if this is doable.
Thanks.
Lianbo
diff --git a/defs.h b/defs.h
index 5e6df14e947d..240e5c27b223 100644
--- a/defs.h
+++ b/defs.h
@@ -6044,7 +6044,7 @@ void parse_kernel_version(char *);
#define SHOW_LOG_AUDIT (0x8)
#define SHOW_LOG_CTIME (0x10)
#define SHOW_LOG_SAFE (0x20)
-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);
@@ -7980,5 +7980,6 @@ enum ppc64_renum {
/* crash_target.c */
extern int gdb_change_cpu_context (unsigned int cpu);
+extern void gdb_refresh_regcache (void);
#endif /* !GDB_COMMON */
diff --git a/gdb-10.2.patch b/gdb-10.2.patch
index a4e45fa0e6fa..9051c83ac784 100644
--- a/gdb-10.2.patch
+++ b/gdb-10.2.patch
@@ -494,13 +494,47 @@ exit 0
+#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
in a given list of threads. */
-@@ -1896,7 +1901,13 @@ thread_command (const char *tidstr, int from_tty)
+@@ -1093,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 ())
+ {
+@@ -1108,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)
+@@ -1180,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)
+@@ -1896,7 +1914,13 @@ thread_command (const char *tidstr, int from_tty)
{
ptid_t previous_ptid = inferior_ptid;
@@ -508,7 +542,7 @@ exit 0
+ 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 a8d60507dd95..edb728aab06d 100644
--- a/kernel.c
+++ b/kernel.c
@@ -6491,9 +6491,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;
@@ -6515,7 +6516,8 @@ set_cpu(int cpu)
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 64088eca29a1..19894ae3a266 100644
--- a/task.c
+++ b/task.c
@@ -706,6 +706,15 @@ task_init(void)
kt->boot_date.tv_nsec = 0;
}
+ /*
+ * Refresh gdb thread's regcache
+ *
+ * This is required since, threads 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.
+ * */
+ gdb_refresh_regcache();
+
tt->flags |= TASK_INIT_DONE;
}
diff --git a/tools.c b/tools.c
index 0f2db108838a..40a78cb392cc 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':
11:56 p.m.
New subject: [PATCH v4 5/5] fix 'info threads' command
Hello Lianbo,
Thanks for the reviews.
On Mon, Dec 18, 2023 at 09:00:54PM +0800, Lianbo Jiang wrote:
On 12/15/23 15:59, Aditya Gupta wrote:
> 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 | 36 +++++++++++++++++++++++++++++++-----
> defs.h | 3 ++-
> gdb-10.2.patch | 40 +++++++++++++++++++++++++++++++++++++---
> kernel.c | 6 ++++--
> task.c | 9 +++++++++
> tools.c | 2 +-
> 6 files changed, 84 insertions(+), 12 deletions(-)
>
> diff --git a/crash_target.c b/crash_target.c
> index 223a51acb1e1..96a65a1fa55c 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(void);
> +extern "C" int set_cpu(int cpu, int print_context);
> /* The crash target. */
> @@ -139,19 +140,44 @@ crash_target_init (void)
> /*
> * Change gdb's thread context to the thread on given CPU
> * */
> -extern "C" int gdb_change_cpu_context (unsigned int 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 ();
> + inferior *inf = current_inferior ();
> thread_info *tp = find_thread_ptid (inf, ptid);
> if (tp == nullptr)
> - return FALSE;
> + 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);
> return TRUE;
> }
> +/* Refresh regcache of all gdb threads
> + *
> + * 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(void)
> +{
> + inferior *inf = current_inferior();
> + int saved_cpu = inferior_thread()->ptid.tid();
> +
> + for (thread_info * tp : inf->threads()) {
> + /* temporarily switch to the cpu so we get correct registers */
> + set_cpu(tp->ptid.tid(), FALSE);
> + target_fetch_registers(get_thread_regcache(tp), -1);
> + }
> +
> + set_cpu(saved_cpu, FALSE);
> +}
The 'saved_cpu' will overwrite the value of set_cpu(tp->ptid.tid(), FALSE)
in the for-loop, is it expected behavior?
Yes that is expected behavior, the idea is to save the current cpu, then change
the cpu temporarily for each thread, then restore the context to 'saved_cpu'.
Didn't want to change the context before entring and after exiting this
function.
In addition, I haven't tested how far it takes the time in the multi-core
environment, is it possible to implement a smaller granularity refresh
operation? For example: When we need switch to another cpu, execute the
refresh operation for corresponding regcache.
Just my thoughts, I did not try it, not sure if this is doable.
True, a smaller granularity should be doable, but might not be a small diff.
Yes, it can take time on a bigger system. Ideally set_cpu in crash should NOT be
very costly, as all tasks have already been cached by crash for all cpus in
tt.context_array, though fetching the registers by gdb should be the more costly
operation.
With current code, it causes all registers to be loaded for all CPUs, so yes
increasing the burden in initialisation rather than lazy loading later.
A smaller impact change might be to refresh registers during
'gdb_change_cpu_context', 'select_thread', and 'task_init' (to
refresh for CPU
0, which at init time is unavailable).
Will try that change in next version.
Thanks for the idea
>
> Thanks.
>
> Lianbo
>
> > diff --git a/defs.h b/defs.h
> > index 5e6df14e947d..240e5c27b223 100644
> > --- a/defs.h
> > +++ b/defs.h
> > @@ -6044,7 +6044,7 @@ void parse_kernel_version(char *);
> > #define SHOW_LOG_AUDIT (0x8)
> > #define SHOW_LOG_CTIME (0x10)
> > #define SHOW_LOG_SAFE (0x20)
> > -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);
> > @@ -7980,5 +7980,6 @@ enum ppc64_renum {
> > /* crash_target.c */
> > extern int gdb_change_cpu_context (unsigned int cpu);
> > +extern void gdb_refresh_regcache (void);
> > #endif /* !GDB_COMMON */
> > diff --git a/gdb-10.2.patch b/gdb-10.2.patch
> > index a4e45fa0e6fa..9051c83ac784 100644
> > --- a/gdb-10.2.patch
> > +++ b/gdb-10.2.patch
> > @@ -494,13 +494,47 @@ exit 0
> > +#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
> > in a given list of threads. */
> > -@@ -1896,7 +1901,13 @@ thread_command (const char *tidstr, int from_tty)
> > +@@ -1093,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 ())
> > + {
> > +@@ -1108,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)
> > +@@ -1180,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)
> > +@@ -1896,7 +1914,13 @@ thread_command (const char *tidstr, int from_tty)
> > {
> > ptid_t previous_ptid = inferior_ptid;
> > @@ -508,7 +542,7 @@ exit 0
> > + 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 a8d60507dd95..edb728aab06d 100644
> > --- a/kernel.c
> > +++ b/kernel.c
> > @@ -6491,9 +6491,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;
> > @@ -6515,7 +6516,8 @@ set_cpu(int cpu)
> > 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 64088eca29a1..19894ae3a266 100644
> > --- a/task.c
> > +++ b/task.c
> > @@ -706,6 +706,15 @@ task_init(void)
> > kt->boot_date.tv_nsec = 0;
> > }
> > + /*
> > + * Refresh gdb thread's regcache
> > + *
> > + * This is required since, threads 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.
> > + * */
> > + gdb_refresh_regcache();
> > +
> > tt->flags |= TASK_INIT_DONE;
> > }
> > diff --git a/tools.c b/tools.c
> > index 0f2db108838a..40a78cb392cc 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':
>
4:54 a.m.
Hi Aditya,
On Fri, Dec 15, 2023 at 4:00 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
I tried to enable live debugging on ppc64 arch based on your patch but
failed. I cannot get the saved register state from kernel:
task_struct-> thread_struct -> struct pt_regs *regs. The "regs" value
I read is always 0.
I'm not very familiar with the ppc64 kernel, won't the kernel save its
registers into "struct pt_regs *regs"? Or is them been saved to
somewhere else?
Thanks,
Tao Liu
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-v4
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) up
(crash) 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.
Additional Notes:
=================
Sorry, it took a long time to send this version. Tried fixing 'info
threads' but wasn't able to. Gave it time again, and was able to fix it
this time after multiple days of debugging.
Some other things from last version review:
* 'info rv' not working:
It's not supported in gdb, instead we need to use 'info locals rv' or
'info variables rv'
* 'info variables' command hangs... and prints nothing after hanging for long
It likely hangs due to a lot of symbols being there, and it's trying to
get all gdb's output and page it, so Control+C messes it up, but if we pass
a regex filter to limit the output, eg. info variables rq, then it doesn't
hang, and prints the variables/symbols.
Even with gdb, ie. simply running 'gdb vmlinux vmcore' also hangs due
to the lot of symbols
* making crashing thread as default in gdb:
This is implemented now, along with synchronising crash & gdb contexts, in
patch #3
* 'info threads' not working:
This turned to be due to a bug in gdb_interface. I fixed 'info
threads' in 2 patches, to simplify it, first for the gdb_interface,
and another patch for setting the context correctly in crash
* other info commands:
I tested all the info commands, in crash along with this patch.
Most of those that fail in crash are due to gdb itself not supporting
them with vmcores, and other than that is the 'info pretty' command,
which might not be needed in crash anyways
* live debugging showing only one thread:
I tried it with crash, crash shows only the current thread, ie.
itself, so it does not have information of registers for the other
CPUs. Similarly gdb does not support live kernel debugging (without
connecting to a gdbstub/QEMU etc.).
If you need I can make it show the current thread id correctly for
the one thread, but I don't think it might help much with live
debugging
Hope, I set the context, thanks for the reviews, I replied and worked
on your suggestions, but got stuck there due to 'info threads'
Changelog:
==========
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
7:15 a.m.
Hi Tao,
On Fri, Dec 15, 2023 at 06:54:52PM +0800, Tao Liu wrote:
Hi Aditya,
On Fri, Dec 15, 2023 at 4:00 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
I tried to enable live debugging on ppc64 arch based on your patch but
failed. I cannot get the saved register state from kernel:
task_struct-> thread_struct -> struct pt_regs *regs. The "regs" value
I read is always 0.
Thanks for trying that. This is based on my observation, but the 'regs' should
not be NULL for most threads, but yes, it is NULL for let's say PID 0, which is
the default we have in crash during live debug session.
Such as I found the PID for my current crash process using 'pgrep -a crash',
then used the 'task' command in crash to get the information, it showed some
value for 'thread.regs', which I dump with 'x' to get registers:
crash> task 26657
PID: 26657 TASK: c00000000dc61780 CPU: 27 COMMAND: "crash"
struct task_struct {
...
thread = {
ksp = 13835058056980675152,
ksp_vsid = 0,
regs = 0xc0000000653d3e80,
...
}
crash> x/40g 0xc0000000653d3e80
0xc0000000653d3e80: 0x00000000000000b3 0x00007ffffcafe9f0
0xc0000000653d3e90: 0x0000000010c47a00 0x0000000000000004
0xc0000000653d3ea0: 0x000000001bbc0440 0x0000000000000008
0xc0000000653d3eb0: 0x00000000653e3eb0 0x00007fff943180c0
0xc0000000653d3ec0: 0x00007fff94310988 0x0000000000000000
0xc0000000653d3ed0: 0x0000000000000000 0x0000000000000000
0xc0000000653d3ee0: 0x0000000000000000 0x00007fff943180c0
0xc0000000653d3ef0: 0x0000000000000000 0xc0000000653d3ef0
0xc0000000653d3f00: 0x000000001096d5f8 0x0000000080000000
0xc0000000653d3f10: 0x0000000000000000 0x00007ffffcafeb28
0xc0000000653d3f20: 0xffffffffffffdfff 0x0000000010c41bc8
0xc0000000653d3f30: 0x0000000000000008 0x0000000000000004
0xc0000000653d3f40: 0x000000001095ebe0 0x0000000010c41bf0
0xc0000000653d3f50: 0x000000001cbfd460 0xc0000000653d3f58
0xc0000000653d3f60: 0x000000001cbfd460 0x0000000000000004
0xc0000000653d3f70: 0x0000000000000000 0x00000000653e3f78
0xc0000000653d3f80: 0x00007fff93d379a4 0x800000000280f033
0xc0000000653d3f90: 0x0000000000000004 0x0000000000000000
0xc0000000653d3fa0: 0x0000000000000000 0x0000000000000000
0xc0000000653d3fb0: 0x0000000044882284 0x0000000000000000
Here the first 32 8-byte values should be the General Purpose Registers on
PPC64, the 33rd 8-byte value (0x00007fff93d379a4) should be NIP, and 34th
should be Machine State Register (MSR), ie. (0x800000000280f033), and so on
So it should be able to get some registers, but for PID 0 atleast the .regs is
NULL.
That said, I think it might be more effort to implement this feature on live
debugging.
Currently this feature requires the current context to be set to the CPU for
the task/thread we want the backtrace for.
But on crash side, changing cpu context is not allowed in live debugging. And on
gdb side, we add only as many threads as the number of CPUs, in
'crash_target_init'. Both might have to be changed to allow support in live
debugging, but for that I guess tools like 'drgn' are already doing good.
Thanks,
Aditya Gupta
7:26 a.m.
Hello Lianbo and Tao,
On Fri, Dec 15, 2023 at 01:29:48PM +0530, Aditya Gupta wrote:
... <snip> ...
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...
Regarding these backtrace you posted earlier with the invalid address errors:
crash> gdb bt
#0 0xc000000000281298 in crash_setup_regs (gdb: invalid kernel virtual
address: fffffffffffffffb type: "gdb_readmem callback"
gdb: invalid kernel virtual address: fffffffffffffff7 type: "gdb_readmem
callback"
gdb: invalid kernel virtual address: fffffffffffffff3 type: "gdb_readmem
callback"
gdb: invalid kernel virtual address: fffffffffffffffb type: "gdb_readmem
callback"
gdb: invalid kernel virtual address: fffffffffffffff7 type: "gdb_readmem
callback"
gdb: invalid kernel virtual address: fffffffffffffff3 type: "gdb_readmem
callback"
oldregs=<optimized out>, newregs=0xc00000000c0f7908) at
./arch/powerpc/include/asm/kexec.h:69
#1 __crash_kexec (regs=<optimized out>) at kernel/kexec_core.c:975
#2 0xfffffffffffffffb in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
To identify that this is due to register mismatch issue in the kernel, and print
a warning, I found this code snippet in 'drgn' tool's code [1]:
// In most cases, nip (word 32) contains the program counter. But, the
// NT_PRSTATUS note in Linux kernel vmcores before Linux kernel commit
// b684c09f09e7 ("powerpc: update ppc_save_regs to save current r1 in
// pt_regs") (in v6.5) is odd, and the saved stack pointer (r1) is for
// the program counter in the link register (word 36). The fix was also
// backported to several stable branches. Unfortunately, there's no good
// way to detect it other than the kernel version.
bool r1_is_for_lr = linux_kernel_prstatus;
if (linux_kernel_prstatus) {
char *p = (char *)prog->vmcoreinfo.osrelease;
long major = strtol(p, &p, 10), minor = 0, patch = 0;
if (*p == '.') {
minor = strtol(p + 1, &p, 10);
if (*p == '.')
patch = strtol(p + 1, NULL, 10);
}
if (major > 6 || (major == 6 && minor >= 5))
r1_is_for_lr = false;
// Commit cc46085350ccae5f3a2a55a48ab93ebf328d5e24 in v6.4.4.
if (major == 6 && minor == 4 && patch >= 4)
r1_is_for_lr = false;
// Commit ca9465056e1a40ec0b729c115871b1b17755b631 in v6.3.13.
if (major == 6 && minor == 3 && patch >= 13)
r1_is_for_lr = false;
// Commit 865d128cab0ded06c41b06cfdc191ef3d121a95f in v6.1.39.
if (major == 6 && minor == 1 && patch >= 39)
r1_is_for_lr = false;
// Commit 3786416e1fa2ec491b25a0bae6deec163a8795d1 in v5.15.121.
if (major == 5 && minor == 15 && patch >= 121)
r1_is_for_lr = false;
}
The kernel version is the parameter they are using to see if the issue is there,
don't know how reliable it is, but should I implement this way and print a
warning ?
[1]
https://github.com/osandov/drgn/blob/a7f9db306764132db3c6344110a50510a7f8...
Thanks,
Aditya Gupta
7:16 a.m.
On 12/15/23 21:26, Aditya Gupta wrote:
Hello Lianbo and Tao,
On Fri, Dec 15, 2023 at 01:29:48PM +0530, Aditya Gupta wrote:
> ... <snip> ...
>
> 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...
>
Regarding these backtrace you posted earlier with the invalid address errors:
crash> gdb bt
#0 0xc000000000281298 in crash_setup_regs (gdb: invalid kernel virtual
address: fffffffffffffffb type: "gdb_readmem callback"
gdb: invalid kernel virtual address: fffffffffffffff7 type: "gdb_readmem
callback"
gdb: invalid kernel virtual address: fffffffffffffff3 type: "gdb_readmem
callback"
gdb: invalid kernel virtual address: fffffffffffffffb type: "gdb_readmem
callback"
gdb: invalid kernel virtual address: fffffffffffffff7 type: "gdb_readmem
callback"
gdb: invalid kernel virtual address: fffffffffffffff3 type: "gdb_readmem
callback"
oldregs=<optimized out>, newregs=0xc00000000c0f7908) at
./arch/powerpc/include/asm/kexec.h:69
#1 __crash_kexec (regs=<optimized out>) at kernel/kexec_core.c:975
#2 0xfffffffffffffffb in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
To identify that this is due to register mismatch issue in the kernel, and print
a warning, I found this code snippet in 'drgn' tool's code [1]:
// In most cases, nip (word 32) contains the program counter. But, the
// NT_PRSTATUS note in Linux kernel vmcores before Linux kernel commit
// b684c09f09e7 ("powerpc: update ppc_save_regs to save current r1 in
// pt_regs") (in v6.5) is odd, and the saved stack pointer (r1) is for
// the program counter in the link register (word 36). The fix was also
// backported to several stable branches. Unfortunately, there's no good
// way to detect it other than the kernel version.
bool r1_is_for_lr = linux_kernel_prstatus;
if (linux_kernel_prstatus) {
char *p = (char *)prog->vmcoreinfo.osrelease;
long major = strtol(p, &p, 10), minor = 0, patch = 0;
if (*p == '.') {
minor = strtol(p + 1, &p, 10);
if (*p == '.')
patch = strtol(p + 1, NULL, 10);
}
if (major > 6 || (major == 6 && minor >= 5))
r1_is_for_lr = false;
// Commit cc46085350ccae5f3a2a55a48ab93ebf328d5e24 in v6.4.4.
if (major == 6 && minor == 4 && patch >= 4)
r1_is_for_lr = false;
// Commit ca9465056e1a40ec0b729c115871b1b17755b631 in v6.3.13.
if (major == 6 && minor == 3 && patch >= 13)
r1_is_for_lr = false;
// Commit 865d128cab0ded06c41b06cfdc191ef3d121a95f in v6.1.39.
if (major == 6 && minor == 1 && patch >= 39)
r1_is_for_lr = false;
// Commit 3786416e1fa2ec491b25a0bae6deec163a8795d1 in v5.15.121.
if (major == 5 && minor == 15 && patch >= 121)
r1_is_for_lr = false;
}
The kernel version is the parameter they are using to see if the issue is there,
don't know how reliable it is, but should I implement this way and print a
In crash tool, sometimes we use the following code to check if the issue
exists, for example:
if (THIS_KERNEL_VERSION >= LINUX(6,5,0))
xxx
However, we do not recommend it in crash tool unless there is no better way.
Thanks.
Lianbo
warning ?
[1]
https://github.com/osandov/drgn/blob/a7f9db306764132db3c6344110a50510a7f8...
Thanks,
Aditya Gupta
11:34 p.m.
Hello Lianbo,
On Mon, Dec 18, 2023 at 09:16:30PM +0800, Lianbo Jiang wrote:
On 12/15/23 21:26, Aditya Gupta wrote:
> Hello Lianbo and Tao,
>
> On Fri, Dec 15, 2023 at 01:29:48PM +0530, Aditya Gupta wrote:
> > ... <snip> ...
> >
> > 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...
> >
> Regarding these backtrace you posted earlier with the invalid address errors:
>
> crash> gdb bt
> #0 0xc000000000281298 in crash_setup_regs (gdb: invalid kernel virtual
> address: fffffffffffffffb type: "gdb_readmem callback"
> gdb: invalid kernel virtual address: fffffffffffffff7 type: "gdb_readmem
> callback"
> gdb: invalid kernel virtual address: fffffffffffffff3 type: "gdb_readmem
> callback"
> gdb: invalid kernel virtual address: fffffffffffffffb type: "gdb_readmem
> callback"
> gdb: invalid kernel virtual address: fffffffffffffff7 type: "gdb_readmem
> callback"
> gdb: invalid kernel virtual address: fffffffffffffff3 type: "gdb_readmem
> callback"
> oldregs=<optimized out>, newregs=0xc00000000c0f7908) at
> ./arch/powerpc/include/asm/kexec.h:69
> #1 __crash_kexec (regs=<optimized out>) at kernel/kexec_core.c:975
> #2 0xfffffffffffffffb in ?? ()
> Backtrace stopped: previous frame inner to this frame (corrupt stack?)
>
> To identify that this is due to register mismatch issue in the kernel, and print
> a warning, I found this code snippet in 'drgn' tool's code [1]:
>
> // In most cases, nip (word 32) contains the program counter. But, the
> // NT_PRSTATUS note in Linux kernel vmcores before Linux kernel commit
> // b684c09f09e7 ("powerpc: update ppc_save_regs to save current r1 in
> // pt_regs") (in v6.5) is odd, and the saved stack pointer (r1) is for
> // the program counter in the link register (word 36). The fix was also
> // backported to several stable branches. Unfortunately, there's no good
> // way to detect it other than the kernel version.
> bool r1_is_for_lr = linux_kernel_prstatus;
> if (linux_kernel_prstatus) {
> char *p = (char *)prog->vmcoreinfo.osrelease;
> long major = strtol(p, &p, 10), minor = 0, patch = 0;
> if (*p == '.') {
> minor = strtol(p + 1, &p, 10);
> if (*p == '.')
> patch = strtol(p + 1, NULL, 10);
> }
> if (major > 6 || (major == 6 && minor >= 5))
> r1_is_for_lr = false;
> // Commit cc46085350ccae5f3a2a55a48ab93ebf328d5e24 in v6.4.4.
> if (major == 6 && minor == 4 && patch >= 4)
> r1_is_for_lr = false;
> // Commit ca9465056e1a40ec0b729c115871b1b17755b631 in v6.3.13.
> if (major == 6 && minor == 3 && patch >= 13)
> r1_is_for_lr = false;
> // Commit 865d128cab0ded06c41b06cfdc191ef3d121a95f in v6.1.39.
> if (major == 6 && minor == 1 && patch >= 39)
> r1_is_for_lr = false;
> // Commit 3786416e1fa2ec491b25a0bae6deec163a8795d1 in v5.15.121.
> if (major == 5 && minor == 15 && patch >= 121)
> r1_is_for_lr = false;
> }
>
> The kernel version is the parameter they are using to see if the issue is there,
> don't know how reliable it is, but should I implement this way and print a
In crash tool, sometimes we use the following code to check if the issue
exists, for example:
if (THIS_KERNEL_VERSION >= LINUX(6,5,0))
xxx
However, we do not recommend it in crash tool unless there is no better way.
Yes, I tried to think for alternative ways, but doesn't seem to be a concrete
way I can say that the issue exists.
Maybe first I will improve the patch series per your reviews and then see how
to handle this.
Thanks,
Aditya Gupta
>
> Thanks.
>
> Lianbo
>
> > warning ?
> >
> > [1]
https://github.com/osandov/drgn/blob/a7f9db306764132db3c6344110a50510a7f8...
> >
> > Thanks,
> > Aditya Gupta
> >
>
414
days inactive
418
days old
devel@lists.crash-utility.osci.io
16 comments
3 participants
participants (3)
-
Aditya Gupta -
Lianbo Jiang -
Tao Liu