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

[Luis Machado <lgustavo@codesourcery.com>]

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