Re: [PATCH 3/3] Dynamic core regset sections support

Luis Machado
2013-06-07T14:43:54Z

H Andreas,

Thanks for the patch.

Just a general thought. If a core file section must not always be
generated, would it make more sense to use dynamic section information
for s390 instead of forcing all the other targets to use a mechanism
that is not required for them?

I'm not necessarily against this modification, but i was wondering about
other options.

See comments below...

> Index: gdb/gdb/ppc-linux-tdep.c
> ===================================================================
> --- gdb.orig/gdb/ppc-linux-tdep.c
> +++ gdb/gdb/ppc-linux-tdep.c
> @@ -256,53 +256,26 @@ ppc_linux_return_value (struct gdbarch *
> readbuf, writebuf);
> }
>
> -static struct core_regset_section ppc_linux_vsx_regset_sections[] =
> -{
> - { ".reg", 48 * 4, "general-purpose" },
> - { ".reg2", 264, "floating-point" },
> - { ".reg-ppc-vmx", 544, "ppc Altivec" },
> - { ".reg-ppc-vsx", 256, "POWER7 VSX" },
> - { NULL, 0}
> -};
> -
> -static struct core_regset_section ppc_linux_vmx_regset_sections[] =
> -{
> - { ".reg", 48 * 4, "general-purpose" },
> - { ".reg2", 264, "floating-point" },
> - { ".reg-ppc-vmx", 544, "ppc Altivec" },
> - { NULL, 0}
> -};
> -
> -static struct core_regset_section ppc_linux_fp_regset_sections[] =
> -{
> - { ".reg", 48 * 4, "general-purpose" },
> - { ".reg2", 264, "floating-point" },
> - { NULL, 0}
> -};
> -
> -static struct core_regset_section ppc64_linux_vsx_regset_sections[] =
> -{
> - { ".reg", 48 * 8, "general-purpose" },
> - { ".reg2", 264, "floating-point" },
> - { ".reg-ppc-vmx", 544, "ppc Altivec" },
> - { ".reg-ppc-vsx", 256, "POWER7 VSX" },
> - { NULL, 0}
> -};
> -
> -static struct core_regset_section ppc64_linux_vmx_regset_sections[] =
> +static void
> +ppc_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
> + iterate_over_regset_sections_cb *cb,
> + void *cb_data,
> + const struct regcache *regcache)
> {
> - { ".reg", 48 * 8, "general-purpose" },
> - { ".reg2", 264, "floating-point" },
> - { ".reg-ppc-vmx", 544, "ppc Altivec" },
> - { NULL, 0}
> -};
> + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> + int have_vsx = tdep->ppc_vr0_regnum != -1;
> + int have_altivec = tdep->ppc_vsr0_upper_regnum != -1;
>
> -static struct core_regset_section ppc64_linux_fp_regset_sections[] =
> -{
> - { ".reg", 48 * 8, "general-purpose" },
> - { ".reg2", 264, "floating-point" },
> - { NULL, 0}
> -};
> + if ((*cb) (".reg", 48 * 4, "general-purpose", cb_data))
> + return;
> + if ((*cb) (".reg2", 264, "floating-point", cb_data))
> + return;
> + if (have_altivec)
> + if ((*cb) (".reg-ppc-vmx", 544, "ppc Altivec", cb_data))
> + return;
> + if (have_vsx)
> + (*cb) (".reg-ppc-vsx", 256, "POWER7 VSX", cb_data);
> +}
>
> /* PLT stub in executable. */
> static struct ppc_insn_pattern powerpc32_plt_stub[] =
> @@ -1305,18 +1278,9 @@ ppc_linux_init_abi (struct gdbarch_info
> else
> set_gdbarch_gcore_bfd_target (gdbarch, "elf32-powerpc");
>
> - /* Supported register sections. */
> - if (tdesc_find_feature (info.target_desc,
> - "org.gnu.gdb.power.vsx"))
> - set_gdbarch_core_regset_sections (gdbarch,
> - ppc_linux_vsx_regset_sections);
> - else if (tdesc_find_feature (info.target_desc,
> - "org.gnu.gdb.power.altivec"))
> - set_gdbarch_core_regset_sections (gdbarch,
> - ppc_linux_vmx_regset_sections);
> - else
> - set_gdbarch_core_regset_sections (gdbarch,
> - ppc_linux_fp_regset_sections);
> + /* Iterator for supported register sections. */
> + set_gdbarch_iterate_over_regset_sections
> + (gdbarch, ppc_linux_iterate_over_regset_sections);
>
> if (powerpc_so_ops.in_dynsym_resolve_code == NULL)
> {
> @@ -1360,18 +1324,9 @@ ppc_linux_init_abi (struct gdbarch_info
> else
> set_gdbarch_gcore_bfd_target (gdbarch, "elf64-powerpc");
>
> - /* Supported register sections. */
> - if (tdesc_find_feature (info.target_desc,
> - "org.gnu.gdb.power.vsx"))
> - set_gdbarch_core_regset_sections (gdbarch,
> - ppc64_linux_vsx_regset_sections);
> - else if (tdesc_find_feature (info.target_desc,
> - "org.gnu.gdb.power.altivec"))
> - set_gdbarch_core_regset_sections (gdbarch,
> - ppc64_linux_vmx_regset_sections);
> - else
> - set_gdbarch_core_regset_sections (gdbarch,
> - ppc64_linux_fp_regset_sections);
> + /* Iterator for supported register sections. */
> + set_gdbarch_iterate_over_regset_sections
> + (gdbarch, ppc_linux_iterate_over_regset_sections);
> }
>
> /* PPC32 uses a different prpsinfo32 compared to most other Linux

I am commenting explicitly for the ppc bits, but this may apply to the
other backends as well.

Did you maybe forget to handle ppc64 core file sections in this change?
I see a core file register set entry for ppc64 being removed, like so:

- { ".reg", 48 * 8, "general-purpose" },

... but it is not being checked by the new code. Though the name is the
same, the size of the section is different compared to ppc32.

- { ".reg", 48 * 4, "general-purpose" },

Another general comment/suggestion. Removing static definitions of core
file register sets and replacing them with hardcoded arguments for a
pointer-based call to a callback function seems to scramble/confuse
things a little.

It may be nicer to keep the static definitions around, but iterate
through them in the new iterator function. What do you think?

Regards,
Luis