diff mbox

gdbarch: Use an anonymous union for target data in `gdbarch_info'

Message ID alpine.DEB.2.00.1610181615410.31859@tp.orcam.me.uk
State Superseded
Headers show

Commit Message

Maciej W. Rozycki Oct. 18, 2016, 4:19 p.m. UTC
As an update to commit ede5f15146ae ("gdbarch.h: Change 
gdbarch_info::tdep_info's type to void *") replace the definition of the 
`tdep_info' member in `struct gdbarch_info' with an anonymous union, 
comprising the original member, with its type reverted to `struct 
gdbarch_tdep_info *', a `tdesc_data' member of a `struct tdesc_arch_data 
*' type and an `id' member of an `int *' type.  Remove now unnecessary 
casts throughout use places then, making code easier to read an less 
prone to errors, which may happen with casting.

	gdb/
	* gdbarch.sh (gdbarch_info): Replace the `tdep_info' member with
	a union of `tdep_info', `tdesc_data' and `id'.
	* aarch64-tdep.c (aarch64_gdbarch_init): Use `info.tdesc_data' 
	rather than `info.tdep_info'.
	* amd64-linux-tdep.c (amd64_linux_init_abi): Likewise.
	* i386-linux-tdep.c (i386_linux_init_abi): Likewise.
	* i386-tdep.c (i386_gdbarch_init): Likewise.
	* mips-linux-tdep.c (mips_linux_init_abi): Likewise.
	* mips-tdep.c (mips_gdbarch_init): Likewise.
	* nds32-tdep.c (nds32_gdbarch_init): Likewise.
	* rs6000-tdep.c (rs6000_gdbarch_init): Likewise.
	* ppc-linux-tdep.c (ppu2spu_sniffer): Use `info.id' rather than 
	`info.tdep_info'.
	(ppc_linux_init_abi): Use `info.tdesc_data' rather than 
	`info.tdep_info'.
	* spu-multiarch.c (spu_gdbarch): Use `info.id' rather than
	`info.tdep_info'.
	* spu-tdep.c (spu_gdbarch_init): Likewise.
	* gdbarch.h: Regenerate.
---
Hi,

 I made this change in response to my own concerns about casts expressed 
here: <https://sourceware.org/ml/gdb-patches/2016-07/msg00332.html> -- 
IMHO having casts sprinkled all over the place is not a sign of a 
particularly good coding style and it certainly makes code less readable 
and more prone to errors.  I decided to give the updated `tdep_info' 
member back its original type, as this will be used by the change the 
review of which I referred to above; for the record the change itself is 
here: <https://sourceware.org/ml/gdb-patches/2016-06/msg00425.html>.  
Besides, having a `void *' member does not make sense if the purpose of 
the change is to avoid casts.  This member is still used even now by 
gdbarch code itself.

 The predominant use for `tdep_info' is to store a pointer to `struct 
tdesc_arch_data', which is also what the MIPS target currently does, so I
concluded running regression testing for `mips-mti-linux-gnu' just to be 
sure I haven't botched something up would be enough.  There have been no 
regressions.  I have no SPU target to test, but the change is I believe 
obviously correct.

 OK to apply?

  Maciej

gdb-gdbarch-tdep-info-union.diff

Comments

Maciej W. Rozycki Nov. 8, 2016, 8:47 a.m. UTC | #1
On Tue, 18 Oct 2016, Maciej W. Rozycki wrote:

> 	gdb/
> 	* gdbarch.sh (gdbarch_info): Replace the `tdep_info' member with
> 	a union of `tdep_info', `tdesc_data' and `id'.
> 	* aarch64-tdep.c (aarch64_gdbarch_init): Use `info.tdesc_data' 
> 	rather than `info.tdep_info'.
> 	* amd64-linux-tdep.c (amd64_linux_init_abi): Likewise.
> 	* i386-linux-tdep.c (i386_linux_init_abi): Likewise.
> 	* i386-tdep.c (i386_gdbarch_init): Likewise.
> 	* mips-linux-tdep.c (mips_linux_init_abi): Likewise.
> 	* mips-tdep.c (mips_gdbarch_init): Likewise.
> 	* nds32-tdep.c (nds32_gdbarch_init): Likewise.
> 	* rs6000-tdep.c (rs6000_gdbarch_init): Likewise.
> 	* ppc-linux-tdep.c (ppu2spu_sniffer): Use `info.id' rather than 
> 	`info.tdep_info'.
> 	(ppc_linux_init_abi): Use `info.tdesc_data' rather than 
> 	`info.tdep_info'.
> 	* spu-multiarch.c (spu_gdbarch): Use `info.id' rather than
> 	`info.tdep_info'.
> 	* spu-tdep.c (spu_gdbarch_init): Likewise.
> 	* gdbarch.h: Regenerate.

 Can I ask for <https://patchwork.sourceware.org/patch/16622/> to be
