Re: [PATCH 2/2] Revert "crash_taget: fetch_registers support"

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

 



On Tue, Sep 27, 2022 at 4:27 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> wrote:
cc Alexey, reverting this affects VMware folks.

On 2022/09/27 15:27, Lianbo Jiang wrote:
> This reverts commit 2f967fb5ebd737ce5eadba462df35935122e8865. John
> Pittman reports that it causes a regression issue on the old Vmware
> vmcore as below:
>
>    crash> set scope dm_table_create
>    set: attempting to find/load "dm_mod" module debuginfo
>         MODULE       NAME                      BASE           SIZE  OBJECT FILE
>    ffffffffa0012dc0  dm_mod             ffffffffa0000000   102823  /home/2.6.32-754.35.1.el6.x86_64/kernel/drivers/md/dm-mod.ko.debug
>    scope: ffffffffa0006cf0 (dm_table_create)
>    crash> whatis struct dm_table
>    struct dm_table {
>        uint64_t features;
>        struct mapped_device *md;
>        unsigned int type;
>        unsigned int depth;
>        unsigned int counts[16];
>        sector_t *index[16];
>        unsigned int num_targets;
>        unsigned int num_allocated;
>        sector_t *highs;
>        struct dm_target *targets;
>        struct target_type *immutable_target_type;
>        unsigned int integrity_supported : 1;
>        unsigned int singleton : 1;
>        fmode_t mode;
>        struct list_head devices;
>        void (*event_fn)(void *);
>        void *event_context;
>        struct dm_md_mempools *mempools;
>        struct list_head target_callbacks;
>    }
>    SIZE: 312
>    crash> whatis _name_buckets        --->| After using the whatis _name_buckets command,
>    struct list_head _name_buckets[64];    |
>    crash> whatis struct dm_table      <---| it won't show the contents of the dm_table struct.
>    struct dm_table {
>        int undefined__;
>    }
>    SIZE: 312
>    crash>
>
> With the patch, this issue disappeared.
At first glance, I did not find any part of the patch that can cause
the issue.  Do you have any detailed information?

 
This issue is observed on an old Vmware vmcore, and looks like a regression issue.
I tried to dig into the detail, the raw_supply() operation may affect the gdb behavior, it
will store the register values to a cache in the gdb.

So far I haven't got a good solution, let's see if Alex has a better way to fix it, that would be welcome. Any thoughts?

Thanks.
Lianbo
 
Thanks,
Kazu

