Hi, Tao and Alex
Thank you for working on this.
On Fri, Jan 3, 2025 at 1:54 PM <devel-requ...@lists.crash-utility.osci.io>
wrote:
> Date: Fri, 3 Jan 2025 18:48:14 +1300
> From: Tao Liu <l...@redhat.com>
> Subject: [Crash-utility] [PATCH v1 2/5] Call cmd_bt silently after
> "set pid"
> To: devel@lists.crash-utility.osci.io
> Cc: alexey.makha...@broadcom.com
> Message-ID: <20250103054817.364053-3-l...@redhat.com>
> Content-Type: text/plain; charset="US-ASCII"; x-default=true
>
> Cmd bt will list multi-stacks of one task. After we "set <pid>" switch
> context to one task, we first need a bt call to detect the multi-stacks,
> however we don't want any console output from it, so a nullfp is used for
> output receive. The silent bt call is only triggered once as part of task
> context switch by cmd set.
>
> A array of user_regs pointers is reserved for each supported arch. If one
> extra stack found, a user_regs structure will be allocated for storing regs
> value of the stack.
>
> Co-developed-by: Alexey Makhalov <alexey.makha...@broadcom.com>
> Co-developed-by: Tao Liu <l...@redhat.com>
> Signed-off-by: Tao Liu <l...@redhat.com>
> ---
> arm64.c | 4 ++++
> crash_target.c | 5 +++++
> gdb-10.2.patch | 2 +-
> kernel.c | 20 ++++++++++++++++++++
> ppc64.c | 4 ++++
> x86_64.c | 3 +++
> 6 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/arm64.c b/arm64.c
> index 1cdde5f..8291301 100644
> --- a/arm64.c
> +++ b/arm64.c
> @@ -126,6 +126,10 @@ struct user_regs_bitmap_struct {
> ulong bitmap[32];
> };
>
>
I have no more comments about the patchset, only two things:
[1] This was defined in various architectures such as X86 64, aarch64 and
ppc64, can it be defined in a common file and included with define macros
such as "#if defined(X86_64) || defined(ARM64) || defined(PPC64)"?
+#define MAX_EXCEPTION_STACKS 7
> +ulong extra_stacks_idx = 0;
> +struct user_regs_bitmap_struct *extra_stacks_regs[MAX_EXCEPTION_STACKS] =
> {0};
> +
>
And also add code comments to say why it(max exception stacks) is 7.
[2] build warning
cc -c -g -DX86_64 -DLZO -DGDB_10_2 kernel.c -I./gdb-10.2/bfd
-I./gdb-10.2/include -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes
-fstack-protector -Wformat-security
kernel.c:12009:6: warning: no previous prototype for ‘silent_call_bt’
[-Wmissing-prototypes]
12009 | void silent_call_bt(void)
| ^~~~~~~~~~~~~~
static inline bool is_mte_kvaddr(ulong addr)
> {
> /* check for ARM64_MTE enabled */
> diff --git a/crash_target.c b/crash_target.c
> index c5ad1df..8cfa744 100644
> --- a/crash_target.c
> +++ b/crash_target.c
> @@ -31,6 +31,9 @@ extern "C" int crash_get_current_task_reg (int regno,
> const char *regname,
> extern "C" int gdb_change_thread_context (void);
> extern "C" int gdb_add_substack (int);
> extern "C" void crash_get_current_task_info(unsigned long *pid, char
> **comm);
> +#if defined (TARGET_X86_64) || defined (TARGET_ARM64) || defined
> (TARGET_PPC64)
> +extern "C" void silent_call_bt(void);
> +#endif
>
> /* The crash target. */
>
> @@ -164,6 +165,10 @@ gdb_change_thread_context (void)
> /* 3rd, refresh regcache for tid 0 */
> target_fetch_registers(get_current_regcache(), -1);
> reinit_frame_cache();
> +#if defined (TARGET_X86_64) || defined (TARGET_ARM64) || defined
> (TARGET_PPC64)
> + /* 4th, invoke bt silently to refresh the additional stacks */
> + silent_call_bt();
>
This looks tricky, but I have no idea for the time being, maybe we can
improve it in the future.
Other changes are fine to me.
Thanks
Lianbo
+#endif
> return TRUE;
> }
>
> diff --git a/gdb-10.2.patch b/gdb-10.2.patch
> index c867660..7baa925 100644
> --- a/gdb-10.2.patch
> +++ b/gdb-10.2.patch
> @@ -55,7 +55,7 @@ exit 0
> # your system doesn't have fcntl.h in /usr/include (which is where it
> # should be according to Posix).
> -DEFS = @DEFS@
> -+DEFS = -DCRASH_MERGE @DEFS@
> ++DEFS = -DCRASH_MERGE -DTARGET_${CRASH_TARGET} @DEFS@
> GDB_CFLAGS = -I. -I$(srcdir) -I$(srcdir)/config \
> -DLOCALEDIR="\"$(localedir)\"" $(DEFS)
>
> diff --git a/kernel.c b/kernel.c
> index 8c2e0ca..1c8f500 100644
> --- a/kernel.c
> +++ b/kernel.c
> @@ -12001,3 +12001,23 @@ int get_linux_banner_from_vmlinux(char *buf,
> size_t size)
>
> return TRUE;
> }
> +
> +#if defined(X86_64) || defined(ARM64) || defined(PPC64)
> +extern ulong extra_stacks_idx;
> +extern void *extra_stacks_regs[];
> +void silent_call_bt(void)
> +{
> + FILE *fp_save = fp;
> + fp = pc->nullfp;
> +
> + for (int i = 0; i < extra_stacks_idx; i++) {
> + FREEBUF(extra_stacks_regs[i]);
> + extra_stacks_regs[i] = NULL;
> + }
> + sprintf(pc->command_line, "bt\n");
> + argcnt = parse_line(pc->command_line, args);
> + optind = 1;
> + cmd_bt();
> + fp = fp_save;
> +}
> +#endif
> \ No newline at end of file
> diff --git a/ppc64.c b/ppc64.c
> index 7ac12fe..532eb3f 100644
> --- a/ppc64.c
> +++ b/ppc64.c
> @@ -80,6 +80,10 @@ struct user_regs_bitmap_struct {
> ulong bitmap[32];
> };
>
> +#define MAX_EXCEPTION_STACKS 7
> +ulong extra_stacks_idx = 0;
> +struct user_regs_bitmap_struct *extra_stacks_regs[MAX_EXCEPTION_STACKS] =
> {0};
> +
> static int is_opal_context(ulong sp, ulong nip)
> {
> uint64_t opal_start, opal_end;
> diff --git a/x86_64.c b/x86_64.c
> index 53ae3ef..e544be8 100644
> --- a/x86_64.c
> +++ b/x86_64.c
> @@ -160,6 +160,9 @@ struct user_regs_bitmap_struct {
> ulong bitmap[32];
> };
>
> +ulong extra_stacks_idx = 0;
> +struct user_regs_bitmap_struct *extra_stacks_regs[MAX_EXCEPTION_STACKS] =
> {0};
> +
> /*
> * Do all necessary machine-specific setup here. This is called several
> * times during initialization.
> --
> 2.47.0
>
--
Crash-utility mailing list -- devel@lists.crash-utility.osci.io
To unsubscribe send an email to devel-le...@lists.crash-utility.osci.io
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