reviewed?

  Maciej
Yao Qi Nov. 8, 2016, 9:20 p.m. UTC | #2
On Tue, Oct 18, 2016 at 05:19:50PM +0100, Maciej W. Rozycki wrote:
> Index: binutils/gdb/gdbarch.sh
> ===================================================================
> --- binutils.orig/gdb/gdbarch.sh	2016-10-18 02:37:31.000000000 +0100
> +++ binutils/gdb/gdbarch.sh	2016-10-18 02:47:30.048973683 +0100
> @@ -1459,7 +1459,12 @@ struct gdbarch_info
>    bfd *abfd;
>  
>    /* Use default: NULL (ZERO).  */
> -  void *tdep_info;
> +  union
> +    {
> +      struct gdbarch_tdep_info *tdep_info;
> +      struct tdesc_arch_data *tdesc_data;
> +      int *id;
> +    };
>  

Patch is good to me.  Could you add comments to each field about
when these fields are used?
Simon Marchi Nov. 8, 2016, 9:43 p.m. UTC | #3
On 2016-10-18 12:19, Maciej W. Rozycki wrote:
> As an update to commit ede5f15146ae ("gdbarch.h: Change
> gdbarch_info::tdep_info's type to void *") replace the definition of 
> the
> `tdep_info' member in `struct gdbarch_info' with an anonymous union,
> comprising the original member, with its type reverted to `struct
> gdbarch_tdep_info *', a `tdesc_data' member of a `struct 
> tdesc_arch_data
> *' type and an `id' member of an `int *' type.  Remove now unnecessary
> casts throughout use places then, making code easier to read an less
> prone to errors, which may happen with casting.

I glanced quickly at your patch, and I think it looks good.  As Yao 
said, some comments would be nice, because just like that it's hard to 
understand why those fields are grouped together in an union, as they 
seem unrelated to each other.
diff mbox

Patch

Index: binutils/gdb/aarch64-tdep.c
===================================================================
--- binutils.orig/gdb/aarch64-tdep.c	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/aarch64-tdep.c	2016-10-18 02:47:30.000642302 +0100
@@ -2807,7 +2807,7 @@  aarch64_gdbarch_init (struct gdbarch_inf
 
   /* Hook in the ABI-specific overrides, if they have been registered.  */
   info.target_desc = tdesc;
-  info.tdep_info = (void *) tdesc_data;
+  info.tdesc_data = tdesc_data;
   gdbarch_init_osabi (info, gdbarch);
 
   dwarf2_frame_set_init_reg (gdbarch, aarch64_dwarf2_frame_init_reg);
Index: binutils/gdb/amd64-linux-tdep.c
===================================================================
--- binutils.orig/gdb/amd64-linux-tdep.c	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/amd64-linux-tdep.c	2016-10-18 02:47:30.016044107 +0100
@@ -1852,8 +1852,7 @@  amd64_linux_init_abi (struct gdbarch_inf
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   const struct target_desc *tdesc = info.target_desc;
-  struct tdesc_arch_data *tdesc_data
-    = (struct tdesc_arch_data *) info.tdep_info;
+  struct tdesc_arch_data *tdesc_data = info.tdesc_data;
   const struct tdesc_feature *feature;
   int valid_p;
 
@@ -2069,8 +2068,7 @@  amd64_x32_linux_init_abi (struct gdbarch
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   const struct target_desc *tdesc = info.target_desc;
-  struct tdesc_arch_data *tdesc_data
-    = (struct tdesc_arch_data *) info.tdep_info;
+  struct tdesc_arch_data *tdesc_data = info.tdesc_data;
   const struct tdesc_feature *feature;
   int valid_p;
 
Index: binutils/gdb/gdbarch.h
===================================================================
--- binutils.orig/gdb/gdbarch.h	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/gdbarch.h	2016-10-18 02:47:30.030463430 +0100
@@ -1613,7 +1613,12 @@  struct gdbarch_info
   bfd *abfd;
 
   /* Use default: NULL (ZERO).  */
-  void *tdep_info;
+  union
+    {
+      struct gdbarch_tdep_info *tdep_info;
+      struct tdesc_arch_data *tdesc_data;
+      int *id;
+    };
 
   /* Use default: GDB_OSABI_UNINITIALIZED (-1).  */
   enum gdb_osabi osabi;
Index: binutils/gdb/gdbarch.sh
===================================================================
--- binutils.orig/gdb/gdbarch.sh	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/gdbarch.sh	2016-10-18 02:47:30.048973683 +0100
@@ -1459,7 +1459,12 @@  struct gdbarch_info
   bfd *abfd;
 
   /* Use default: NULL (ZERO).  */
-  void *tdep_info;
+  union
+    {
+      struct gdbarch_tdep_info *tdep_info;
+      struct tdesc_arch_data *tdesc_data;
+      int *id;
+    };
 
   /* Use default: GDB_OSABI_UNINITIALIZED (-1).  */
   enum gdb_osabi osabi;
Index: binutils/gdb/i386-linux-tdep.c
===================================================================
--- binutils.orig/gdb/i386-linux-tdep.c	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/i386-linux-tdep.c	2016-10-18 02:47:30.060328441 +0100
@@ -821,8 +821,7 @@  i386_linux_init_abi (struct gdbarch_info
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   const struct target_desc *tdesc = info.target_desc;
-  struct tdesc_arch_data *tdesc_data
-    = (struct tdesc_arch_data *) info.tdep_info;
+  struct tdesc_arch_data *tdesc_data = info.tdesc_data;
   const struct tdesc_feature *feature;
   int valid_p;
 
Index: binutils/gdb/i386-tdep.c
===================================================================
--- binutils.orig/gdb/i386-tdep.c	2016-10-18 02:47:24.000000000 +0100
+++ binutils/gdb/i386-tdep.c	2016-10-18 02:47:30.072577119 +0100
@@ -8563,7 +8563,7 @@  i386_gdbarch_init (struct gdbarch_info i
   set_gdbarch_insn_is_jump (gdbarch, i386_insn_is_jump);
 
   /* Hook in ABI-specific overrides, if they have been registered.  */
-  info.tdep_info = tdesc_data;
+  info.tdesc_data = tdesc_data;
   gdbarch_init_osabi (info, gdbarch);
 
   if (!i386_validate_tdesc_p (tdep, tdesc_data))
Index: binutils/gdb/mips-linux-tdep.c
===================================================================
--- binutils.orig/gdb/mips-linux-tdep.c	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/mips-linux-tdep.c	2016-10-18 02:47:30.083801277 +0100
@@ -1646,8 +1646,7 @@  mips_linux_init_abi (struct gdbarch_info
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   enum mips_abi abi = mips_abi (gdbarch);
-  struct tdesc_arch_data *tdesc_data
-    = (struct tdesc_arch_data *) info.tdep_info;
+  struct tdesc_arch_data *tdesc_data = info.tdesc_data;
 
   linux_init_abi (info, gdbarch);
 
Index: binutils/gdb/mips-tdep.c
===================================================================
--- binutils.orig/gdb/mips-tdep.c	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/mips-tdep.c	2016-10-18 02:47:30.101120720 +0100
@@ -8850,7 +8850,7 @@  mips_gdbarch_init (struct gdbarch_info i
   mips_register_g_packet_guesses (gdbarch);
 
   /* Hook in OS ABI-specific overrides, if they have been registered.  */
-  info.tdep_info = tdesc_data;
+  info.tdesc_data = tdesc_data;
   gdbarch_init_osabi (info, gdbarch);
 
   /* The hook may have adjusted num_regs, fetch the final value and
Index: binutils/gdb/nds32-tdep.c
===================================================================
--- binutils.orig/gdb/nds32-tdep.c	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/nds32-tdep.c	2016-10-18 03:32:20.843129509 +0100
@@ -2138,7 +2138,7 @@  nds32_gdbarch_init (struct gdbarch_info 
   nds32_add_reggroups (gdbarch);
 
   /* Hook in ABI-specific overrides, if they have been registered.  */
-  info.tdep_info = (void *) tdesc_data;
+  info.tdesc_data = tdesc_data;
   gdbarch_init_osabi (info, gdbarch);
 
   /* Override tdesc_register callbacks for system registers.  */
Index: binutils/gdb/ppc-linux-tdep.c
===================================================================
--- binutils.orig/gdb/ppc-linux-tdep.c	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/ppc-linux-tdep.c	2016-10-18 02:47:30.120562194 +0100
@@ -1352,7 +1352,7 @@  ppu2spu_sniffer (const struct frame_unwi
       info.bfd_arch_info = bfd_lookup_arch (bfd_arch_spu, bfd_mach_spu);
       info.byte_order = BFD_ENDIAN_BIG;
       info.osabi = GDB_OSABI_LINUX;
-      info.tdep_info = &data.id;
+      info.id = &data.id;
       data.gdbarch = gdbarch_find_by_info (info);
       if (!data.gdbarch)
 	return 0;
@@ -1652,8 +1652,7 @@  ppc_linux_init_abi (struct gdbarch_info 
                     struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
-  struct tdesc_arch_data *tdesc_data
-    = (struct tdesc_arch_data *) info.tdep_info;
+  struct tdesc_arch_data *tdesc_data = info.tdesc_data;
   static const char *const stap_integer_prefixes[] = { "i", NULL };
   static const char *const stap_register_indirection_prefixes[] = { "(",
 								    NULL };
Index: binutils/gdb/rs6000-tdep.c
===================================================================
--- binutils.orig/gdb/rs6000-tdep.c	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/rs6000-tdep.c	2016-10-18 02:47:30.139154795 +0100
@@ -6528,7 +6528,7 @@  rs6000_gdbarch_init (struct gdbarch_info
 
   /* Hook in ABI-specific overrides, if they have been registered.  */
   info.target_desc = tdesc;
-  info.tdep_info = tdesc_data;
+  info.tdesc_data = tdesc_data;
   gdbarch_init_osabi (info, gdbarch);
 
   switch (info.osabi)
Index: binutils/gdb/spu-multiarch.c
===================================================================
--- binutils.orig/gdb/spu-multiarch.c	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/spu-multiarch.c	2016-10-18 02:47:30.152561941 +0100
@@ -107,7 +107,7 @@  spu_gdbarch (int spufs_fd)
   info.bfd_arch_info = bfd_lookup_arch (bfd_arch_spu, bfd_mach_spu);
   info.byte_order = BFD_ENDIAN_BIG;
   info.osabi = GDB_OSABI_LINUX;
-  info.tdep_info = &spufs_fd;
+  info.id = &spufs_fd;
   return gdbarch_find_by_info (info);
 }
 
Index: binutils/gdb/spu-tdep.c
===================================================================
--- binutils.orig/gdb/spu-tdep.c	2016-10-18 02:37:31.000000000 +0100
+++ binutils/gdb/spu-tdep.c	2016-10-18 02:47:30.166769728 +0100
@@ -2697,8 +2697,8 @@  spu_gdbarch_init (struct gdbarch_info in
   int id = -1;
 
   /* Which spufs ID was requested as address space?  */
-  if (info.tdep_info)
-    id = *(int *)info.tdep_info;
+  if (info.id)
+    id = *info.id;
   /* For objfile architectures of SPU solibs, decode the ID from the name.
      This assumes the filename convention employed by solib-spu.c.  */
   else if (info.abfd)