>
> Signed-off-by: Lianbo Jiang <lijiang@xxxxxxxxxx>
> ---
>   crash_target.c  | 33 +-------------------------------
>   defs.h          | 51 -------------------------------------------------
>   gdb_interface.c | 19 +++++-------------
>   vmware_vmss.c   | 51 +------------------------------------------------
>   x86_64.c        | 16 ----------------
>   5 files changed, 7 insertions(+), 163 deletions(-)
>
> diff --git a/crash_target.c b/crash_target.c
> index 455480679741..a123329019f5 100644
> --- a/crash_target.c
> +++ b/crash_target.c
> @@ -27,8 +27,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);
>   
>   
>   /* The crash target.  */
> @@ -46,7 +44,6 @@ public:
>     const target_info &info () const override
>     { return crash_target_info; }
>   
> -  void fetch_registers (struct regcache *, int) override;
>     enum target_xfer_status xfer_partial (enum target_object object,
>                                           const char *annex,
>                                           gdb_byte *readbuf,
> @@ -57,35 +54,13 @@ public:
>     bool has_all_memory () override { return true; }
>     bool has_memory () override { return true; }
>     bool has_stack () override { return true; }
> -  bool has_registers () override { return true; }
> +  bool has_registers () override { return false; }
>     bool thread_alive (ptid_t ptid) override { return true; }
>     std::string pid_to_str (ptid_t ptid) override
>     { return string_printf ("CPU %ld", ptid.tid ()); }
>   
>   };
>   
> -/* We just get all the registers, so we don't use regno.  */
> -void
> -crash_target::fetch_registers (struct regcache *regcache, int regno)
> -{
> -  gdb_byte regval[16];
> -  int cpu = inferior_ptid.tid();
> -  struct gdbarch *arch = regcache->arch ();
> -
> -  for (int r = 0; r < gdbarch_num_regs (arch); r++)
> -    {
> -      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))
> -        regcache->raw_supply (r, regval);
> -      else
> -        regcache->raw_supply (r, NULL);
> -    }
> -}
> -
>   
>   enum target_xfer_status
>   crash_target::xfer_partial (enum target_object object, const char *annex,
> @@ -126,10 +101,4 @@ crash_target_init (void)
>         if (!i)
>           switch_to_thread (thread);
>       }
> -
> -  /* Fetch all registers from core file.  */
> -  target_fetch_registers (get_current_regcache (), -1);
> -
> -  /* Now, set up the frame cache. */
> -  reinit_frame_cache ();
>   }
> diff --git a/defs.h b/defs.h
> index e0ccf2ddc9c2..e8acf49894b8 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -1015,7 +1015,6 @@ struct machdep_table {
>           ulong (*processor_speed)(void);
>           int (*uvtop)(struct task_context *, ulong, physaddr_t *, int);
>           int (*kvtop)(struct task_context *, ulong, physaddr_t *, int);
> -     int (*get_cpu_reg)(int, int, const char *, int, void *);
>           ulong (*get_task_pgd)(ulong);
>       void (*dump_irq)(int);
>       void (*get_stack_frame)(struct bt_info *, ulong *, ulong *);
> @@ -6977,7 +6976,6 @@ int vmware_vmss_get_nr_cpus(void);
>   int vmware_vmss_get_cr3_cr4_idtr(int, ulong *, ulong *, ulong *);
>   int vmware_vmss_phys_base(ulong *phys_base);
>   int vmware_vmss_set_phys_base(ulong);
> -int vmware_vmss_get_cpu_reg(int, int, const char *, int, void *);
>   
>   /*
>    * vmware_guestdump.c
> @@ -7403,53 +7401,4 @@ extern int have_full_symbols(void);
>   #define XEN_HYPERVISOR_ARCH
>   #endif
>   
> -/*
> - * Register numbers must be in sync with gdb/features/i386/64bit-core.c
> - * to make crash_target->fetch_registers() ---> machdep->get_cpu_reg()
> - * working properly.
> - */
> -enum x86_64_regnum {
> -        RAX_REGNUM,
> -        RBX_REGNUM,
> -        RCX_REGNUM,
> -        RDX_REGNUM,
> -        RSI_REGNUM,
> -        RDI_REGNUM,
> -        RBP_REGNUM,
> -        RSP_REGNUM,
> -        R8_REGNUM,
> -        R9_REGNUM,
> -        R10_REGNUM,
> -        R11_REGNUM,
> -        R12_REGNUM,
> -        R13_REGNUM,
> -        R14_REGNUM,
> -        R15_REGNUM,
> -        RIP_REGNUM,
> -        EFLAGS_REGNUM,
> -        CS_REGNUM,
> -        SS_REGNUM,
> -        DS_REGNUM,
> -        ES_REGNUM,
> -        FS_REGNUM,
> -        GS_REGNUM,
> -        ST0_REGNUM,
> -        ST1_REGNUM,
> -        ST2_REGNUM,
> -        ST3_REGNUM,
> -        ST4_REGNUM,
> -        ST5_REGNUM,
> -        ST6_REGNUM,
> -        ST7_REGNUM,
> -        FCTRL_REGNUM,
> -        FSTAT_REGNUM,
> -        FTAG_REGNUM,
> -        FISEG_REGNUM,
> -        FIOFF_REGNUM,
> -        FOSEG_REGNUM,
> -        FOOFF_REGNUM,
> -        FOP_REGNUM,
> -        LAST_REGNUM
> -};
> -
>   #endif /* !GDB_COMMON */
> diff --git a/gdb_interface.c b/gdb_interface.c
> index b14319c66147..574447fd82b7 100644
> --- a/gdb_interface.c
> +++ b/gdb_interface.c
> @@ -710,10 +710,11 @@ static char *prohibited_list[] = {
>       "run", "r", "break", "b", "tbreak", "hbreak", "thbreak", "rbreak",
>       "watch", "rwatch", "awatch", "attach", "continue", "c", "fg", "detach",
>       "finish", "handle", "interrupt", "jump", "kill", "next", "nexti",
> -     "signal", "step", "s", "stepi", "target", "until", "delete",
> -     "clear", "disable", "enable", "condition", "ignore", "frame", "catch",
> -     "tcatch", "return", "file", "exec-file", "core-file", "symbol-file",
> -     "load", "si", "ni", "shell", "sy",
> +     "signal", "step", "s", "stepi", "target", "thread", "until", "delete",
> +     "clear", "disable", "enable", "condition", "ignore", "frame",
> +     "select-frame", "f", "up", "down", "catch", "tcatch", "return",
> +     "file", "exec-file", "core-file", "symbol-file", "load", "si", "ni",
> +     "shell", "sy",
>       NULL  /* must be last */
>   };
>   
> @@ -1068,8 +1069,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);
>   
>   int crash_get_nr_cpus(void)
>   {
> @@ -1086,11 +1085,3 @@ int crash_get_nr_cpus(void)
>           return 1;
>   }
>   
> -int crash_get_cpu_reg (int cpu, int regno, const char *regname,
> -                       int regsize, void *value)
> -{
> -        if (!machdep->get_cpu_reg)
> -                return FALSE;
> -        return machdep->get_cpu_reg(cpu, regno, regname, regsize, value);
> -}
> -
> diff --git a/vmware_vmss.c b/vmware_vmss.c
> index f6c5f32ea4c0..151cfdbca5e5 100644
> --- a/vmware_vmss.c
> +++ b/vmware_vmss.c
> @@ -1,7 +1,7 @@
>   /*
>    * vmware_vmss.c
>    *
> - * Copyright (c) 2015, 2020 VMware, Inc.
> + * Copyright (c) 2015 VMware, Inc.
>    * Copyright (c) 2018 Red Hat Inc.
>    *
>    * This program is free software; you can redistribute it and/or modify
> @@ -16,7 +16,6 @@
>    *
>    * Authors: Dyno Hongjun Fu <hfu@xxxxxxxxxx>
>    *          Sergio Lopez <slp@xxxxxxxxxx>
> - *          Alexey Makhalov <amakhalov@xxxxxxxxxx>
>    */
>   
>   #include "defs.h"
> @@ -895,54 +894,6 @@ vmware_vmss_get_cr3_cr4_idtr(int cpu, ulong *cr3, ulong *cr4, ulong *idtr)
>       return TRUE;
>   }
>   
> -int
> -vmware_vmss_get_cpu_reg(int cpu, int regno, const char *name, int size,
> -                        void *value)
> -{
> -        if (cpu >= vmss.num_vcpus)
> -                return FALSE;
> -
> -        /* All supported registers are 8 bytes long. */
> -        if (size != 8)
> -                return FALSE;
> -
> -#define CASE(R,r) \
> -                case R##_REGNUM: \
> -                        if (!(vmss.vcpu_regs[cpu] & REGS_PRESENT_##R)) \
> -                                return FALSE; \
> -                        memcpy(value, &vmss.regs64[cpu]->r, size); \
> -                        break
> -
> -
> -        switch (regno) {
> -                CASE (RAX, rax);
> -                CASE (RBX, rbx);
> -                CASE (RCX, rcx);
> -                CASE (RDX, rdx);
> -                CASE (RSI, rsi);
> -                CASE (RDI, rdi);
> -                CASE (RBP, rbp);
> -                CASE (RSP, rsp);
> -                CASE (R8, r8);
> -                CASE (R9, r9);
> -                CASE (R10, r10);
> -                CASE (R11, r11);
> -                CASE (R12, r12);
> -                CASE (R13, r13);
> -                CASE (R14, r14);
> -                CASE (R15, r15);
> -                CASE (RIP, rip);
> -                case EFLAGS_REGNUM:
> -                        if (!(vmss.vcpu_regs[cpu] & REGS_PRESENT_RFLAGS))
> -                                return FALSE;
> -                        memcpy(value, &vmss.regs64[cpu]->rflags, size);
> -                        break;
> -                default:
> -                        return FALSE;
> -        }
> -        return TRUE;
> -}
> -
>   int
>   vmware_vmss_phys_base(ulong *phys_base)
>   {
> diff --git a/x86_64.c b/x86_64.c
> index 74bd1bbde41c..da94b1c0e917 100644
> --- a/x86_64.c
> +++ b/x86_64.c
> @@ -126,7 +126,6 @@ static int x86_64_get_framesize(struct bt_info *, ulong, ulong);
>   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_verify_paddr(uint64_t);
>   static void GART_init(void);
>   static void x86_64_exception_stacks_init(void);
> @@ -195,7 +194,6 @@ 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;
>                   if (machdep->cmdline_args[0])
>                           parse_cmdline_args();
>               if ((string = pc->read_vmcoreinfo("relocate"))) {
> @@ -889,7 +887,6 @@ 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, "    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() ?
> @@ -8957,19 +8954,6 @@ x86_64_get_kvaddr_ranges(struct vaddr_range *vrp)
>       return cnt;
>   }
>   
> -static int
> -x86_64_get_cpu_reg(int cpu, int regno, const char *name,
> -                   int size, void *value)
> -{
> -        if (regno >= LAST_REGNUM)
> -                return FALSE;
> -
> -        if (VMSS_DUMPFILE())
> -                return vmware_vmss_get_cpu_reg(cpu, regno, name, size, value);
> -
> -        return FALSE;
> -}
> -
>   /*
>    *  Determine the physical memory range reserved for GART.
>    */
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
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