[Crash-utility] Re: [[PATCH v2]] Clean up on top of one-thread-v2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Mar 31, 2024 at 9:12 PM Tao Liu <ltao@xxxxxxxxxx> wrote:
>
> Hi Alexey & Aditya,
>
> I made a rebase v2 [1] based on Alexey's v2 and Aditya's rebase [2],
> aiming to make the patchset easiter for upstream review.
>
> $ git log
> --- part3
> f38c944  ppc64: doesn't get the unavaliable registers' value
> 2f3f993 x86_64: doesn't get the unavaliable registers' value
> --- part 2
> b531447 arm64: Add gdb stack unwinding support
> 8a42408 Fix cpumask_t recursive dependence issue
> 8cefd1f Parse stack by inactive_stack_frame priorily if the struct is valid
> 66d99a5 x86_64: Add gdb stack unwinding support
> d1da7df ppc64: correct gdb passthroughs by implementing machdep->get_cpu_reg
> ---- part 1
> 67e6b95 Stop stack unwinding at non-kernel address
> cde2a99 Fix gdb_interface: restore gdb's output streams at end of gdb_interface
> 03a107a Print task pid/command instead of CPU index
> 07c6789 Rename get_cpu_reg to get_current_task_reg
> 8cbff50 Let crash change gdb context
> 1f63f3b Leave only one gdb thread for crash
> 214a5c9 Remove 'frame' from prohibited commands list
> ---
>
> part 1 is the common base or preparation of gdb stack unwinding, which
> merge the 3 of our patches for "crash_target.c, gdb_interface.c ...".
> part 2 is the arch specific stack unwinding support for ppc64, x86_64, arm64.
> part 3 is the code which I'm going to add to part2, for addressing the
> reg unavaliable issue. I agree with Alexey, that not all registers can
> be get from stack, those value should be given as "unavaliable"
> instead of 0, or it may mislead people when debugging. We can get this
> part improved later if we know how to get those "unavaliable" regs
> from kernel.
>
> As for Alexey's v2 patch, I made the following change in the rebase:
>
> 1) Removed the code which getting regs from inactive_task_frame in
> x86_64.c:x86_64_get_current_task_reg(), which is replica of
> x86_64.c:x86_64_get_stack_frame() counterpart.
>
> 2) Use "target_fetch_registers(get_current_regcache(), -1);" instead
> of "registers_changed();" in
> crash_target.c:gdb_change_thread_context(). The "registers_changed();"
> code will cause a "recursive secondary temporary file usage" bug. I
> didn't dig into the root cause.

3) I forget to mention item 3:
Removed the following code in crash_target.c:gdb_change_thread_context()

-  static ulong prev_task = 0;
-
-  /*
-   * Crash calls this method even if CURRENT task is not updated.
-   * Ignore it and keep gdb caches active.
-   */
-  if (task == prev_task)
-    return TRUE;
-  prev_task = task;

Because I experienced a bug as follows:
$ /tmp/crash-dev/crash vmcore vmlinux.gz
crash> gdb bt
#0  <unavailable> in ?? ()
Backtrace stopped: not enough registers or memory available to unwind further
crash> set -p
    PID: 11665
COMMAND: "bigmem"
   TASK: ffff800fd05f7600  [THREAD_INFO: ffff800fd05f7600]
    CPU: 0
  STATE: TASK_RUNNING (PANIC)
crash> gdb bt
#0  <unavailable> in ?? ()
Backtrace stopped: not enough registers or memory available to unwind further

When crash finished bootup, the task context should be panic task's.
But it doesn't get refreshed even re-call "set -p". Remove the above
code will fix this.


