2:07 a.m.
On 5/31/24 5:21 PM, devel-request(a)lists.crash-utility.osci.io wrote:
Date: Fri, 31 May 2024 17:19:25 +0800
From: Tao Liu<ltao(a)redhat.com>
Subject: [Crash-utility] [PATCH v4 02/16] Leave only one gdb thread
for crash
To:devel@lists.crash-utility.osci.io
Cc: Alexey Makhalov<alexey.makhalov(a)broadcom.com>, Mahesh J
Salgaonkar<mahesh(a)linux.ibm.com>, "Naveen N . Rao"
<naveen.n.rao(a)linux.vnet.ibm.com>, Lianbo Jiang<lijiang(a)redhat.com>
Message-ID:<20240531091939.97828-3-ltao@redhat.com>
Content-Type: text/plain; charset=UTF-8
This patch is a preparation of gdb stack unwinding support.
There are 3 designs for supporting arbitrary tasks stack unwinding:
1) One gdb thread represent a task[1][2].
2) One gdb thread represent a cpu[3].
3) Leaving only one gdb thread[4].
1 & 2 have a flaw that, when there are lots of tasks/cpus, it will slow
the startup of crash, introduce complexity of the registers context
synchronization between crash and gdb, hard to cover live debug mode
etc. So here we used the 3rd design.
To switch task, or view arbitrary tasks stack unwinding, we will reuse
the current gdb thread, and load the target task's regcache to the
thread(see the next patch). This will simplify many code.
[1]:https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg0052...
[2]:https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg0052...
[3]:https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg0047...
[4]:https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg0054...
Co-developed-by: Aditya Gupta<adityag(a)linux.ibm.com>
Co-developed-by: Alexey Makhalov<alexey.makhalov(a)broadcom.com>
Co-developed-by: Tao Liu<ltao(a)redhat.com>
Cc: Sourabh Jain<sourabhjain(a)linux.ibm.com>
Cc: Hari Bathini<hbathini(a)linux.ibm.com>
Cc: Mahesh J Salgaonkar<mahesh(a)linux.ibm.com>
Cc: Naveen N. Rao<naveen.n.rao(a)linux.vnet.ibm.com>
Cc: Lianbo Jiang<lijiang(a)redhat.com>
Cc: HAGIO KAZUHITO(萩尾 一仁)<k-hagio-ab(a)nec.com>
Cc: Tao Liu<ltao(a)redhat.com>
Cc: Alexey Makhalov<alexey.makhalov(a)broadcom.com>
Signed-off-by: Tao Liu<ltao(a)redhat.com>
---
crash_target.c | 14 +++++---------
gdb_interface.c | 19 -------------------
2 files changed, 5 insertions(+), 28 deletions(-)
diff --git a/crash_target.c b/crash_target.c
index 4554806..1f62bf6 100644
--- a/crash_target.c
+++ b/crash_target.c
@@ -26,7 +26,6 @@
void crash_target_init (void);
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);
@@ -110,7 +109,6 @@ crash_target::xfer_partial (enum target_object object, const char
*annex,
void
crash_target_init (void)
{
- int nr_cpus = crash_get_nr_cpus();
crash_target *target = new crash_target ();
/* Own the target until it is successfully pushed. */
@@ -119,13 +117,11 @@ crash_target_init (void)
push_target (std::move (target_holder));
inferior_appeared (current_inferior (), CRASH_INFERIOR_PID);
- for (int i = 0; i < nr_cpus; i++)
- {
- thread_info *thread = add_thread_silent (target,
- ptid_t(CRASH_INFERIOR_PID, 0, i));
- if (!i)
- switch_to_thread (thread);
- }
+
+ /*Only create 1 gdb threads to view tasks' stack unwinding*/
+ thread_info *thread = add_thread_silent (target,
+ ptid_t(CRASH_INFERIOR_PID, 0, 0));
+ switch_to_thread (thread);
/* Fetch all registers from core file. */
target_fetch_registers (get_current_regcache (), -1);
diff --git a/gdb_interface.c b/gdb_interface.c
index 8f99a0d..ab1bd52 100644
--- a/gdb_interface.c
+++ b/gdb_interface.c
@@ -1067,25 +1067,6 @@ unsigned long crash_get_kaslr_offset(void)
}
/* Callbacks for crash_target */
-int crash_get_nr_cpus(void);
-int crash_get_cpu_reg (int cpu, int regno, const char *regname,
- int regsize, void *val);
-
Removing the above declaration will trigger a warning:
cc -c -g -DX86_64 -DLZO -DGDB_10_2 gdb_interface.c -Wall -O2
-Wstrict-prototypes -Wmissing-prototypes -fstack-protector
-Wformat-security
gdb_interface.c:1070:5: warning: no previous prototype for
‘crash_get_cpu_reg’ [-Wmissing-prototypes]
1070 | int crash_get_cpu_reg (int cpu, int regno, const char *regname,
| ^~~~~~~~~~~~~~~~~
Thanks
Lianbo
-int crash_get_nr_cpus(void)
-{
- if (SADUMP_DUMPFILE())
- return sadump_get_nr_cpus();
- else if (DISKDUMP_DUMPFILE())
- return diskdump_get_nr_cpus();
- else if (KDUMP_DUMPFILE())
- return kdump_get_nr_cpus();
- else if (VMSS_DUMPFILE())
- return vmware_vmss_get_nr_cpus();
-
- /* Just CPU #0 */
- return 1;
-}
-
int crash_get_cpu_reg (int cpu, int regno, const char *regname,
int regsize, void *value)
{
-- 2.40.1
5:16 a.m.
New subject: [PATCH v4 02/16] Leave only one gdb thread for crash
On 6/20/24 3:07 PM, Lianbo Jiang wrote:
Date: Fri, 31 May 2024 17:19:27 +0800
From: Tao Liu<ltao(a)redhat.com>
Subject: [Crash-utility] [PATCH v4 04/16] Rename get_cpu_reg to
get_current_task_reg
To:devel@lists.crash-utility.osci.io
Cc: Alexey Makhalov<alexey.makhalov(a)broadcom.com>, Mahesh J
Salgaonkar<mahesh(a)linux.ibm.com>, "Naveen N . Rao"
<naveen.n.rao(a)linux.vnet.ibm.com>, Lianbo Jiang<lijiang(a)redhat.com>
Message-ID:<20240531091939.97828-5-ltao@redhat.com>
Content-Type: text/plain; charset=UTF-8
Since we only leave one gdb thread, or 1 cpu thread, and can use cmd "set"
to switch task context by reusing the thread. So the word "get_cpu_reg",
which stands for "fetch registers' value for cpu thread x", is no longer
appropriate, better using "get_current_task_reg", which stands for "fetch
registers' value for the current task", and makes more sense.
Co-developed-by: Aditya Gupta<adityag(a)linux.ibm.com>
Co-developed-by: Alexey Makhalov<alexey.makhalov(a)broadcom.com>
Co-developed-by: Tao Liu<ltao(a)redhat.com>
Cc: Sourabh Jain<sourabhjain(a)linux.ibm.com>
Cc: Hari Bathini<hbathini(a)linux.ibm.com>
Cc: Mahesh J Salgaonkar<mahesh(a)linux.ibm.com>
Cc: Naveen N. Rao<naveen.n.rao(a)linux.vnet.ibm.com>
Cc: Lianbo Jiang<lijiang(a)redhat.com>
Cc: HAGIO KAZUHITO(萩尾 一仁)<k-hagio-ab(a)nec.com>
Cc: Tao Liu<ltao(a)redhat.com>
Cc: Alexey Makhalov<alexey.makhalov(a)broadcom.com>
Signed-off-by: Tao Liu<ltao(a)redhat.com>
---
crash_target.c | 5 ++---
defs.h | 2 +-
gdb_interface.c | 6 +++---
x86_64.c | 2 +-
4 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/crash_target.c b/crash_target.c
index 03718b5..52966f7 100644
--- a/crash_target.c
+++ b/crash_target.c
@@ -26,7 +26,7 @@
void crash_target_init (void);
extern "C" int gdb_readmem_callback(unsigned long, void *, int, int);
-extern "C" int crash_get_cpu_reg (int cpu, int regno, const char *regname,
+extern "C" int crash_get_current_task_reg (int regno, const char *regname,
int regsize, void *val);
extern "C" int gdb_change_thread_context ();
@@ -68,7 +68,6 @@ crash_target::fetch_registers (struct regcache *regcache, int regno)
{
int r;
gdb_byte regval[16];
- int cpu = inferior_ptid.tid();
struct gdbarch *arch = regcache->arch ();
if (regno >= 0) {
@@ -84,7 +83,7 @@ onetime:
if (regsize > sizeof (regval))
error (_("fatal error: buffer size is not enough to fit register
value"));
- if (crash_get_cpu_reg (cpu, r, regname, regsize, (void *)®val))
+ if (crash_get_current_task_reg (r, regname, regsize, (void *)®val))
regcache->raw_supply (r, regval);
else
regcache->raw_supply (r, NULL);
diff --git a/defs.h b/defs.h
index 0d872b2..012ffdc 100644
--- a/defs.h
+++ b/defs.h
@@ -1080,7 +1080,7 @@ struct machdep_table {
void (*get_irq_affinity)(int);
void (*show_interrupts)(int, ulong *);
int (*is_page_ptr)(ulong, physaddr_t *);
- int (*get_cpu_reg)(int, int, const char *, int, void *);
+ int (*get_current_task_reg)(int, const char *, int, void *);
int (*is_cpu_prstatus_valid)(int cpu);
};
diff --git a/gdb_interface.c b/gdb_interface.c
index ab1bd52..b13d5fd 100644
--- a/gdb_interface.c
+++ b/gdb_interface.c
@@ -1067,11 +1067,11 @@ unsigned long crash_get_kaslr_offset(void)
}
/* Callbacks for crash_target */
-int crash_get_cpu_reg (int cpu, int regno, const char *regname,
+int crash_get_current_task_reg (int regno, const char *regname,
int regsize, void *value)
{
- if (!machdep->get_cpu_reg)
+ if (!machdep->get_current_task_reg)
return FALSE;
- return machdep->get_cpu_reg(cpu, regno, regname, regsize, value);
+ return machdep->get_current_task_reg(regno, regname, regsize, value);
}
diff --git a/x86_64.c b/x86_64.c
index 0c21eb8..5ab2852 100644
--- a/x86_64.c
+++ b/x86_64.c
@@ -195,7 +195,7 @@ x86_64_init(int when)
machdep->machspec->irq_eframe_link = UNINITIALIZED;
machdep->machspec->irq_stack_gap = UNINITIALIZED;
machdep->get_kvaddr_ranges = x86_64_get_kvaddr_ranges;
- machdep->get_cpu_reg = x86_64_get_cpu_reg;
+ machdep->get_current_task_reg = x86_64_get_cpu_reg;
Still getting a compilation error:
cc -c -g -DX86_64 -DLZO -DGDB_10_2 x86_64.c -Wall -O2
-Wstrict-prototypes -Wmissing-prototypes -fstack-protector
-Wformat-security
In file included from x86_64.c:16:
defs.h:8058:1: warning: function declaration isn’t a prototype
[-Wstrict-prototypes]
8058 | extern int gdb_change_thread_context ();
| ^~~~~~
x86_64.c: In function ‘x86_64_init’:
x86_64.c:198:47: error: assignment to ‘int (*)(int, const char *, int,
void *)’ from incompatible pointer type ‘int (*)(int, int, const char
*, int, void *)’ [-Wincompatible-pointer-types]
198 | machdep->get_current_task_reg = x86_64_get_cpu_reg;
| ^
make[4]: *** [Makefile:432: x86_64.o] Error 1
make[3]: *** [Makefile:1877: gdb] Error 2
make[2]: *** [Makefile:278: rebuild] Error 2
make[1]: *** [Makefile:266: gdb_merge] Error 2
make: *** [Makefile:258: all] Error 2
Looks like the current changes are not complete because of removing a
parameter in the get_cpu_reg().
Thanks
Lianbo
if (machdep->cmdline_args[0])
parse_cmdline_args();
if ((string = pc->read_vmcoreinfo("relocate"))) {
-- 2.40.1
5:26 a.m.
New subject: [PATCH v4 04/16] Rename get_cpu_reg to get_current_task_reg
On 6/20/24 6:16 PM, Lianbo Jiang wrote:
On 6/20/24 3:07 PM, Lianbo Jiang wrote:
> Date: Fri, 31 May 2024 17:19:27 +0800
> From: Tao Liu<ltao(a)redhat.com>
> Subject: [Crash-utility] [PATCH v4 04/16] Rename get_cpu_reg to
> get_current_task_reg
> To:devel@lists.crash-utility.osci.io
> Cc: Alexey Makhalov<alexey.makhalov(a)broadcom.com>, Mahesh J
> Salgaonkar<mahesh(a)linux.ibm.com>, "Naveen N . Rao"
> <naveen.n.rao(a)linux.vnet.ibm.com>, Lianbo Jiang<lijiang(a)redhat.com>
> Message-ID:<20240531091939.97828-5-ltao@redhat.com>
> Content-Type: text/plain; charset=UTF-8
>
> Since we only leave one gdb thread, or 1 cpu thread, and can use cmd
> "set"
> to switch task context by reusing the thread. So the word "get_cpu_reg",
> which stands for "fetch registers' value for cpu thread x", is no
longer
> appropriate, better using "get_current_task_reg", which stands for
> "fetch
> registers' value for the current task", and makes more sense.
>
> Co-developed-by: Aditya Gupta<adityag(a)linux.ibm.com>
> Co-developed-by: Alexey Makhalov<alexey.makhalov(a)broadcom.com>
> Co-developed-by: Tao Liu<ltao(a)redhat.com>
> Cc: Sourabh Jain<sourabhjain(a)linux.ibm.com>
> Cc: Hari Bathini<hbathini(a)linux.ibm.com>
> Cc: Mahesh J Salgaonkar<mahesh(a)linux.ibm.com>
> Cc: Naveen N. Rao<naveen.n.rao(a)linux.vnet.ibm.com>
> Cc: Lianbo Jiang<lijiang(a)redhat.com>
> Cc: HAGIO KAZUHITO(萩尾 一仁)<k-hagio-ab(a)nec.com>
> Cc: Tao Liu<ltao(a)redhat.com>
> Cc: Alexey Makhalov<alexey.makhalov(a)broadcom.com>
> Signed-off-by: Tao Liu<ltao(a)redhat.com>
> ---
> crash_target.c | 5 ++---
> defs.h | 2 +-
> gdb_interface.c | 6 +++---
> x86_64.c | 2 +-
> 4 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/crash_target.c b/crash_target.c
> index 03718b5..52966f7 100644
> --- a/crash_target.c
> +++ b/crash_target.c
> @@ -26,7 +26,7 @@
> void crash_target_init (void);
> extern "C" int gdb_readmem_callback(unsigned long, void *, int,
> int);
> -extern "C" int crash_get_cpu_reg (int cpu, int regno, const char
> *regname,
> +extern "C" int crash_get_current_task_reg (int regno, const char
> *regname,
> int regsize, void *val);
> extern "C" int gdb_change_thread_context ();
> @@ -68,7 +68,6 @@ crash_target::fetch_registers (struct regcache
> *regcache, int regno)
> {
> int r;
> gdb_byte regval[16];
> - int cpu = inferior_ptid.tid();
> struct gdbarch *arch = regcache->arch ();
> if (regno >= 0) {
> @@ -84,7 +83,7 @@ onetime:
> if (regsize > sizeof (regval))
> error (_("fatal error: buffer size is not enough to fit
> register value"));
> - if (crash_get_cpu_reg (cpu, r, regname, regsize, (void
> *)®val))
> + if (crash_get_current_task_reg (r, regname, regsize, (void
> *)®val))
> regcache->raw_supply (r, regval);
> else
> regcache->raw_supply (r, NULL);
> diff --git a/defs.h b/defs.h
> index 0d872b2..012ffdc 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -1080,7 +1080,7 @@ struct machdep_table {
> void (*get_irq_affinity)(int);
> void (*show_interrupts)(int, ulong *);
> int (*is_page_ptr)(ulong, physaddr_t *);
> - int (*get_cpu_reg)(int, int, const char *, int, void *);
> + int (*get_current_task_reg)(int, const char *, int, void *);
> int (*is_cpu_prstatus_valid)(int cpu);
> };
> diff --git a/gdb_interface.c b/gdb_interface.c
> index ab1bd52..b13d5fd 100644
> --- a/gdb_interface.c
> +++ b/gdb_interface.c
> @@ -1067,11 +1067,11 @@ unsigned long crash_get_kaslr_offset(void)
> }
> /* Callbacks for crash_target */
> -int crash_get_cpu_reg (int cpu, int regno, const char *regname,
> +int crash_get_current_task_reg (int regno, const char *regname,
> int regsize, void *value)
> {
> - if (!machdep->get_cpu_reg)
> + if (!machdep->get_current_task_reg)
> return FALSE;
> - return machdep->get_cpu_reg(cpu, regno, regname, regsize,
> value);
> + return machdep->get_current_task_reg(regno, regname,
> regsize, value);
> }
> diff --git a/x86_64.c b/x86_64.c
> index 0c21eb8..5ab2852 100644
> --- a/x86_64.c
> +++ b/x86_64.c
> @@ -195,7 +195,7 @@ x86_64_init(int when)
> machdep->machspec->irq_eframe_link = UNINITIALIZED;
> machdep->machspec->irq_stack_gap = UNINITIALIZED;
> machdep->get_kvaddr_ranges = x86_64_get_kvaddr_ranges;
> - machdep->get_cpu_reg = x86_64_get_cpu_reg;
> + machdep->get_current_task_reg = x86_64_get_cpu_reg;
Still getting a compilation error:
cc -c -g -DX86_64 -DLZO -DGDB_10_2 x86_64.c -Wall -O2
-Wstrict-prototypes -Wmissing-prototypes -fstack-protector
-Wformat-security
In file included from x86_64.c:16:
defs.h:8058:1: warning: function declaration isn’t a prototype
[-Wstrict-prototypes]
8058 | extern int gdb_change_thread_context ();
| ^~~~~~
x86_64.c: In function ‘x86_64_init’:
x86_64.c:198:47: error: assignment to ‘int (*)(int, const char *,
int, void *)’ from incompatible pointer type ‘int (*)(int, int,
const char *, int, void *)’ [-Wincompatible-pointer-types]
198 | machdep->get_current_task_reg =
x86_64_get_cpu_reg;
| ^
make[4]: *** [Makefile:432: x86_64.o] Error 1
make[3]: *** [Makefile:1877: gdb] Error 2
make[2]: *** [Makefile:278: rebuild] Error 2
make[1]: *** [Makefile:266: gdb_merge] Error 2
make: *** [Makefile:258: all] Error 2
Looks like the current changes are not complete because of removing a
parameter in the get_cpu_reg().
Sorry, I did not change the email title, it should be:
Re: [PATCH v4 04/16] Rename get_cpu_reg to get_current_task_reg
Let me correct this one here.
Thanks
Lianbo
Thanks
Lianbo
> if (machdep->cmdline_args[0])
> parse_cmdline_args();
> if ((string = pc->read_vmcoreinfo("relocate"))) {
> -- 2.40.1
498
days inactive
498
days old
devel@lists.crash-utility.osci.io
2 comments
1 participants
participants (1)
-
Lianbo Jiang