[Crash-utility] Re: [PATCH v4 03/16] Let crash change gdb context

On 5/31/24 5:22 PM, devel-request(a)lists.crash-utility.osci.io wrote: > Date: Fri, 31 May 2024 17:19:26 +0800 > From: Tao Liu&lt;ltao(a)redhat.com&gt; > Subject: [Crash-utility] [PATCH v4 03/16] Let crash change gdb context > To:devel@lists.crash-utility.osci.io > Cc: Alexey Makhalov&lt;alexey.makhalov(a)broadcom.com&gt;, Mahesh J >     Salgaonkar&lt;mahesh(a)linux.ibm.com&gt;, "Naveen N . Rao" >     &lt;naveen.n.rao(a)linux.vnet.ibm.com&gt;, Lianbo Jiang&lt;lijiang(a)redhat.com&gt; > Message-ID:<20240531091939.97828-4-ltao@redhat.com> > Content-Type: text/plain; charset=UTF-8 > > This patch is a preparation of gdb stack unwinding support. "set" > command > is extended with gdb context change: > > crash> set <pid> > or > crash> set <task> > > Then the task context of crash and gdb will both be switched to > pid/task, and the following command: "bt" "gdb bt" will output > the same task context. > > Co-developed-by: Aditya Gupta&lt;adityag(a)linux.ibm.com&gt; > Co-developed-by: Alexey Makhalov&lt;alexey.makhalov(a)broadcom.com&gt; > Co-developed-by: Tao Liu&lt;ltao(a)redhat.com&gt; > Cc: Sourabh Jain&lt;sourabhjain(a)linux.ibm.com&gt; > Cc: Hari Bathini&lt;hbathini(a)linux.ibm.com&gt; > Cc: Mahesh J Salgaonkar&lt;mahesh(a)linux.ibm.com&gt; > Cc: Naveen N. Rao&lt;naveen.n.rao(a)linux.vnet.ibm.com&gt; > Cc: Lianbo Jiang&lt;lijiang(a)redhat.com&gt; > Cc: HAGIO KAZUHITO(萩尾 一仁)&lt;k-hagio-ab(a)nec.com&gt; > Cc: Tao Liu&lt;ltao(a)redhat.com&gt; > Cc: Alexey Makhalov&lt;alexey.makhalov(a)broadcom.com&gt; > Signed-off-by: Tao Liu&lt;ltao(a)redhat.com&gt; > --- >   crash_target.c | 20 +++++++++++++++++--- >   defs.h         |  5 ++++- >   kernel.c       | 11 ++++++++--- >   task.c         | 21 +++++++++++++-------- >   tools.c        |  8 ++++---- >   5 files changed, 46 insertions(+), 19 deletions(-) > > diff --git a/crash_target.c b/crash_target.c > index 1f62bf6..03718b5 100644 > --- a/crash_target.c > +++ b/crash_target.c > @@ -28,7 +28,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, >                                     int regsize, void *val); > - > +extern "C" int gdb_change_thread_context (); >     /* The crash target.  */ >   @@ -63,16 +63,22 @@ public: >     }; >   -/* We just get all the registers, so we don't use regno.  */ >   void >   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 (); >   -  for (int r = 0; r < gdbarch_num_regs (arch); r++) > +  if (regno >= 0) { > +    r = regno; > +    goto onetime; > +  } > + > +  for (r = 0; regno == -1 && r < gdbarch_num_regs (arch); r++) >       { > +onetime: >         const char *regname = gdbarch_register_name(arch, r); >         int regsize = register_size (arch, r); >         if (regsize > sizeof (regval)) > @@ -129,3 +135,11 @@ crash_target_init (void) >     /* Now, set up the frame cache. */ >     reinit_frame_cache (); >   } This function looks weird to me. Can you try to refactor this one? For example: +static void supply_registers(struct regcache *regcache, int regno) +{ +        gdb_byte regval[16]; +        struct gdbarch *arch = regcache->arch (); +        const char *regname = gdbarch_register_name(arch, regno); +        int regsize = register_size(arch, regno); + +        if (regsize > sizeof (regval)) +                error (_("fatal error: buffer size is not enough to fit register value")); + +        if (crash_get_current_task_reg (regno, regname, regsize, (void *)&regval)) +                regcache->raw_supply (regno, regval); +        else +                regcache->raw_supply (regno, NULL); +} void crash_target::fetch_registers (struct regcache *regcache, int regno) {   int r;   if (regno >= 0) {           supply_registers(regcache, regno);           return;   }   for (r = 0; regno == -1 && r < gdbarch_num_regs (regcache->arch ()); r++)             supply_registers(regcache, r); } That can avoid jumping into a for-loop code block with goto. And It looks more friendly and readable. Just an idea, What do you think? Thanks Lianbo > + > +extern "C" int > +gdb_change_thread_context () > +{ > +  target_fetch_registers(get_current_regcache(), -1); > +  reinit_frame_cache(); > +  return TRUE; > +}