>
> Please have a test or review, any comments would be nice.
>
> [1]: https://github.com/liutgnu/crash-dev/tree/tao-rebase-v2
> [2]: https://github.com/adi-g15-ibm/crash/tree/stack-unwind-one-thread-merged
>
> Thanks,
> Tao Liu
>
> On Wed, Mar 27, 2024 at 4:05 AM Alexey Makhalov
> <alexey.makhalov@xxxxxxxxxxxx> wrote:
> >
> > Hi Tao,
> >
> > I need to see if there is any existing code in crash which is doing it.
> > I would like to avoid wheel reinvention.
> >
> > For just stack unwinding, IP/SP is enough. But to see function arguments and local variables
> > especially lower frames, gdb needs to know other GPRs.
> >
> > inactive_task_frame does not use pt_regs layout for saved registers and from the latest Linux sources
> > __switch_to_asm() does not provide dwarf/debug annotations regarding where the registers
> > were saved. That's why I ended up with an inactive_task_frame parsing implementation.
> >
> > About other kernel versions, I need to see how it has evolved over time. AFAIR, at some point
> > in the past it was pt_regs based.
> >
> > Thanks,
> > --Alexey
> >
> >
> > On Tue, Mar 26, 2024 at 2:14 AM Tao Liu <ltao@xxxxxxxxxx> wrote:
> >>
> >> Hi Alexey,
> >>
> >> For the rest of the patch, I didn't see any problem. But since it only
> >> supports modern kernels, could you please implement the rest, so I can
> >> have a testing for variety versions of kernels(ranging from 2.6.x to
> >> 5.x), in order to find if there are further problems.
> >>
> >> Thanks,
> >> Tao Liu
> >>
> >> On Mon, Mar 25, 2024 at 11:34 AM Tao Liu <ltao@xxxxxxxxxx> wrote:
> >> >
> >> > Hi Alexey,
> >> >
> >> > On Fri, Mar 22, 2024 at 7:20 AM Alexey Makhalov
> >> > <alexey.makhalov@xxxxxxxxxxxx> wrote:
> >> > >
> >> > > Hi folks, please take a look on v2 of this patch and give it a try.
> >> > > IMHO, Only remaining piece is to clean up x86_64_get_current_task_reg
> >> > > around getting pt_regs.
> >> > >
> >> > > Things done:
> >> > > 1. Rename machdep->get_cpu_reg to machdep->get_current_task_reg and rename
> >> > >    x86_64_get_cpu_reg to x86_64_get_current_task_reg, plus corresponding changes
> >> > >    for arm64 and ppc64.
> >> > > 2. Untied gdb's TPID from any tasks or CPUs ID. Always use CURRENT_CONTEXT
> >> > >    for regs and properties fetching.
> >> > > 3. When CURRENT switched, gdb_change_thread_context() must be called to
> >> > >    invalidate caches.
> >> > > 4. Update x86_64_get_current_task_reg to support inactive tasks for offline
> >> > >    debugging.
> >> > > 5. Fixed frame selection issue. It was caused by multiple calls of set_context()
> >> > >    which calls gdb_change_thread_context() unnecessarily dropping gdb caches.
> >> > >
> >> > > TODO:
> >> > > 1. x86_64_get_current_task_reg is questionable about fetching pt_regs for
> >> > >    inactive tasks. How do you gyus get pt_regs structure on modern linux kernels?
> >> > >    PS: I added my approach. More testing required.
> >> > >
> >> > > Known issues
> >> > > 1. After switching from all tasks to one thread:
> >> > >    b. unwinding started to work differently around top (garbaged) frames - Not quite
> >> > >    an issue for me.
> >> > >
> >> > > Signed-off-by: Alexey Makhalov <alexey.makhalov@xxxxxxxxxxxx>
> >> > > ---
> >> > >  arm64.c         |  4 ++--
> >> > >  crash_target.c  | 48 ++++++++++++++++++-------------------
> >> > >  defs.h          | 10 +++-----
> >> > >  gdb_interface.c |  8 +++----
> >> > >  kernel.c        | 23 ------------------
> >> > >  ppc64.c         |  8 +++----
> >> > >  task.c          | 16 ++++---------
> >> > >  x86_64.c        | 64 ++++++++++++++++++++++++++++++++++++++++++++-----
> >> > >  8 files changed, 99 insertions(+), 82 deletions(-)
> >> > >
> >> > > diff --git a/arm64.c b/arm64.c
> >> > > index 8ff10bf..340db2d 100644
> >> > > --- a/arm64.c
> >> > > +++ b/arm64.c
> >> > > @@ -153,7 +153,7 @@ static void arm64_calc_kernel_start(void)
> >> > >  }
> >> > >
> >> > >  static int
> >> > > -arm64_get_cpu_reg(int cpu, int regno, const char *name,
> >> > > +arm64_get_current_task_reg(int regno, const char *name,
> >> > >                     int size, void *value)
> >> > >  {
> >> > >         struct bt_info bt_info, bt_setup;
> >> > > @@ -511,7 +511,7 @@ arm64_init(int when)
> >> > >                 machdep->dumpfile_init = NULL;
> >> > >                 machdep->verify_line_number = NULL;
> >> > >                 machdep->init_kernel_pgd = arm64_init_kernel_pgd;
> >> > > -               machdep->get_cpu_reg = arm64_get_cpu_reg;
> >> > > +               machdep->get_current_task_reg = arm64_get_current_task_reg;
> >> > >
> >> > >                 /* use machdep parameters */
> >> > >                 arm64_calc_phys_offset();
> >> > > diff --git a/crash_target.c b/crash_target.c
> >> > > index fb3b634..6e96506 100644
> >> > > --- a/crash_target.c
> >> > > +++ b/crash_target.c
> >> > > @@ -26,11 +26,8 @@
> >> > >  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" void gdb_refresh_regcache(unsigned int cpu);
> >> > > -extern "C" int set_cpu(int cpu, int print_context);
> >> > > -extern "C" int crash_set_thread(ulong);
> >> > >  extern "C" int gdb_change_thread_context (ulong);
> >> > >  extern "C" void crash_get_current_task_info(unsigned long *pid, char **comm);
> >> > >
> >> > > @@ -72,22 +69,27 @@ 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))
> >> > >          error (_("fatal error: buffer size is not enough to fit register value"));
> >> > >
> >> > > -      if (crash_get_cpu_reg (cpu, r, regname, regsize, (void *)&regval))
> >> > > +      if (crash_get_current_task_reg (r, regname, regsize, (void *)&regval))
> >> > >          regcache->raw_supply (r, regval);
> >> > >        else
> >> > >          regcache->raw_supply (r, NULL);
> >> > > @@ -142,19 +144,17 @@ crash_target_init (void)
> >> > >  extern "C" int
> >> > >  gdb_change_thread_context (ulong task)
> >> > >  {
> >> > > -  inferior* inf = current_inferior();
> >> > > -  int cpu = crash_set_thread(task);
> >> > > -  if (cpu < 0)
> >> > > -    return FALSE;
> >> > > -
> >> > > -  ptid_t ptid = ptid_t(CRASH_INFERIOR_PID, 0, cpu);
> >> > > -   thread_info *tp = find_thread_ptid (inf, ptid);
> >> > > -
> >> > > -   if (tp == nullptr)
> >> > > -     return FALSE;
> >> > > -
> >> > > -   target_fetch_registers(get_thread_regcache(tp), -1);
> >> > > -   switch_to_thread(tp);
> >> > > -   reinit_frame_cache ();
> >> > > -   return TRUE;
> >> > > -}
> >> > > \ No newline at end of file
> >> > > +  static ulong prev_task = 0;
> >> > > +
> >> > > +  /*
> >> > > +   * Crash calls this method even if CURRENT task is not updated.
> >> > > +   * Ignore it and keep gdb caches active.
> >> > > +   */
> >> > > +  if (task == prev_task)
> >> > > +         return TRUE;
> >> > > +  prev_task = task;
> >> > > +
> >> > > +  registers_changed();
> >> > > +  reinit_frame_cache();
> >> > > +  return TRUE;
> >> > > +}
> >> > > diff --git a/defs.h b/defs.h
> >> > > index d7e3b18..7596179 100644
> >> > > --- a/defs.h
> >> > > +++ b/defs.h
> >> > > @@ -1081,7 +1081,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);
> >> > >  };
> >> > >
> >> > > @@ -6124,7 +6124,6 @@ void dump_kernel_table(int);
> >> > >  void dump_bt_info(struct bt_info *, char *where);
> >> > >  void dump_log(int);
> >> > >  void parse_kernel_version(char *);
> >> > > -int crash_set_thread(ulong);
> >> > >
> >> > >  #define LOG_LEVEL(v) ((v) & 0x07)
> >> > >  #define SHOW_LOG_LEVEL    (0x1)
> >> > > @@ -8023,7 +8022,7 @@ extern int have_full_symbols(void);
> >> > >
> >> > >  /*
> >> > >   * Register numbers must be in sync with gdb/features/i386/64bit-core.c
> >> > > - * to make crash_target->fetch_registers() ---> machdep->get_cpu_reg()
> >> > > + * to make crash_target->fetch_registers() ---> machdep->get_current_task_reg()
> >> > >   * working properly.
> >> > >   */
> >> > >  enum x86_64_regnum {
> >> > > @@ -8081,7 +8080,7 @@ enum arm64_regnum {
> >> > >
> >> > >  /*
> >> > >   * Register numbers to make crash_target->fetch_registers()
> >> > > - * ---> machdep->get_cpu_reg() work properly.
> >> > > + * ---> machdep->get_current_task_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
> >> > > @@ -8199,7 +8198,4 @@ enum ppc64_regnum {
> >> > >         PPC64_VRSAVE_REGNU = 139
> >> > >  };
> >> > >
> >> > > -/* crash_target.c */
> >> > > -extern void gdb_refresh_regcache (unsigned int cpu);
> >> > > -
> >> > >  #endif /* !GDB_COMMON */
> >> > > diff --git a/gdb_interface.c b/gdb_interface.c
> >> > > index 28264c2..2e9117a 100644
> >> > > --- a/gdb_interface.c
> >> > > +++ b/gdb_interface.c
> >> > > @@ -1073,14 +1073,14 @@ 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 *val);
> >> > >
> >> > > -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/kernel.c b/kernel.c
> >> > > index 446fd78..15b00dc 100644
> >> > > --- a/kernel.c
> >> > > +++ b/kernel.c
> >> > > @@ -6543,29 +6543,6 @@ set_cpu(int cpu, int print_context)
> >> > >                 show_context(CURRENT_CONTEXT());
> >> > >  }
> >> > >
> >> > > -int
> >> > > -crash_set_thread(ulong task)
> >> > > -{
> >> > > -       bool found = FALSE;
> >> > > -       struct task_context *tc = FIRST_CONTEXT();
> >> > > -
> >> > > -       for (int i = 0; i < RUNNING_TASKS(); i++, tc++) {
> >> > > -               if (tc->task == task) {
> >> > > -                       found = TRUE;
> >> > > -                       break;
> >> > > -               }
> >> > > -       }
> >> > > -
> >> > > -       if (!found)
> >> > > -               return -1;
> >> > > -
> >> > > -       if (CURRENT_TASK() == tc->task)
> >> > > -               return 0;
> >> > > -
> >> > > -       set_context(tc->task, NO_PID, TRUE);
> >> > > -       return 0;
> >> > > -}
> >> > > -
> >> > >  /*
> >> > >   *  Collect the irq_desc[] entry along with its associated handler and
> >> > >   *  action structures.
> >> > > diff --git a/ppc64.c b/ppc64.c
> >> > > index a87e621..aeccb70 100644
> >> > > --- a/ppc64.c
> >> > > +++ b/ppc64.c
> >> > > @@ -55,7 +55,7 @@ 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 *);
> >> > > -static int ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
> >> > > +static int ppc64_get_current_task_reg(int regno, const char *name, int size,
> >> > >                   void *value);
> >> > >  static void parse_cmdline_args(void);
> >> > >  static int ppc64_paca_percpu_offset_init(int);
> >> > > @@ -706,7 +706,7 @@ ppc64_init(int when)
> >> > >                                 error(FATAL, "cannot malloc hwirqstack buffer space.");
> >> > >                 }
> >> > >
> >> > > -               machdep->get_cpu_reg = ppc64_get_cpu_reg;
> >> > > +               machdep->get_current_task_reg = ppc64_get_current_task_reg;
> >> > >
> >> > >                 ppc64_init_paca_info();
> >> > >
> >> > > @@ -2506,7 +2506,7 @@ ppc64_print_eframe(char *efrm_str, struct ppc64_pt_regs *regs,
> >> > >  }
> >> > >
> >> > >  static int
> >> > > -ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
> >> > > +ppc64_get_current_task_reg(int regno, const char *name, int size,
> >> > >                   void *value)
> >> > >  {
> >> > >         struct bt_info bt_info, bt_setup;
> >> > > @@ -2555,7 +2555,7 @@ ppc64_get_cpu_reg(int cpu, int regno, const char *name, int size,
> >> > >         pt_regs = (struct ppc64_pt_regs *)bt_info.machdep;
> >> > >
> >> > >         if (!pt_regs) {
> >> > > -               error(WARNING, "pt_regs not available for cpu %d\n", cpu);
> >> > > +               error(WARNING, "pt_regs not available for cpu %d\n", tc->processor);
> >> > >                 return FALSE;
> >> > >         }
> >> > >
> >> > > diff --git a/task.c b/task.c
> >> > > index f7e5b05..5f1ddad 100644
> >> > > --- a/task.c
> >> > > +++ b/task.c
> >> > > @@ -11286,19 +11286,11 @@ check_stack_end_magic:
> >> > >                 fprintf(fp, "No stack overflows detected\n");
> >> > >  }
> >> > >
> >> > > +void crash_get_current_task_info(unsigned long *pid, char **comm);
> >> > >  void crash_get_current_task_info(unsigned long *pid, char **comm)
> >> > >  {
> >> > > -       int i;
> >> > > -       struct task_context *tc;
> >> > > +       struct task_context *tc = CURRENT_CONTEXT();
> >> > >
> >> > > -       tc = FIRST_CONTEXT();
> >> > > -       for (i = 0; i < RUNNING_TASKS(); i++, tc++)
> >> > > -               if (tc->task == CURRENT_TASK()) {
> >> > > -                       *pid = tc->pid;
> >> > > -                       *comm = tc->comm;
> >> > > -                       return;
> >> > > -               }
> >> > > -       *pid = 0;
> >> > > -       *comm = NULL;
> >> > > -       return;
> >> > > +       *pid = tc->pid;
> >> > > +       *comm = tc->comm;
> >> > >  }
> >> > > diff --git a/x86_64.c b/x86_64.c
> >> > > index 41e67c6..73bb353 100644
> >> > > --- a/x86_64.c
> >> > > +++ b/x86_64.c
> >> > > @@ -126,7 +126,7 @@ static int x86_64_get_framesize(struct bt_info *, ulong, ulong, char *);
> >> > >  static void x86_64_framesize_debug(struct bt_info *);
> >> > >  static void x86_64_get_active_set(void);
> >> > >  static int x86_64_get_kvaddr_ranges(struct vaddr_range *);
> >> > > -static int x86_64_get_cpu_reg(int, int, const char *, int, void *);
> >> > > +static int x86_64_get_current_task_reg(int, const char *, int, void *);
> >> > >  static int x86_64_verify_paddr(uint64_t);
> >> > >  static void GART_init(void);
> >> > >  static void x86_64_exception_stacks_init(void);
> >> > > @@ -207,7 +207,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_current_task_reg;
> >> > >                  if (machdep->cmdline_args[0])
> >> > >                          parse_cmdline_args();
> >> > >                 if ((string = pc->read_vmcoreinfo("relocate"))) {
> >> > > @@ -898,7 +898,7 @@ x86_64_dump_machdep_table(ulong arg)
> >> > >          fprintf(fp, "        is_page_ptr: x86_64_is_page_ptr()\n");
> >> > >          fprintf(fp, "       verify_paddr: x86_64_verify_paddr()\n");
> >> > >          fprintf(fp, "  get_kvaddr_ranges: x86_64_get_kvaddr_ranges()\n");
> >> > > -       fprintf(fp, "        get_cpu_reg: x86_64_get_cpu_reg()\n");
> >> > > +       fprintf(fp, "get_current_task_reg: x86_64_get_current_task_reg()\n");
> >> > >          fprintf(fp, "    init_kernel_pgd: x86_64_init_kernel_pgd()\n");
> >> > >          fprintf(fp, "clear_machdep_cache: x86_64_clear_machdep_cache()\n");
> >> > >         fprintf(fp, " xendump_p2m_create: %s\n", PVOPS_XEN() ?
> >> > > @@ -9186,7 +9186,7 @@ x86_64_get_kvaddr_ranges(struct vaddr_range *vrp)
> >> > >                 }
> >> > >
> >> > >  static int
> >> > > -x86_64_get_cpu_reg(int cpu, int regno, const char *name,
> >> > > +x86_64_get_current_task_reg(int regno, const char *name,
> >> > >                     int size, void *value)
> >> > >  {
> >> > >         struct bt_info bt_info, bt_setup;
> >> > > @@ -9195,8 +9195,6 @@ x86_64_get_cpu_reg(int cpu, int regno, const char *name,
> >> > >         ulong ip, sp;
> >> > >         bool ret = FALSE;
> >> > >
> >> > > -       if (VMSS_DUMPFILE())
> >> > > -               return vmware_vmss_get_cpu_reg(cpu, regno, name, size, value);
> >> > >         switch (regno) {
> >> > >         case RAX_REGNUM ... GS_REGNUM:
> >> > >         case FS_BASE_REGNUM ... ORIG_RAX_REGNUM:
> >> > > @@ -9208,6 +9206,60 @@ x86_64_get_cpu_reg(int cpu, int regno, const char *name,
> >> > >         tc = CURRENT_CONTEXT();
> >> > >         if (!tc)
> >> > >                 return FALSE;
> >> > > +
> >> > > +       /*
> >> > > +        * For inactive task, grab rip, rbp, rbx, r12, r13, r14 and r15 from
> >> > > +        * inactive_task_frame (see __switch_to_asm). Other regs saved on
> >> > > +        * regular frame.
> >> > > +        */
> >> > > +       if (!is_task_active(tc->task)) {
> >> > > +               int frame_size = STRUCT_SIZE("inactive_task_frame");
> >> > > +
> >> > > +               /* Only modern kernels supported. */
> >> > > +               if (tt->flags & THREAD_INFO && frame_size == 7 * 8) {
> >> > > +                       ulong rsp;
> >> > > +                       int offset = 0;
> >> > > +                       switch (regno) {
> >> > > +                               case RSP_REGNUM:
> >> > > +                                       readmem(tc->task + OFFSET(task_struct_thread) +
> >> > > +                                               OFFSET(thread_struct_rsp), KVADDR,
> >> > > +                                               &rsp, sizeof(void *),
> >> > > +                                               "thread_struct rsp", FAULT_ON_ERROR);
> >> > > +                                       rsp += frame_size;
> >> > > +                                       memcpy(value, &rsp, size);
> >> > > +                                       return TRUE;
> >> > > +                               case RIP_REGNUM:
> >> > > +                                       offset += 8;
> >> > > +                               case RBP_REGNUM:
> >> > > +                                       offset += 8;
> >> > > +                               case RBX_REGNUM:
> >> > > +                                       offset += 8;
> >> > > +                               case R12_REGNUM:
> >> > > +                                       offset += 8;
> >> > > +                               case R13_REGNUM:
> >> > > +                                       offset += 8;
> >> > > +                               case R14_REGNUM:
> >> > > +                                       offset += 8;
> >> > > +                               case R15_REGNUM:
> >> > > +                                       readmem(tc->task + OFFSET(task_struct_thread) +
> >> > > +                                               OFFSET(thread_struct_rsp), KVADDR,
> >> > > +                                               &rsp, sizeof(void *),
> >> > > +                                               "thread_struct rsp", FAULT_ON_ERROR);
> >> > > +                                       readmem(rsp + offset, KVADDR, value, sizeof(void *),
> >> > > +                                                       "inactive_thread_frame saved regs", FAULT_ON_ERROR);
> >> > > +                                       return TRUE;
> >> > > +                       }
> >> >
> >> > For this part, it looks similar to what
> >> > x86_64.c:x86_64_get_stack_frame():5016 is trying to do, which is
> >> > extracting reg values from stack. Is there a special consideration to
> >> > put it in here?
> >> >
> >> > IMHO, the x86_64_get_stack_frame() is responsible for extracting reg
> >> > values from stack frame or elf notes(aka the regs value reproducer).
> >> > And it will give a universal data structure, aka pt_regs, to
> >> > x86_64_get_current_task_reg() (aka the regs value consumer).
> >> >
> >> > In this design, the x86_64_get_current_task_reg() is decoupled from
> >> > stack, no matter inactive_task_frame or traditional linked list stack
> >> > frame or active tasks regs coming from elf notes, the consumer won't
> >> > care, all it need is a pt_regs structure and get values from it.
> >> > Personally I prefer this design. Any comments?
> >> >
> >> > For other parts of the patch, I'm still reviewing and testing.
> >> >
> >> > Thanks,
> >> > Tao Liu
> >> >
> >> > > +               }
> >> > > +               /* TBD: older kernels support. */
> >> > > +               return FALSE;
> >> > > +       }
> >> > > +
> >> > > +       /*
> >> > > +        * Task is active, grab CPU's registers
> >> > > +        */
> >> > > +       if (VMSS_DUMPFILE())
> >> > > +               return vmware_vmss_get_cpu_reg(tc->processor, regno, name, size, value);
> >> > > +
> >> > >         BZERO(&bt_setup, sizeof(struct bt_info));
> >> > >         clone_bt_info(&bt_setup, &bt_info, tc);
> >> > >         fill_stackbuf(&bt_info);
> >> > > --
> >> > > 2.39.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




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux