gdbarch: Use an anonymous union for target data in `gdbarch_info'
Commit Message
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
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
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?
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.
===================================================================
@@ -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);
===================================================================
@@ -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;
===================================================================
@@ -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;
===================================================================
@@ -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;
===================================================================
@@ -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;
===================================================================
@@ -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))
===================================================================
@@ -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);
===================================================================
@@ -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
===================================================================
@@ -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. */
===================================================================
@@ -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 };
===================================================================
@@ -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)
===================================================================
@@ -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);
}
===================================================================
@@ -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)