> diff --git a/defs.h b/defs.h > index 3cb8e63..0d872b2 100644 > --- a/defs.h > +++ b/defs.h > @@ -6013,7 +6013,7 @@ extern char *help_map[]; >    *  task.c >    */ >   void task_init(void); > -int set_context(ulong, ulong); > +int set_context(ulong, ulong, uint); >   void show_context(struct task_context *); >   ulong pid_to_task(ulong); >   ulong task_to_pid(ulong); > @@ -8051,4 +8051,7 @@ enum x86_64_regnum { >           LAST_REGNUM >   }; >   +/* crash_target.c */ > +extern int gdb_change_thread_context (); > + >   #endif /* !GDB_COMMON */ > diff --git a/kernel.c b/kernel.c > index 1728b70..78b7b1e 100644 > --- a/kernel.c > +++ b/kernel.c > @@ -6494,15 +6494,20 @@ 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))) > -        set_context(task, NO_PID); > +    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, TRUE); >       else >           error(FATAL, "cannot determine active task on cpu %ld\n", > cpu); >         show_context(CURRENT_CONTEXT()); >   } >   - >   /* >    *  Collect the irq_desc[] entry along with its associated handler and >    *  action structures. > diff --git a/task.c b/task.c > index ebdb5be..d47d268 100644 > --- a/task.c > +++ b/task.c > @@ -672,7 +672,7 @@ task_init(void) >       if (ACTIVE()) { >           active_pid = REMOTE() ? pc->server_pid : >               LOCAL_ACTIVE() ? pc->program_pid : 1; > -        set_context(NO_TASK, active_pid); > +        set_context(NO_TASK, active_pid, FALSE); >           tt->this_task = pid_to_task(active_pid); >       } >       else { > @@ -684,7 +684,7 @@ task_init(void) >           else if (ELF_NOTES_VALID() && DISKDUMP_DUMPFILE()) >               map_cpus_to_prstatus_kdump_cmprs(); >           please_wait("determining panic task"); > -        set_context(get_panic_context(), NO_PID); > +        set_context(get_panic_context(), NO_PID, TRUE); >           please_wait_done(); >       } >   @@ -2985,9 +2985,9 @@ refresh_context(ulong curtask, ulong curpid) >       struct task_context *tc; >         if (task_exists(curtask) && pid_exists(curpid)) { > -                set_context(curtask, NO_PID); > +                set_context(curtask, NO_PID, FALSE); >           } else { > -                set_context(tt->this_task, NO_PID); > +                set_context(tt->this_task, NO_PID, FALSE); >                     complain = TRUE; >                   if (STREQ(args[0], "set") && (argcnt == 2) && > @@ -3053,7 +3053,7 @@ sort_context_array(void) >       curtask = CURRENT_TASK(); >       qsort((void *)tt->context_array, (size_t)tt->running_tasks, >               sizeof(struct task_context), sort_by_pid); > -    set_context(curtask, NO_PID); > +    set_context(curtask, NO_PID, TRUE); >         sort_context_by_task(); >   } > @@ -3100,7 +3100,7 @@ sort_context_array_by_last_run(void) >       curtask = CURRENT_TASK(); >       qsort((void *)tt->context_array, (size_t)tt->running_tasks, >               sizeof(struct task_context), sort_by_last_run); > -    set_context(curtask, NO_PID); > +    set_context(curtask, NO_PID, TRUE); >         sort_context_by_task(); >   } > @@ -5281,7 +5281,7 @@ comm_exists(char *s) >    *  that pid is selected. >    */ >   int > -set_context(ulong task, ulong pid) > +set_context(ulong task, ulong pid, uint update_gdb_thread) >   { >       int i; >       struct task_context *tc; > @@ -5301,7 +5301,12 @@ set_context(ulong task, ulong pid) >         if (found) { >           CURRENT_CONTEXT() = tc; > -        return TRUE; > + > +        /* change the selected thread in gdb, according to current > context */ > +        if (update_gdb_thread) > +            return gdb_change_thread_context(); > +        else > +            return TRUE; >       } else { >           if (task) >               error(INFO, "cannot set context for task: %lx\n", task); > diff --git a/tools.c b/tools.c > index 0f2db10..80d4244 100644 > --- a/tools.c > +++ b/tools.c > @@ -1871,7 +1871,7 @@ cmd_set(void) >                   return; >                 if (ACTIVE()) { > -                set_context(tt->this_task, NO_PID); > +                set_context(tt->this_task, NO_PID, TRUE); >                   show_context(CURRENT_CONTEXT()); >                   return; >               } > @@ -1880,7 +1880,7 @@ cmd_set(void) >                           error(INFO, "no panic task found!\n"); >                   return; >               } > -                set_context(tt->panic_task, NO_PID); > +            set_context(tt->panic_task, NO_PID, TRUE); >               show_context(CURRENT_CONTEXT()); >               return; >   @@ -2559,14 +2559,14 @@ cmd_set(void) >                       case STR_PID: >                                   pid = value; >                                   task = NO_TASK; > -                            if (set_context(task, pid)) > +                                if (set_context(task, pid, TRUE)) > show_context(CURRENT_CONTEXT()); >                               break; > >                       case STR_TASK: >                                   task = value; >                                   pid = NO_PID; > -                                if (set_context(task, pid)) > +                                if (set_context(task, pid, TRUE)) > show_context(CURRENT_CONTEXT()); >                               break; > > -- 2.40.1