Hi Lianbo,
Thanks for the patch review!
On Sun, Jan 26, 2025 at 8:02 PM lijiang <lijiang@xxxxxxxxxx> wrote:
>
> Hi, Tao and Alex
> Thank you for working on this.
>
> On Fri, Jan 3, 2025 at 1:54 PM <devel-request@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> Date: Fri, 3 Jan 2025 18:48:14 +1300
>> From: Tao Liu <ltao@xxxxxxxxxx>
>> Subject: [PATCH v1 2/5] Call cmd_bt silently after
>> "set pid"
>> To: devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
>> Cc: alexey.makhalov@xxxxxxxxxxxx
>> Message-ID: <20250103054817.364053-3-ltao@xxxxxxxxxx>
>> 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.makhalov@xxxxxxxxxxxx>
>> Co-developed-by: Tao Liu <ltao@xxxxxxxxxx>
>> Signed-off-by: Tao Liu <ltao@xxxxxxxxxx>
>> ---
>> 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)"?
OK, will give this a try.
>
>> +#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.
>
There is no reason for 7. Previously there has been a
MAX_EXCEPTION_STACKS for x86_64, so I take this for x86_64 multi-stack
support directly, and ported the code to arm64/ppc64 as well, so the
MAX_EXCEPTION_STACKS was there too. Frankly I have no idea the maximum
stacks a process may have, personally I think 3~4 is enough, e.g:
process stack/irq stack/exception stack. The max stacks I encountered
during coding is 3. So I think 7 is good enough for this on different
archs.
> [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)
> | ^~~~~~~~~~~~~~
>
Will get this fixed in v2.
>
>> 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.
>
Thanks! Please note this patchset will have conflict on the "x86_64:
Fix 'bt -S/-I' segfault issue" one, please merge the latter one first.
I will update this patchset accordingly in v2.
Thanks,
Tao Liu
Tao Liu
> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxx
> To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
> https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
> Contribution Guidelines: https://github.com/crash-utility/crash/wiki
--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
