* [PATCH 1/2] gdb: remove gdbarch_info::tdep_info
@ 2021-06-23 19:27 Simon Marchi
2021-06-23 19:27 ` [PATCH 2/2] gdb: remove gdbarch_info_init Simon Marchi
2021-06-25 20:24 ` [PATCH 1/2] gdb: remove gdbarch_info::tdep_info Andrew Burgess
0 siblings, 2 replies; 12+ messages in thread
From: Simon Marchi @ 2021-06-23 19:27 UTC (permalink / raw)
To: gdb-patches
This field is not actually used, remove it.
gdb/ChangeLog:
* gdbarch.sh (struct gdbarch_info) <tdep_info>: Remove.
(gdbarch_find_by_info): Remove print.
Change-Id: I00af4681b8e1a27727441cbadc3827f5914bd8eb
---
gdb/gdbarch.c | 3 ---
gdb/gdbarch.h | 4 ----
gdb/gdbarch.sh | 7 -------
3 files changed, 14 deletions(-)
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 208cf4b5aaab..830a86df89f3 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -5695,9 +5695,6 @@ gdbarch_find_by_info (struct gdbarch_info info)
fprintf_unfiltered (gdb_stdlog,
"gdbarch_find_by_info: info.abfd %s\n",
host_address_to_string (info.abfd));
- fprintf_unfiltered (gdb_stdlog,
- "gdbarch_find_by_info: info.tdep_info %s\n",
- host_address_to_string (info.tdep_info));
}
/* Find the tdep code that knows about this architecture. */
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 7157e5596fd3..36ea4de17c8c 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1795,10 +1795,6 @@ struct gdbarch_info
/* Use default: NULL (ZERO). */
union
{
- /* Architecture-specific information. The generic form for targets
- that have extra requirements. */
- struct gdbarch_tdep_info *tdep_info;
-
/* Architecture-specific target description data. Numerous targets
need only this, so give them an easy way to hold it. */
struct tdesc_arch_data *tdesc_data;
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 43e51341f97b..01a5ac88219d 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1547,10 +1547,6 @@ struct gdbarch_info
/* Use default: NULL (ZERO). */
union
{
- /* Architecture-specific information. The generic form for targets
- that have extra requirements. */
- struct gdbarch_tdep_info *tdep_info;
-
/* Architecture-specific target description data. Numerous targets
need only this, so give them an easy way to hold it. */
struct tdesc_arch_data *tdesc_data;
@@ -2479,9 +2475,6 @@ gdbarch_find_by_info (struct gdbarch_info info)
fprintf_unfiltered (gdb_stdlog,
"gdbarch_find_by_info: info.abfd %s\n",
host_address_to_string (info.abfd));
- fprintf_unfiltered (gdb_stdlog,
- "gdbarch_find_by_info: info.tdep_info %s\n",
- host_address_to_string (info.tdep_info));
}
/* Find the tdep code that knows about this architecture. */
--
2.32.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] gdb: remove gdbarch_info_init
2021-06-23 19:27 [PATCH 1/2] gdb: remove gdbarch_info::tdep_info Simon Marchi
@ 2021-06-23 19:27 ` Simon Marchi
2021-06-23 20:01 ` Luis Machado
2021-06-25 20:55 ` Andrew Burgess
2021-06-25 20:24 ` [PATCH 1/2] gdb: remove gdbarch_info::tdep_info Andrew Burgess
1 sibling, 2 replies; 12+ messages in thread
From: Simon Marchi @ 2021-06-23 19:27 UTC (permalink / raw)
To: gdb-patches
While reviewing another patch, I realized that gdbarch_info_init could
easily be removed in favor of initializing gdbarch_info fields directly
in the struct declaration. The only odd part is the union. I don't
know if it's actually important for it to be zero-initialized, but I
presume it is. I added a constructor to gdbarch_info to take care of
that. A proper solution would be to use std::variant. Or, these could
also be separate fields, the little extra space required wouldn't
matter.
gdb/ChangeLog:
* gdbarch.sh (struct gdbarch_info): Initialize fields, add
constructor.
* gdbarch.h: Re-generate.
* arch-utils.h (gdbarch_info_init): Remove, delete all usages.
* arch-utils.c (gdbarch_info_init): Remove.
Change-Id: I7502e08fe0f278d84eef1667a072e8a97bda5ab5
---
gdb/aarch64-linux-nat.c | 1 -
gdb/aarch64-tdep.c | 2 --
gdb/arch-utils.c | 26 +-------------------------
gdb/arch-utils.h | 6 ------
gdb/arm-tdep.c | 7 +------
gdb/cris-tdep.c | 3 ---
gdb/gdbarch.h | 28 ++++++++++++++--------------
gdb/gdbarch.sh | 28 ++++++++++++++--------------
gdb/i386-darwin-nat.c | 3 +--
gdb/inferior.c | 3 +--
gdb/mips-tdep.c | 5 -----
gdb/osabi.c | 4 +---
gdb/rs6000-nat.c | 3 +--
gdb/rs6000-tdep.c | 4 +---
gdb/selftest-arch.c | 1 -
gdb/target-descriptions.c | 2 --
16 files changed, 35 insertions(+), 91 deletions(-)
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 61224022f6aa..c7cbebbc3517 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -1060,7 +1060,6 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
new one), stashing the vector length inside id. Use -1 for when SVE
unavailable, to distinguish from an unset value of 0. */
struct gdbarch_info info;
- gdbarch_info_init (&info);
info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
info.id = (int *) (vq == 0 ? -1 : vq);
return gdbarch_find_by_info (info);
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 202b02108744..dea16e729c03 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -627,7 +627,6 @@ aarch64_analyze_prologue_test (void)
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
info.bfd_arch_info = bfd_scan_arch ("aarch64");
struct gdbarch *gdbarch = gdbarch_find_by_info (info);
@@ -4559,7 +4558,6 @@ aarch64_process_record_test (void)
struct gdbarch_info info;
uint32_t ret;
- gdbarch_info_init (&info);
info.bfd_arch_info = bfd_scan_arch ("aarch64");
struct gdbarch *gdbarch = gdbarch_find_by_info (info);
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index c75a79757ea2..4290d637ce16 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -389,8 +389,6 @@ set_endian (const char *ignore_args, int from_tty, struct cmd_list_element *c)
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
-
if (set_endian_string == endian_auto)
{
target_byte_order_user = BFD_ENDIAN_UNKNOWN;
@@ -548,8 +546,6 @@ set_architecture (const char *ignore_args,
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
-
if (strcmp (set_architecture_string, "auto") == 0)
{
target_architecture_user = NULL;
@@ -630,7 +626,6 @@ struct gdbarch *
gdbarch_from_bfd (bfd *abfd)
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
info.abfd = abfd;
return gdbarch_find_by_info (info);
@@ -645,7 +640,6 @@ set_gdbarch_from_file (bfd *abfd)
struct gdbarch_info info;
struct gdbarch *gdbarch;
- gdbarch_info_init (&info);
info.abfd = abfd;
info.target_desc = target_current_description ();
gdbarch = gdbarch_find_by_info (info);
@@ -679,10 +673,6 @@ void
initialize_current_architecture (void)
{
const char **arches = gdbarch_printable_names ();
- struct gdbarch_info info;
-
- /* determine a default architecture and byte order. */
- gdbarch_info_init (&info);
/* Find a default architecture. */
if (default_bfd_arch == NULL)
@@ -705,6 +695,7 @@ initialize_current_architecture (void)
_("initialize_current_architecture: Arch not found"));
}
+ gdbarch_info info;
info.bfd_arch_info = default_bfd_arch;
/* Take several guesses at a byte order. */
@@ -769,21 +760,6 @@ initialize_current_architecture (void)
}
}
-
-/* Initialize a gdbarch info to values that will be automatically
- overridden. Note: Originally, this ``struct info'' was initialized
- using memset(0). Unfortunately, that ran into problems, namely
- BFD_ENDIAN_BIG is zero. An explicit initialization function that
- can explicitly set each field to a well defined value is used. */
-
-void
-gdbarch_info_init (struct gdbarch_info *info)
-{
- memset (info, 0, sizeof (struct gdbarch_info));
- info->byte_order = BFD_ENDIAN_UNKNOWN;
- info->byte_order_for_code = info->byte_order;
-}
-
/* Similar to init, but this time fill in the blanks. Information is
obtained from the global "set ..." options and explicitly
initialized INFO fields. */
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index a5b40ad8ff1b..03e9082f6d77 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -196,12 +196,6 @@ extern enum bfd_endian selected_byte_order (void);
was explicitly selected. */
extern const char *selected_architecture_name (void);
-/* Initialize a ``struct info''. Can't use memset(0) since some
- default values are not zero. "fill" takes all available
- information and fills in any unspecified fields. */
-
-extern void gdbarch_info_init (struct gdbarch_info *info);
-
/* Similar to init, but this time fill in the blanks. Information is
obtained from the global "set ..." options and explicitly
initialized INFO fields. */
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 857a52a9a513..339e03271e7e 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8403,15 +8403,12 @@ arm_skip_stub (struct frame_info *frame, CORE_ADDR pc)
static void
arm_update_current_architecture (void)
{
- struct gdbarch_info info;
-
/* If the current architecture is not ARM, we have nothing to do. */
if (gdbarch_bfd_arch_info (target_gdbarch ())->arch != bfd_arch_arm)
return;
/* Update the architecture. */
- gdbarch_info_init (&info);
-
+ gdbarch_info info;
if (!gdbarch_update_p (info))
internal_error (__FILE__, __LINE__, _("could not update architecture"));
}
@@ -13237,7 +13234,6 @@ static void
arm_record_test (void)
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
info.bfd_arch_info = bfd_scan_arch ("arm");
struct gdbarch *gdbarch = gdbarch_find_by_info (info);
@@ -13329,7 +13325,6 @@ arm_analyze_prologue_test ()
for (bfd_endian endianness : {BFD_ENDIAN_LITTLE, BFD_ENDIAN_BIG})
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
info.byte_order = endianness;
info.byte_order_for_code = endianness;
info.bfd_arch_info = bfd_scan_arch ("arm");
diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c
index e1c141865cc6..01ef1604bffa 100644
--- a/gdb/cris-tdep.c
+++ b/gdb/cris-tdep.c
@@ -3879,7 +3879,6 @@ set_cris_version (const char *ignore_args, int from_tty,
usr_cmd_cris_version_valid = 1;
/* Update the current architecture, if needed. */
- gdbarch_info_init (&info);
if (!gdbarch_update_p (info))
internal_error (__FILE__, __LINE__,
_("cris_gdbarch_update: failed to update architecture."));
@@ -3892,7 +3891,6 @@ set_cris_mode (const char *ignore_args, int from_tty,
struct gdbarch_info info;
/* Update the current architecture, if needed. */
- gdbarch_info_init (&info);
if (!gdbarch_update_p (info))
internal_error (__FILE__, __LINE__,
"cris_gdbarch_update: failed to update architecture.");
@@ -3905,7 +3903,6 @@ set_cris_dwarf2_cfi (const char *ignore_args, int from_tty,
struct gdbarch_info info;
/* Update the current architecture, if needed. */
- gdbarch_info_init (&info);
if (!gdbarch_update_p (info))
internal_error (__FILE__, __LINE__,
_("cris_gdbarch_update: failed to update architecture."));
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 36ea4de17c8c..f94695adaa08 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -1781,18 +1781,20 @@ struct gdbarch_list
struct gdbarch_info
{
- /* Use default: NULL (ZERO). */
- const struct bfd_arch_info *bfd_arch_info;
+ gdbarch_info ()
+ /* Ensure the union is zero-initialized. Relies on the fact that there's
+ no member larget than TDESC_DATA. */
+ : tdesc_data ()
+ {}
- /* Use default: BFD_ENDIAN_UNKNOWN (NB: is not ZERO). */
- enum bfd_endian byte_order;
+ const struct bfd_arch_info *bfd_arch_info = nullptr;
- enum bfd_endian byte_order_for_code;
+ enum bfd_endian byte_order = BFD_ENDIAN_UNKNOWN;
- /* Use default: NULL (ZERO). */
- bfd *abfd;
+ enum bfd_endian byte_order_for_code = BFD_ENDIAN_UNKNOWN;
+
+ bfd *abfd = nullptr;
- /* Use default: NULL (ZERO). */
union
{
/* Architecture-specific target description data. Numerous targets
@@ -1805,11 +1807,9 @@ struct gdbarch_info
int *id;
};
- /* Use default: GDB_OSABI_UNINITIALIZED (-1). */
- enum gdb_osabi osabi;
+ enum gdb_osabi osabi = GDB_OSABI_UNKNOWN;
- /* Use default: NULL (ZERO). */
- const struct target_desc *target_desc;
+ const struct target_desc *target_desc = nullptr;
};
typedef struct gdbarch *(gdbarch_init_ftype) (struct gdbarch_info info, struct gdbarch_list *arches);
@@ -1883,8 +1883,8 @@ extern int gdbarch_update_p (struct gdbarch_info info);
/* Helper function. Find an architecture matching info.
- INFO should be initialized using gdbarch_info_init, relevant fields
- set, and then finished using gdbarch_info_fill.
+ INFO should have relevant fields set, and then finished using
+ gdbarch_info_fill.
Returns the corresponding architecture, or NULL if no matching
architecture was found. */
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 01a5ac88219d..e82d711ff2c3 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -1533,18 +1533,20 @@ struct gdbarch_list
struct gdbarch_info
{
- /* Use default: NULL (ZERO). */
- const struct bfd_arch_info *bfd_arch_info;
+ gdbarch_info ()
+ /* Ensure the union is zero-initialized. Relies on the fact that there's
+ no member larget than TDESC_DATA. */
+ : tdesc_data ()
+ {}
+
+ const struct bfd_arch_info *bfd_arch_info = nullptr;
- /* Use default: BFD_ENDIAN_UNKNOWN (NB: is not ZERO). */
- enum bfd_endian byte_order;
+ enum bfd_endian byte_order = BFD_ENDIAN_UNKNOWN;
- enum bfd_endian byte_order_for_code;
+ enum bfd_endian byte_order_for_code = BFD_ENDIAN_UNKNOWN;
- /* Use default: NULL (ZERO). */
- bfd *abfd;
+ bfd *abfd = nullptr;
- /* Use default: NULL (ZERO). */
union
{
/* Architecture-specific target description data. Numerous targets
@@ -1557,11 +1559,9 @@ struct gdbarch_info
int *id;
};
- /* Use default: GDB_OSABI_UNINITIALIZED (-1). */
- enum gdb_osabi osabi;
+ enum gdb_osabi osabi = GDB_OSABI_UNKNOWN;
- /* Use default: NULL (ZERO). */
- const struct target_desc *target_desc;
+ const struct target_desc *target_desc = nullptr;
};
typedef struct gdbarch *(gdbarch_init_ftype) (struct gdbarch_info info, struct gdbarch_list *arches);
@@ -1637,8 +1637,8 @@ extern int gdbarch_update_p (struct gdbarch_info info);
/* Helper function. Find an architecture matching info.
- INFO should be initialized using gdbarch_info_init, relevant fields
- set, and then finished using gdbarch_info_fill.
+ INFO should have relevant fields set, and then finished using
+ gdbarch_info_fill.
Returns the corresponding architecture, or NULL if no matching
architecture was found. */
diff --git a/gdb/i386-darwin-nat.c b/gdb/i386-darwin-nat.c
index 922d7a6df0ca..c1d6139490d8 100644
--- a/gdb/i386-darwin-nat.c
+++ b/gdb/i386-darwin-nat.c
@@ -479,7 +479,6 @@ darwin_check_osabi (darwin_inferior *inf, thread_t thread)
{
/* Attaching to a process. Let's figure out what kind it is. */
x86_thread_state_t gp_regs;
- struct gdbarch_info info;
unsigned int gp_count = x86_THREAD_STATE_COUNT;
kern_return_t ret;
@@ -491,7 +490,7 @@ darwin_check_osabi (darwin_inferior *inf, thread_t thread)
return;
}
- gdbarch_info_init (&info);
+ gdbarch_info info;
gdbarch_info_fill (&info);
info.byte_order = gdbarch_byte_order (target_gdbarch ());
info.osabi = GDB_OSABI_DARWIN;
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 059839ec9626..e1c70d553fde 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -747,7 +747,6 @@ add_inferior_with_spaces (void)
struct address_space *aspace;
struct program_space *pspace;
struct inferior *inf;
- struct gdbarch_info info;
/* If all inferiors share an address space on this system, this
doesn't really return a new address space; otherwise, it
@@ -760,7 +759,7 @@ add_inferior_with_spaces (void)
/* Setup the inferior's initial arch, based on information obtained
from the global "set ..." options. */
- gdbarch_info_init (&info);
+ gdbarch_info info;
inf->gdbarch = gdbarch_find_by_info (info);
/* The "set ..." options reject invalid settings, so we should
always have a valid arch by now. */
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index dbace1212508..2aee4f10b316 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -865,7 +865,6 @@ set_mips64_transfers_32bit_regs (const char *args, int from_tty,
struct cmd_list_element *c)
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
/* FIXME: cagney/2003-11-15: Should be setting a field in "info"
instead of relying on globals. Doing that would let generic code
handle the search for this specific architecture. */
@@ -6918,7 +6917,6 @@ static void
set_mipsfpu_single_command (const char *args, int from_tty)
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
mips_fpu_type = MIPS_FPU_SINGLE;
mips_fpu_type_auto = 0;
/* FIXME: cagney/2003-11-15: Should be setting a field in "info"
@@ -6932,7 +6930,6 @@ static void
set_mipsfpu_double_command (const char *args, int from_tty)
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
mips_fpu_type = MIPS_FPU_DOUBLE;
mips_fpu_type_auto = 0;
/* FIXME: cagney/2003-11-15: Should be setting a field in "info"
@@ -6946,7 +6943,6 @@ static void
set_mipsfpu_none_command (const char *args, int from_tty)
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
mips_fpu_type = MIPS_FPU_NONE;
mips_fpu_type_auto = 0;
/* FIXME: cagney/2003-11-15: Should be setting a field in "info"
@@ -8790,7 +8786,6 @@ mips_abi_update (const char *ignore_args,
/* Force the architecture to update, and (if it's a MIPS architecture)
mips_gdbarch_init will take care of the rest. */
- gdbarch_info_init (&info);
gdbarch_update_p (info);
}
diff --git a/gdb/osabi.c b/gdb/osabi.c
index b1603c09c855..aabf895c0451 100644
--- a/gdb/osabi.c
+++ b/gdb/osabi.c
@@ -598,8 +598,6 @@ generic_elf_osabi_sniffer (bfd *abfd)
static void
set_osabi (const char *args, int from_tty, struct cmd_list_element *c)
{
- struct gdbarch_info info;
-
if (strcmp (set_osabi_string, "auto") == 0)
user_osabi_state = osabi_auto;
else if (strcmp (set_osabi_string, "default") == 0)
@@ -630,7 +628,7 @@ set_osabi (const char *args, int from_tty, struct cmd_list_element *c)
/* NOTE: At some point (true multiple architectures) we'll need to be more
graceful here. */
- gdbarch_info_init (&info);
+ gdbarch_info info;
if (! gdbarch_update_p (info))
internal_error (__FILE__, __LINE__, _("Updating OS ABI failed."));
}
diff --git a/gdb/rs6000-nat.c b/gdb/rs6000-nat.c
index 50c96e281c64..ee00a600343a 100644
--- a/gdb/rs6000-nat.c
+++ b/gdb/rs6000-nat.c
@@ -558,7 +558,6 @@ rs6000_nat_target::create_inferior (const char *exec_file,
enum bfd_architecture arch;
unsigned long mach;
bfd abfd;
- struct gdbarch_info info;
inf_ptrace_target::create_inferior (exec_file, allargs, env, from_tty);
@@ -593,7 +592,7 @@ rs6000_nat_target::create_inferior (const char *exec_file,
bfd_default_set_arch_mach (&abfd, arch, mach);
- gdbarch_info_init (&info);
+ gdbarch_info info;
info.bfd_arch_info = bfd_get_arch_info (&abfd);
info.abfd = current_program_space->exec_bfd ();
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 5bcf5fca2c3b..a629450d746f 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -7260,7 +7260,6 @@ powerpc_set_soft_float (const char *args, int from_tty,
struct gdbarch_info info;
/* Update the architecture. */
- gdbarch_info_init (&info);
if (!gdbarch_update_p (info))
internal_error (__FILE__, __LINE__, _("could not update architecture"));
}
@@ -7269,7 +7268,6 @@ static void
powerpc_set_vector_abi (const char *args, int from_tty,
struct cmd_list_element *c)
{
- struct gdbarch_info info;
int vector_abi;
for (vector_abi = POWERPC_VEC_AUTO;
@@ -7287,7 +7285,7 @@ powerpc_set_vector_abi (const char *args, int from_tty,
powerpc_vector_abi_string);
/* Update the architecture. */
- gdbarch_info_init (&info);
+ gdbarch_info info;
if (!gdbarch_update_p (info))
internal_error (__FILE__, __LINE__, _("could not update architecture"));
}
diff --git a/gdb/selftest-arch.c b/gdb/selftest-arch.c
index ab1215c848c3..0eef134d4841 100644
--- a/gdb/selftest-arch.c
+++ b/gdb/selftest-arch.c
@@ -64,7 +64,6 @@ struct gdbarch_selftest : public selftest
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
info.bfd_arch_info = bfd_scan_arch (arches[i]);
struct gdbarch *gdbarch = gdbarch_find_by_info (info);
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index f7d0a688c62f..f94ace756d4d 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -555,7 +555,6 @@ target_find_description (void)
{
struct gdbarch_info info;
- gdbarch_info_init (&info);
info.target_desc = tdesc_info->tdesc;
if (!gdbarch_update_p (info))
warning (_("Architecture rejected target-supplied description"));
@@ -592,7 +591,6 @@ target_clear_description (void)
tdesc_info->tdesc = nullptr;
gdbarch_info info;
- gdbarch_info_init (&info);
if (!gdbarch_update_p (info))
internal_error (__FILE__, __LINE__,
_("Could not remove target-supplied description"));
--
2.32.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] gdb: remove gdbarch_info_init
2021-06-23 19:27 ` [PATCH 2/2] gdb: remove gdbarch_info_init Simon Marchi
@ 2021-06-23 20:01 ` Luis Machado
2021-06-23 20:35 ` Simon Marchi
2021-06-25 20:55 ` Andrew Burgess
1 sibling, 1 reply; 12+ messages in thread
From: Luis Machado @ 2021-06-23 20:01 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
On 6/23/21 4:27 PM, Simon Marchi via Gdb-patches wrote:
> While reviewing another patch, I realized that gdbarch_info_init could
> easily be removed in favor of initializing gdbarch_info fields directly
> in the struct declaration. The only odd part is the union. I don't
> know if it's actually important for it to be zero-initialized, but I
> presume it is. I added a constructor to gdbarch_info to take care of
> that. A proper solution would be to use std::variant. Or, these could
> also be separate fields, the little extra space required wouldn't
> matter.
>
> gdb/ChangeLog:
>
> * gdbarch.sh (struct gdbarch_info): Initialize fields, add
> constructor.
> * gdbarch.h: Re-generate.
> * arch-utils.h (gdbarch_info_init): Remove, delete all usages.
> * arch-utils.c (gdbarch_info_init): Remove.
>
> Change-Id: I7502e08fe0f278d84eef1667a072e8a97bda5ab5
> ---
> gdb/aarch64-linux-nat.c | 1 -
> gdb/aarch64-tdep.c | 2 --
> gdb/arch-utils.c | 26 +-------------------------
> gdb/arch-utils.h | 6 ------
> gdb/arm-tdep.c | 7 +------
> gdb/cris-tdep.c | 3 ---
> gdb/gdbarch.h | 28 ++++++++++++++--------------
> gdb/gdbarch.sh | 28 ++++++++++++++--------------
> gdb/i386-darwin-nat.c | 3 +--
> gdb/inferior.c | 3 +--
> gdb/mips-tdep.c | 5 -----
> gdb/osabi.c | 4 +---
> gdb/rs6000-nat.c | 3 +--
> gdb/rs6000-tdep.c | 4 +---
> gdb/selftest-arch.c | 1 -
> gdb/target-descriptions.c | 2 --
> 16 files changed, 35 insertions(+), 91 deletions(-)
>
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 61224022f6aa..c7cbebbc3517 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -1060,7 +1060,6 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
> new one), stashing the vector length inside id. Use -1 for when SVE
> unavailable, to distinguish from an unset value of 0. */
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
> info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
> info.id = (int *) (vq == 0 ? -1 : vq);
> return gdbarch_find_by_info (info);
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 202b02108744..dea16e729c03 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -627,7 +627,6 @@ aarch64_analyze_prologue_test (void)
> {
> struct gdbarch_info info;
>
> - gdbarch_info_init (&info);
> info.bfd_arch_info = bfd_scan_arch ("aarch64");
>
> struct gdbarch *gdbarch = gdbarch_find_by_info (info);
> @@ -4559,7 +4558,6 @@ aarch64_process_record_test (void)
> struct gdbarch_info info;
> uint32_t ret;
>
> - gdbarch_info_init (&info);
> info.bfd_arch_info = bfd_scan_arch ("aarch64");
>
> struct gdbarch *gdbarch = gdbarch_find_by_info (info);
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index c75a79757ea2..4290d637ce16 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -389,8 +389,6 @@ set_endian (const char *ignore_args, int from_tty, struct cmd_list_element *c)
> {
> struct gdbarch_info info;
>
> - gdbarch_info_init (&info);
> -
> if (set_endian_string == endian_auto)
> {
> target_byte_order_user = BFD_ENDIAN_UNKNOWN;
> @@ -548,8 +546,6 @@ set_architecture (const char *ignore_args,
> {
> struct gdbarch_info info;
>
> - gdbarch_info_init (&info);
> -
> if (strcmp (set_architecture_string, "auto") == 0)
> {
> target_architecture_user = NULL;
> @@ -630,7 +626,6 @@ struct gdbarch *
> gdbarch_from_bfd (bfd *abfd)
> {
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
>
> info.abfd = abfd;
> return gdbarch_find_by_info (info);
> @@ -645,7 +640,6 @@ set_gdbarch_from_file (bfd *abfd)
> struct gdbarch_info info;
> struct gdbarch *gdbarch;
>
> - gdbarch_info_init (&info);
> info.abfd = abfd;
> info.target_desc = target_current_description ();
> gdbarch = gdbarch_find_by_info (info);
> @@ -679,10 +673,6 @@ void
> initialize_current_architecture (void)
> {
> const char **arches = gdbarch_printable_names ();
> - struct gdbarch_info info;
> -
> - /* determine a default architecture and byte order. */
> - gdbarch_info_init (&info);
>
> /* Find a default architecture. */
> if (default_bfd_arch == NULL)
> @@ -705,6 +695,7 @@ initialize_current_architecture (void)
> _("initialize_current_architecture: Arch not found"));
> }
>
> + gdbarch_info info;
> info.bfd_arch_info = default_bfd_arch;
>
> /* Take several guesses at a byte order. */
> @@ -769,21 +760,6 @@ initialize_current_architecture (void)
> }
> }
>
> -
> -/* Initialize a gdbarch info to values that will be automatically
> - overridden. Note: Originally, this ``struct info'' was initialized
> - using memset(0). Unfortunately, that ran into problems, namely
> - BFD_ENDIAN_BIG is zero. An explicit initialization function that
> - can explicitly set each field to a well defined value is used. */
> -
> -void
> -gdbarch_info_init (struct gdbarch_info *info)
> -{
> - memset (info, 0, sizeof (struct gdbarch_info));
> - info->byte_order = BFD_ENDIAN_UNKNOWN;
> - info->byte_order_for_code = info->byte_order;
> -}
> -
> /* Similar to init, but this time fill in the blanks. Information is
> obtained from the global "set ..." options and explicitly
> initialized INFO fields. */
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index a5b40ad8ff1b..03e9082f6d77 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -196,12 +196,6 @@ extern enum bfd_endian selected_byte_order (void);
> was explicitly selected. */
> extern const char *selected_architecture_name (void);
>
> -/* Initialize a ``struct info''. Can't use memset(0) since some
> - default values are not zero. "fill" takes all available
> - information and fills in any unspecified fields. */
> -
> -extern void gdbarch_info_init (struct gdbarch_info *info);
> -
> /* Similar to init, but this time fill in the blanks. Information is
> obtained from the global "set ..." options and explicitly
> initialized INFO fields. */
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 857a52a9a513..339e03271e7e 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -8403,15 +8403,12 @@ arm_skip_stub (struct frame_info *frame, CORE_ADDR pc)
> static void
> arm_update_current_architecture (void)
> {
> - struct gdbarch_info info;
> -
> /* If the current architecture is not ARM, we have nothing to do. */
> if (gdbarch_bfd_arch_info (target_gdbarch ())->arch != bfd_arch_arm)
> return;
>
> /* Update the architecture. */
> - gdbarch_info_init (&info);
> -
> + gdbarch_info info;
> if (!gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__, _("could not update architecture"));
> }
> @@ -13237,7 +13234,6 @@ static void
> arm_record_test (void)
> {
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
> info.bfd_arch_info = bfd_scan_arch ("arm");
>
> struct gdbarch *gdbarch = gdbarch_find_by_info (info);
> @@ -13329,7 +13325,6 @@ arm_analyze_prologue_test ()
> for (bfd_endian endianness : {BFD_ENDIAN_LITTLE, BFD_ENDIAN_BIG})
> {
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
> info.byte_order = endianness;
> info.byte_order_for_code = endianness;
> info.bfd_arch_info = bfd_scan_arch ("arm");
> diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c
> index e1c141865cc6..01ef1604bffa 100644
> --- a/gdb/cris-tdep.c
> +++ b/gdb/cris-tdep.c
> @@ -3879,7 +3879,6 @@ set_cris_version (const char *ignore_args, int from_tty,
> usr_cmd_cris_version_valid = 1;
>
> /* Update the current architecture, if needed. */
> - gdbarch_info_init (&info);
> if (!gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__,
> _("cris_gdbarch_update: failed to update architecture."));
> @@ -3892,7 +3891,6 @@ set_cris_mode (const char *ignore_args, int from_tty,
> struct gdbarch_info info;
>
> /* Update the current architecture, if needed. */
> - gdbarch_info_init (&info);
> if (!gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__,
> "cris_gdbarch_update: failed to update architecture.");
> @@ -3905,7 +3903,6 @@ set_cris_dwarf2_cfi (const char *ignore_args, int from_tty,
> struct gdbarch_info info;
>
> /* Update the current architecture, if needed. */
> - gdbarch_info_init (&info);
> if (!gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__,
> _("cris_gdbarch_update: failed to update architecture."));
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 36ea4de17c8c..f94695adaa08 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1781,18 +1781,20 @@ struct gdbarch_list
>
> struct gdbarch_info
> {
> - /* Use default: NULL (ZERO). */
> - const struct bfd_arch_info *bfd_arch_info;
> + gdbarch_info ()
> + /* Ensure the union is zero-initialized. Relies on the fact that there's
> + no member larget than TDESC_DATA. */
> + : tdesc_data ()
> + {}
>
> - /* Use default: BFD_ENDIAN_UNKNOWN (NB: is not ZERO). */
> - enum bfd_endian byte_order;
> + const struct bfd_arch_info *bfd_arch_info = nullptr;
>
> - enum bfd_endian byte_order_for_code;
> + enum bfd_endian byte_order = BFD_ENDIAN_UNKNOWN;
>
> - /* Use default: NULL (ZERO). */
> - bfd *abfd;
> + enum bfd_endian byte_order_for_code = BFD_ENDIAN_UNKNOWN;
> +
> + bfd *abfd = nullptr;
>
> - /* Use default: NULL (ZERO). */
> union
> {
> /* Architecture-specific target description data. Numerous targets
> @@ -1805,11 +1807,9 @@ struct gdbarch_info
> int *id;
> };
>
> - /* Use default: GDB_OSABI_UNINITIALIZED (-1). */
> - enum gdb_osabi osabi;
> + enum gdb_osabi osabi = GDB_OSABI_UNKNOWN;
>
> - /* Use default: NULL (ZERO). */
> - const struct target_desc *target_desc;
> + const struct target_desc *target_desc = nullptr;
> };
>
> typedef struct gdbarch *(gdbarch_init_ftype) (struct gdbarch_info info, struct gdbarch_list *arches);
> @@ -1883,8 +1883,8 @@ extern int gdbarch_update_p (struct gdbarch_info info);
>
> /* Helper function. Find an architecture matching info.
>
> - INFO should be initialized using gdbarch_info_init, relevant fields
> - set, and then finished using gdbarch_info_fill.
> + INFO should have relevant fields set, and then finished using
> + gdbarch_info_fill.
>
> Returns the corresponding architecture, or NULL if no matching
> architecture was found. */
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 01a5ac88219d..e82d711ff2c3 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1533,18 +1533,20 @@ struct gdbarch_list
>
> struct gdbarch_info
> {
> - /* Use default: NULL (ZERO). */
> - const struct bfd_arch_info *bfd_arch_info;
> + gdbarch_info ()
> + /* Ensure the union is zero-initialized. Relies on the fact that there's
> + no member larget than TDESC_DATA. */
Typo, larger?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] gdb: remove gdbarch_info_init
2021-06-23 20:01 ` Luis Machado
@ 2021-06-23 20:35 ` Simon Marchi
2021-06-23 21:40 ` Weimin Pan
0 siblings, 1 reply; 12+ messages in thread
From: Simon Marchi @ 2021-06-23 20:35 UTC (permalink / raw)
To: Luis Machado, gdb-patches
>> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
>> index 01a5ac88219d..e82d711ff2c3 100755
>> --- a/gdb/gdbarch.sh
>> +++ b/gdb/gdbarch.sh
>> @@ -1533,18 +1533,20 @@ struct gdbarch_list
>> struct gdbarch_info
>> {
>> - /* Use default: NULL (ZERO). */
>> - const struct bfd_arch_info *bfd_arch_info;
>> + gdbarch_info ()
>> + /* Ensure the union is zero-initialized. Relies on the fact that there's
>> + no member larget than TDESC_DATA. */
>
> Typo, larger?
Indeed! Will fix locally, thanks.
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] gdb: remove gdbarch_info_init
2021-06-23 20:35 ` Simon Marchi
@ 2021-06-23 21:40 ` Weimin Pan
2021-06-25 13:51 ` Simon Marchi
0 siblings, 1 reply; 12+ messages in thread
From: Weimin Pan @ 2021-06-23 21:40 UTC (permalink / raw)
To: Simon Marchi, Luis Machado, gdb-patches
On 6/23/2021 1:35 PM, Simon Marchi via Gdb-patches wrote:
>>> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
>>> index 01a5ac88219d..e82d711ff2c3 100755
>>> --- a/gdb/gdbarch.sh
>>> +++ b/gdb/gdbarch.sh
>>> @@ -1533,18 +1533,20 @@ struct gdbarch_list
>>> struct gdbarch_info
>>> {
>>> - /* Use default: NULL (ZERO). */
>>> - const struct bfd_arch_info *bfd_arch_info;
>>> + gdbarch_info ()
>>> + /* Ensure the union is zero-initialized. Relies on the fact that there's
>>> + no member larget than TDESC_DATA. */
>> Typo, larger?
> Indeed! Will fix locally, thanks.
The same typo is also in gdbarch.h.
>
> Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] gdb: remove gdbarch_info_init
2021-06-23 21:40 ` Weimin Pan
@ 2021-06-25 13:51 ` Simon Marchi
0 siblings, 0 replies; 12+ messages in thread
From: Simon Marchi @ 2021-06-25 13:51 UTC (permalink / raw)
To: Weimin Pan, Luis Machado, gdb-patches
On 2021-06-23 5:40 p.m., Weimin Pan wrote:
>
> On 6/23/2021 1:35 PM, Simon Marchi via Gdb-patches wrote:
>>>> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
>>>> index 01a5ac88219d..e82d711ff2c3 100755
>>>> --- a/gdb/gdbarch.sh
>>>> +++ b/gdb/gdbarch.sh
>>>> @@ -1533,18 +1533,20 @@ struct gdbarch_list
>>>> struct gdbarch_info
>>>> {
>>>> - /* Use default: NULL (ZERO). */
>>>> - const struct bfd_arch_info *bfd_arch_info;
>>>> + gdbarch_info ()
>>>> + /* Ensure the union is zero-initialized. Relies on the fact that there's
>>>> + no member larget than TDESC_DATA. */
>>> Typo, larger?
>> Indeed! Will fix locally, thanks.
>
> The same typo is also in gdbarch.h.
This is generated from gdbarch.sh, so is fixed when re-running gdbarch.h (which I must not forget to run!). Thanks for noticing.
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] gdb: remove gdbarch_info::tdep_info
2021-06-23 19:27 [PATCH 1/2] gdb: remove gdbarch_info::tdep_info Simon Marchi
2021-06-23 19:27 ` [PATCH 2/2] gdb: remove gdbarch_info_init Simon Marchi
@ 2021-06-25 20:24 ` Andrew Burgess
2021-06-25 20:37 ` Simon Marchi
1 sibling, 1 reply; 12+ messages in thread
From: Andrew Burgess @ 2021-06-25 20:24 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-06-23 15:27:16 -0400]:
> This field is not actually used, remove it.
>
> gdb/ChangeLog:
>
> * gdbarch.sh (struct gdbarch_info) <tdep_info>: Remove.
> (gdbarch_find_by_info): Remove print.
Shouldn't this ChangeLog have 'gdbarch.{ch}: Regenerate.' too?
Otherwise, LGTM.
thanks,
Andrew
>
> Change-Id: I00af4681b8e1a27727441cbadc3827f5914bd8eb
> ---
> gdb/gdbarch.c | 3 ---
> gdb/gdbarch.h | 4 ----
> gdb/gdbarch.sh | 7 -------
> 3 files changed, 14 deletions(-)
>
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 208cf4b5aaab..830a86df89f3 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -5695,9 +5695,6 @@ gdbarch_find_by_info (struct gdbarch_info info)
> fprintf_unfiltered (gdb_stdlog,
> "gdbarch_find_by_info: info.abfd %s\n",
> host_address_to_string (info.abfd));
> - fprintf_unfiltered (gdb_stdlog,
> - "gdbarch_find_by_info: info.tdep_info %s\n",
> - host_address_to_string (info.tdep_info));
> }
>
> /* Find the tdep code that knows about this architecture. */
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 7157e5596fd3..36ea4de17c8c 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1795,10 +1795,6 @@ struct gdbarch_info
> /* Use default: NULL (ZERO). */
> union
> {
> - /* Architecture-specific information. The generic form for targets
> - that have extra requirements. */
> - struct gdbarch_tdep_info *tdep_info;
> -
> /* Architecture-specific target description data. Numerous targets
> need only this, so give them an easy way to hold it. */
> struct tdesc_arch_data *tdesc_data;
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 43e51341f97b..01a5ac88219d 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1547,10 +1547,6 @@ struct gdbarch_info
> /* Use default: NULL (ZERO). */
> union
> {
> - /* Architecture-specific information. The generic form for targets
> - that have extra requirements. */
> - struct gdbarch_tdep_info *tdep_info;
> -
> /* Architecture-specific target description data. Numerous targets
> need only this, so give them an easy way to hold it. */
> struct tdesc_arch_data *tdesc_data;
> @@ -2479,9 +2475,6 @@ gdbarch_find_by_info (struct gdbarch_info info)
> fprintf_unfiltered (gdb_stdlog,
> "gdbarch_find_by_info: info.abfd %s\n",
> host_address_to_string (info.abfd));
> - fprintf_unfiltered (gdb_stdlog,
> - "gdbarch_find_by_info: info.tdep_info %s\n",
> - host_address_to_string (info.tdep_info));
> }
>
> /* Find the tdep code that knows about this architecture. */
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] gdb: remove gdbarch_info::tdep_info
2021-06-25 20:24 ` [PATCH 1/2] gdb: remove gdbarch_info::tdep_info Andrew Burgess
@ 2021-06-25 20:37 ` Simon Marchi
0 siblings, 0 replies; 12+ messages in thread
From: Simon Marchi @ 2021-06-25 20:37 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
On 2021-06-25 4:24 p.m., Andrew Burgess wrote:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-06-23 15:27:16 -0400]:
>
>> This field is not actually used, remove it.
>>
>> gdb/ChangeLog:
>>
>> * gdbarch.sh (struct gdbarch_info) <tdep_info>: Remove.
>> (gdbarch_find_by_info): Remove print.
>
> Shouldn't this ChangeLog have 'gdbarch.{ch}: Regenerate.' too?
>
> Otherwise, LGTM.
>
> thanks,
> Andrew
Oh, yes, will add. Thanks!
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] gdb: remove gdbarch_info_init
2021-06-23 19:27 ` [PATCH 2/2] gdb: remove gdbarch_info_init Simon Marchi
2021-06-23 20:01 ` Luis Machado
@ 2021-06-25 20:55 ` Andrew Burgess
2021-06-28 2:11 ` Simon Marchi
1 sibling, 1 reply; 12+ messages in thread
From: Andrew Burgess @ 2021-06-25 20:55 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-06-23 15:27:17 -0400]:
> While reviewing another patch, I realized that gdbarch_info_init could
> easily be removed in favor of initializing gdbarch_info fields directly
> in the struct declaration. The only odd part is the union. I don't
> know if it's actually important for it to be zero-initialized, but I
> presume it is. I added a constructor to gdbarch_info to take care of
> that. A proper solution would be to use std::variant. Or, these could
> also be separate fields, the little extra space required wouldn't
> matter.
>
> gdb/ChangeLog:
>
> * gdbarch.sh (struct gdbarch_info): Initialize fields, add
> constructor.
> * gdbarch.h: Re-generate.
> * arch-utils.h (gdbarch_info_init): Remove, delete all usages.
I'm still seeing a use of gdbarch_info_init in corelow.c
(core_target::core_target) that prevents GDB compiling.
I was not previously aware of the 'id' field within gdbarch_info, and
I was wondering what it was for. As far as I can tell it's only used
for aarch64 with it being set in:
aarch64_linux_nat_target::thread_architecture
and used in:
aarch64_gdbarch_init
like this:
uint64_t vq = 0;
if (info.id == (int *) -1)
vq = 0;
else if (info.id != 0)
vq = (uint64_t) info.id;
else
vq = aarch64_get_tdesc_vq (info.target_desc);
Now, maybe I'm missing some cleverness here, but isn't the final use
of 'info.target_desc' meaningless? It shares space with the id field
that we're checking?
Given that only ::thread_architecture seems to set the id before
calling gdbarch_find_by_info, I assume that if we, for example,
connected to a remote target, the first pass through this code would
end up treating the target_desc point like id?
I guess, I'm suggesting that this aarch64 code looks broken to me, and
I wonder if the fix is to split the target_desc and id out from the
union?
Thanks,
Andrew
> * arch-utils.c (gdbarch_info_init): Remove.
>
> Change-Id: I7502e08fe0f278d84eef1667a072e8a97bda5ab5
> ---
> gdb/aarch64-linux-nat.c | 1 -
> gdb/aarch64-tdep.c | 2 --
> gdb/arch-utils.c | 26 +-------------------------
> gdb/arch-utils.h | 6 ------
> gdb/arm-tdep.c | 7 +------
> gdb/cris-tdep.c | 3 ---
> gdb/gdbarch.h | 28 ++++++++++++++--------------
> gdb/gdbarch.sh | 28 ++++++++++++++--------------
> gdb/i386-darwin-nat.c | 3 +--
> gdb/inferior.c | 3 +--
> gdb/mips-tdep.c | 5 -----
> gdb/osabi.c | 4 +---
> gdb/rs6000-nat.c | 3 +--
> gdb/rs6000-tdep.c | 4 +---
> gdb/selftest-arch.c | 1 -
> gdb/target-descriptions.c | 2 --
> 16 files changed, 35 insertions(+), 91 deletions(-)
>
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 61224022f6aa..c7cbebbc3517 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -1060,7 +1060,6 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
> new one), stashing the vector length inside id. Use -1 for when SVE
> unavailable, to distinguish from an unset value of 0. */
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
> info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);
> info.id = (int *) (vq == 0 ? -1 : vq);
> return gdbarch_find_by_info (info);
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 202b02108744..dea16e729c03 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -627,7 +627,6 @@ aarch64_analyze_prologue_test (void)
> {
> struct gdbarch_info info;
>
> - gdbarch_info_init (&info);
> info.bfd_arch_info = bfd_scan_arch ("aarch64");
>
> struct gdbarch *gdbarch = gdbarch_find_by_info (info);
> @@ -4559,7 +4558,6 @@ aarch64_process_record_test (void)
> struct gdbarch_info info;
> uint32_t ret;
>
> - gdbarch_info_init (&info);
> info.bfd_arch_info = bfd_scan_arch ("aarch64");
>
> struct gdbarch *gdbarch = gdbarch_find_by_info (info);
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index c75a79757ea2..4290d637ce16 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -389,8 +389,6 @@ set_endian (const char *ignore_args, int from_tty, struct cmd_list_element *c)
> {
> struct gdbarch_info info;
>
> - gdbarch_info_init (&info);
> -
> if (set_endian_string == endian_auto)
> {
> target_byte_order_user = BFD_ENDIAN_UNKNOWN;
> @@ -548,8 +546,6 @@ set_architecture (const char *ignore_args,
> {
> struct gdbarch_info info;
>
> - gdbarch_info_init (&info);
> -
> if (strcmp (set_architecture_string, "auto") == 0)
> {
> target_architecture_user = NULL;
> @@ -630,7 +626,6 @@ struct gdbarch *
> gdbarch_from_bfd (bfd *abfd)
> {
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
>
> info.abfd = abfd;
> return gdbarch_find_by_info (info);
> @@ -645,7 +640,6 @@ set_gdbarch_from_file (bfd *abfd)
> struct gdbarch_info info;
> struct gdbarch *gdbarch;
>
> - gdbarch_info_init (&info);
> info.abfd = abfd;
> info.target_desc = target_current_description ();
> gdbarch = gdbarch_find_by_info (info);
> @@ -679,10 +673,6 @@ void
> initialize_current_architecture (void)
> {
> const char **arches = gdbarch_printable_names ();
> - struct gdbarch_info info;
> -
> - /* determine a default architecture and byte order. */
> - gdbarch_info_init (&info);
>
> /* Find a default architecture. */
> if (default_bfd_arch == NULL)
> @@ -705,6 +695,7 @@ initialize_current_architecture (void)
> _("initialize_current_architecture: Arch not found"));
> }
>
> + gdbarch_info info;
> info.bfd_arch_info = default_bfd_arch;
>
> /* Take several guesses at a byte order. */
> @@ -769,21 +760,6 @@ initialize_current_architecture (void)
> }
> }
>
> -
> -/* Initialize a gdbarch info to values that will be automatically
> - overridden. Note: Originally, this ``struct info'' was initialized
> - using memset(0). Unfortunately, that ran into problems, namely
> - BFD_ENDIAN_BIG is zero. An explicit initialization function that
> - can explicitly set each field to a well defined value is used. */
> -
> -void
> -gdbarch_info_init (struct gdbarch_info *info)
> -{
> - memset (info, 0, sizeof (struct gdbarch_info));
> - info->byte_order = BFD_ENDIAN_UNKNOWN;
> - info->byte_order_for_code = info->byte_order;
> -}
> -
> /* Similar to init, but this time fill in the blanks. Information is
> obtained from the global "set ..." options and explicitly
> initialized INFO fields. */
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index a5b40ad8ff1b..03e9082f6d77 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -196,12 +196,6 @@ extern enum bfd_endian selected_byte_order (void);
> was explicitly selected. */
> extern const char *selected_architecture_name (void);
>
> -/* Initialize a ``struct info''. Can't use memset(0) since some
> - default values are not zero. "fill" takes all available
> - information and fills in any unspecified fields. */
> -
> -extern void gdbarch_info_init (struct gdbarch_info *info);
> -
> /* Similar to init, but this time fill in the blanks. Information is
> obtained from the global "set ..." options and explicitly
> initialized INFO fields. */
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 857a52a9a513..339e03271e7e 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -8403,15 +8403,12 @@ arm_skip_stub (struct frame_info *frame, CORE_ADDR pc)
> static void
> arm_update_current_architecture (void)
> {
> - struct gdbarch_info info;
> -
> /* If the current architecture is not ARM, we have nothing to do. */
> if (gdbarch_bfd_arch_info (target_gdbarch ())->arch != bfd_arch_arm)
> return;
>
> /* Update the architecture. */
> - gdbarch_info_init (&info);
> -
> + gdbarch_info info;
> if (!gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__, _("could not update architecture"));
> }
> @@ -13237,7 +13234,6 @@ static void
> arm_record_test (void)
> {
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
> info.bfd_arch_info = bfd_scan_arch ("arm");
>
> struct gdbarch *gdbarch = gdbarch_find_by_info (info);
> @@ -13329,7 +13325,6 @@ arm_analyze_prologue_test ()
> for (bfd_endian endianness : {BFD_ENDIAN_LITTLE, BFD_ENDIAN_BIG})
> {
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
> info.byte_order = endianness;
> info.byte_order_for_code = endianness;
> info.bfd_arch_info = bfd_scan_arch ("arm");
> diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c
> index e1c141865cc6..01ef1604bffa 100644
> --- a/gdb/cris-tdep.c
> +++ b/gdb/cris-tdep.c
> @@ -3879,7 +3879,6 @@ set_cris_version (const char *ignore_args, int from_tty,
> usr_cmd_cris_version_valid = 1;
>
> /* Update the current architecture, if needed. */
> - gdbarch_info_init (&info);
> if (!gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__,
> _("cris_gdbarch_update: failed to update architecture."));
> @@ -3892,7 +3891,6 @@ set_cris_mode (const char *ignore_args, int from_tty,
> struct gdbarch_info info;
>
> /* Update the current architecture, if needed. */
> - gdbarch_info_init (&info);
> if (!gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__,
> "cris_gdbarch_update: failed to update architecture.");
> @@ -3905,7 +3903,6 @@ set_cris_dwarf2_cfi (const char *ignore_args, int from_tty,
> struct gdbarch_info info;
>
> /* Update the current architecture, if needed. */
> - gdbarch_info_init (&info);
> if (!gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__,
> _("cris_gdbarch_update: failed to update architecture."));
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 36ea4de17c8c..f94695adaa08 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -1781,18 +1781,20 @@ struct gdbarch_list
>
> struct gdbarch_info
> {
> - /* Use default: NULL (ZERO). */
> - const struct bfd_arch_info *bfd_arch_info;
> + gdbarch_info ()
> + /* Ensure the union is zero-initialized. Relies on the fact that there's
> + no member larget than TDESC_DATA. */
> + : tdesc_data ()
> + {}
>
> - /* Use default: BFD_ENDIAN_UNKNOWN (NB: is not ZERO). */
> - enum bfd_endian byte_order;
> + const struct bfd_arch_info *bfd_arch_info = nullptr;
>
> - enum bfd_endian byte_order_for_code;
> + enum bfd_endian byte_order = BFD_ENDIAN_UNKNOWN;
>
> - /* Use default: NULL (ZERO). */
> - bfd *abfd;
> + enum bfd_endian byte_order_for_code = BFD_ENDIAN_UNKNOWN;
> +
> + bfd *abfd = nullptr;
>
> - /* Use default: NULL (ZERO). */
> union
> {
> /* Architecture-specific target description data. Numerous targets
> @@ -1805,11 +1807,9 @@ struct gdbarch_info
> int *id;
> };
>
> - /* Use default: GDB_OSABI_UNINITIALIZED (-1). */
> - enum gdb_osabi osabi;
> + enum gdb_osabi osabi = GDB_OSABI_UNKNOWN;
>
> - /* Use default: NULL (ZERO). */
> - const struct target_desc *target_desc;
> + const struct target_desc *target_desc = nullptr;
> };
>
> typedef struct gdbarch *(gdbarch_init_ftype) (struct gdbarch_info info, struct gdbarch_list *arches);
> @@ -1883,8 +1883,8 @@ extern int gdbarch_update_p (struct gdbarch_info info);
>
> /* Helper function. Find an architecture matching info.
>
> - INFO should be initialized using gdbarch_info_init, relevant fields
> - set, and then finished using gdbarch_info_fill.
> + INFO should have relevant fields set, and then finished using
> + gdbarch_info_fill.
>
> Returns the corresponding architecture, or NULL if no matching
> architecture was found. */
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 01a5ac88219d..e82d711ff2c3 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -1533,18 +1533,20 @@ struct gdbarch_list
>
> struct gdbarch_info
> {
> - /* Use default: NULL (ZERO). */
> - const struct bfd_arch_info *bfd_arch_info;
> + gdbarch_info ()
> + /* Ensure the union is zero-initialized. Relies on the fact that there's
> + no member larget than TDESC_DATA. */
> + : tdesc_data ()
> + {}
> +
> + const struct bfd_arch_info *bfd_arch_info = nullptr;
>
> - /* Use default: BFD_ENDIAN_UNKNOWN (NB: is not ZERO). */
> - enum bfd_endian byte_order;
> + enum bfd_endian byte_order = BFD_ENDIAN_UNKNOWN;
>
> - enum bfd_endian byte_order_for_code;
> + enum bfd_endian byte_order_for_code = BFD_ENDIAN_UNKNOWN;
>
> - /* Use default: NULL (ZERO). */
> - bfd *abfd;
> + bfd *abfd = nullptr;
>
> - /* Use default: NULL (ZERO). */
> union
> {
> /* Architecture-specific target description data. Numerous targets
> @@ -1557,11 +1559,9 @@ struct gdbarch_info
> int *id;
> };
>
> - /* Use default: GDB_OSABI_UNINITIALIZED (-1). */
> - enum gdb_osabi osabi;
> + enum gdb_osabi osabi = GDB_OSABI_UNKNOWN;
>
> - /* Use default: NULL (ZERO). */
> - const struct target_desc *target_desc;
> + const struct target_desc *target_desc = nullptr;
> };
>
> typedef struct gdbarch *(gdbarch_init_ftype) (struct gdbarch_info info, struct gdbarch_list *arches);
> @@ -1637,8 +1637,8 @@ extern int gdbarch_update_p (struct gdbarch_info info);
>
> /* Helper function. Find an architecture matching info.
>
> - INFO should be initialized using gdbarch_info_init, relevant fields
> - set, and then finished using gdbarch_info_fill.
> + INFO should have relevant fields set, and then finished using
> + gdbarch_info_fill.
>
> Returns the corresponding architecture, or NULL if no matching
> architecture was found. */
> diff --git a/gdb/i386-darwin-nat.c b/gdb/i386-darwin-nat.c
> index 922d7a6df0ca..c1d6139490d8 100644
> --- a/gdb/i386-darwin-nat.c
> +++ b/gdb/i386-darwin-nat.c
> @@ -479,7 +479,6 @@ darwin_check_osabi (darwin_inferior *inf, thread_t thread)
> {
> /* Attaching to a process. Let's figure out what kind it is. */
> x86_thread_state_t gp_regs;
> - struct gdbarch_info info;
> unsigned int gp_count = x86_THREAD_STATE_COUNT;
> kern_return_t ret;
>
> @@ -491,7 +490,7 @@ darwin_check_osabi (darwin_inferior *inf, thread_t thread)
> return;
> }
>
> - gdbarch_info_init (&info);
> + gdbarch_info info;
> gdbarch_info_fill (&info);
> info.byte_order = gdbarch_byte_order (target_gdbarch ());
> info.osabi = GDB_OSABI_DARWIN;
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index 059839ec9626..e1c70d553fde 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -747,7 +747,6 @@ add_inferior_with_spaces (void)
> struct address_space *aspace;
> struct program_space *pspace;
> struct inferior *inf;
> - struct gdbarch_info info;
>
> /* If all inferiors share an address space on this system, this
> doesn't really return a new address space; otherwise, it
> @@ -760,7 +759,7 @@ add_inferior_with_spaces (void)
>
> /* Setup the inferior's initial arch, based on information obtained
> from the global "set ..." options. */
> - gdbarch_info_init (&info);
> + gdbarch_info info;
> inf->gdbarch = gdbarch_find_by_info (info);
> /* The "set ..." options reject invalid settings, so we should
> always have a valid arch by now. */
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index dbace1212508..2aee4f10b316 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -865,7 +865,6 @@ set_mips64_transfers_32bit_regs (const char *args, int from_tty,
> struct cmd_list_element *c)
> {
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
> /* FIXME: cagney/2003-11-15: Should be setting a field in "info"
> instead of relying on globals. Doing that would let generic code
> handle the search for this specific architecture. */
> @@ -6918,7 +6917,6 @@ static void
> set_mipsfpu_single_command (const char *args, int from_tty)
> {
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
> mips_fpu_type = MIPS_FPU_SINGLE;
> mips_fpu_type_auto = 0;
> /* FIXME: cagney/2003-11-15: Should be setting a field in "info"
> @@ -6932,7 +6930,6 @@ static void
> set_mipsfpu_double_command (const char *args, int from_tty)
> {
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
> mips_fpu_type = MIPS_FPU_DOUBLE;
> mips_fpu_type_auto = 0;
> /* FIXME: cagney/2003-11-15: Should be setting a field in "info"
> @@ -6946,7 +6943,6 @@ static void
> set_mipsfpu_none_command (const char *args, int from_tty)
> {
> struct gdbarch_info info;
> - gdbarch_info_init (&info);
> mips_fpu_type = MIPS_FPU_NONE;
> mips_fpu_type_auto = 0;
> /* FIXME: cagney/2003-11-15: Should be setting a field in "info"
> @@ -8790,7 +8786,6 @@ mips_abi_update (const char *ignore_args,
>
> /* Force the architecture to update, and (if it's a MIPS architecture)
> mips_gdbarch_init will take care of the rest. */
> - gdbarch_info_init (&info);
> gdbarch_update_p (info);
> }
>
> diff --git a/gdb/osabi.c b/gdb/osabi.c
> index b1603c09c855..aabf895c0451 100644
> --- a/gdb/osabi.c
> +++ b/gdb/osabi.c
> @@ -598,8 +598,6 @@ generic_elf_osabi_sniffer (bfd *abfd)
> static void
> set_osabi (const char *args, int from_tty, struct cmd_list_element *c)
> {
> - struct gdbarch_info info;
> -
> if (strcmp (set_osabi_string, "auto") == 0)
> user_osabi_state = osabi_auto;
> else if (strcmp (set_osabi_string, "default") == 0)
> @@ -630,7 +628,7 @@ set_osabi (const char *args, int from_tty, struct cmd_list_element *c)
>
> /* NOTE: At some point (true multiple architectures) we'll need to be more
> graceful here. */
> - gdbarch_info_init (&info);
> + gdbarch_info info;
> if (! gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__, _("Updating OS ABI failed."));
> }
> diff --git a/gdb/rs6000-nat.c b/gdb/rs6000-nat.c
> index 50c96e281c64..ee00a600343a 100644
> --- a/gdb/rs6000-nat.c
> +++ b/gdb/rs6000-nat.c
> @@ -558,7 +558,6 @@ rs6000_nat_target::create_inferior (const char *exec_file,
> enum bfd_architecture arch;
> unsigned long mach;
> bfd abfd;
> - struct gdbarch_info info;
>
> inf_ptrace_target::create_inferior (exec_file, allargs, env, from_tty);
>
> @@ -593,7 +592,7 @@ rs6000_nat_target::create_inferior (const char *exec_file,
>
> bfd_default_set_arch_mach (&abfd, arch, mach);
>
> - gdbarch_info_init (&info);
> + gdbarch_info info;
> info.bfd_arch_info = bfd_get_arch_info (&abfd);
> info.abfd = current_program_space->exec_bfd ();
>
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 5bcf5fca2c3b..a629450d746f 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -7260,7 +7260,6 @@ powerpc_set_soft_float (const char *args, int from_tty,
> struct gdbarch_info info;
>
> /* Update the architecture. */
> - gdbarch_info_init (&info);
> if (!gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__, _("could not update architecture"));
> }
> @@ -7269,7 +7268,6 @@ static void
> powerpc_set_vector_abi (const char *args, int from_tty,
> struct cmd_list_element *c)
> {
> - struct gdbarch_info info;
> int vector_abi;
>
> for (vector_abi = POWERPC_VEC_AUTO;
> @@ -7287,7 +7285,7 @@ powerpc_set_vector_abi (const char *args, int from_tty,
> powerpc_vector_abi_string);
>
> /* Update the architecture. */
> - gdbarch_info_init (&info);
> + gdbarch_info info;
> if (!gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__, _("could not update architecture"));
> }
> diff --git a/gdb/selftest-arch.c b/gdb/selftest-arch.c
> index ab1215c848c3..0eef134d4841 100644
> --- a/gdb/selftest-arch.c
> +++ b/gdb/selftest-arch.c
> @@ -64,7 +64,6 @@ struct gdbarch_selftest : public selftest
> {
> struct gdbarch_info info;
>
> - gdbarch_info_init (&info);
> info.bfd_arch_info = bfd_scan_arch (arches[i]);
>
> struct gdbarch *gdbarch = gdbarch_find_by_info (info);
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index f7d0a688c62f..f94ace756d4d 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -555,7 +555,6 @@ target_find_description (void)
> {
> struct gdbarch_info info;
>
> - gdbarch_info_init (&info);
> info.target_desc = tdesc_info->tdesc;
> if (!gdbarch_update_p (info))
> warning (_("Architecture rejected target-supplied description"));
> @@ -592,7 +591,6 @@ target_clear_description (void)
> tdesc_info->tdesc = nullptr;
>
> gdbarch_info info;
> - gdbarch_info_init (&info);
> if (!gdbarch_update_p (info))
> internal_error (__FILE__, __LINE__,
> _("Could not remove target-supplied description"));
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] gdb: remove gdbarch_info_init
2021-06-25 20:55 ` Andrew Burgess
@ 2021-06-28 2:11 ` Simon Marchi
2021-06-28 9:16 ` Andrew Burgess
0 siblings, 1 reply; 12+ messages in thread
From: Simon Marchi @ 2021-06-28 2:11 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
On 2021-06-25 4:55 p.m., Andrew Burgess wrote:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-06-23 15:27:17 -0400]:
>
>> While reviewing another patch, I realized that gdbarch_info_init could
>> easily be removed in favor of initializing gdbarch_info fields directly
>> in the struct declaration. The only odd part is the union. I don't
>> know if it's actually important for it to be zero-initialized, but I
>> presume it is. I added a constructor to gdbarch_info to take care of
>> that. A proper solution would be to use std::variant. Or, these could
>> also be separate fields, the little extra space required wouldn't
>> matter.
>>
>> gdb/ChangeLog:
>>
>> * gdbarch.sh (struct gdbarch_info): Initialize fields, add
>> constructor.
>> * gdbarch.h: Re-generate.
>> * arch-utils.h (gdbarch_info_init): Remove, delete all usages.
>
> I'm still seeing a use of gdbarch_info_init in corelow.c
> (core_target::core_target) that prevents GDB compiling.
Yes, that's the call I saw in review that prompted me to write this
patch. Luis merged his patch, so after rebasing I need to get rid of
that call too, thanks.
> I was not previously aware of the 'id' field within gdbarch_info, and
> I was wondering what it was for. As far as I can tell it's only used
> for aarch64 with it being set in:
It was originally used for SPU, as the comment implies, but AArch64
started using it because... it's an int field, and AArch64 needed an int
field to pass the vq value.
>
> aarch64_linux_nat_target::thread_architecture
>
> and used in:
>
> aarch64_gdbarch_init
>
> like this:
>
> uint64_t vq = 0;
> if (info.id == (int *) -1)
> vq = 0;
> else if (info.id != 0)
> vq = (uint64_t) info.id;
> else
> vq = aarch64_get_tdesc_vq (info.target_desc);
>
> Now, maybe I'm missing some cleverness here, but isn't the final use
> of 'info.target_desc' meaningless? It shares space with the id field
> that we're checking?
>
> Given that only ::thread_architecture seems to set the id before
> calling gdbarch_find_by_info, I assume that if we, for example,
> connected to a remote target, the first pass through this code would
> end up treating the target_desc point like id?
>
> I guess, I'm suggesting that this aarch64 code looks broken to me, and
> I wonder if the fix is to split the target_desc and id out from the
> union?
I think you are confusing tdesc_data and target_desc. The id field
shares its space with:
struct tdesc_arch_data *tdesc_data;
not
const struct target_desc *target_desc = nullptr;
I can't explain really explain why this is designed this way, with a
union, but it looks like "id" and "tdesc_data" are not used
simultaneously. "id" is used between
aarch64_linux_nat_target::thread_architecture and aarch64_gdbarch_init
(or it is zero, the default value). "tdesc_data" is then used between
aarch64_gdbarch_init and gdbarch_osabi_init and the osabi init handler.
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] gdb: remove gdbarch_info_init
2021-06-28 2:11 ` Simon Marchi
@ 2021-06-28 9:16 ` Andrew Burgess
2021-06-28 15:44 ` Simon Marchi
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Burgess @ 2021-06-28 9:16 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
* Simon Marchi <simon.marchi@polymtl.ca> [2021-06-27 22:11:24 -0400]:
>
>
> On 2021-06-25 4:55 p.m., Andrew Burgess wrote:
> > * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-06-23 15:27:17 -0400]:
> >
> >> While reviewing another patch, I realized that gdbarch_info_init could
> >> easily be removed in favor of initializing gdbarch_info fields directly
> >> in the struct declaration. The only odd part is the union. I don't
> >> know if it's actually important for it to be zero-initialized, but I
> >> presume it is. I added a constructor to gdbarch_info to take care of
> >> that. A proper solution would be to use std::variant. Or, these could
> >> also be separate fields, the little extra space required wouldn't
> >> matter.
> >>
> >> gdb/ChangeLog:
> >>
> >> * gdbarch.sh (struct gdbarch_info): Initialize fields, add
> >> constructor.
> >> * gdbarch.h: Re-generate.
> >> * arch-utils.h (gdbarch_info_init): Remove, delete all usages.
> >
> > I'm still seeing a use of gdbarch_info_init in corelow.c
> > (core_target::core_target) that prevents GDB compiling.
>
> Yes, that's the call I saw in review that prompted me to write this
> patch. Luis merged his patch, so after rebasing I need to get rid of
> that call too, thanks.
>
> > I was not previously aware of the 'id' field within gdbarch_info, and
> > I was wondering what it was for. As far as I can tell it's only used
> > for aarch64 with it being set in:
>
> It was originally used for SPU, as the comment implies, but AArch64
> started using it because... it's an int field, and AArch64 needed an int
> field to pass the vq value.
>
> >
> > aarch64_linux_nat_target::thread_architecture
> >
> > and used in:
> >
> > aarch64_gdbarch_init
> >
> > like this:
> >
> > uint64_t vq = 0;
> > if (info.id == (int *) -1)
> > vq = 0;
> > else if (info.id != 0)
> > vq = (uint64_t) info.id;
> > else
> > vq = aarch64_get_tdesc_vq (info.target_desc);
> >
> > Now, maybe I'm missing some cleverness here, but isn't the final use
> > of 'info.target_desc' meaningless? It shares space with the id field
> > that we're checking?
> >
> > Given that only ::thread_architecture seems to set the id before
> > calling gdbarch_find_by_info, I assume that if we, for example,
> > connected to a remote target, the first pass through this code would
> > end up treating the target_desc point like id?
> >
> > I guess, I'm suggesting that this aarch64 code looks broken to me, and
> > I wonder if the fix is to split the target_desc and id out from the
> > union?
>
> I think you are confusing tdesc_data and target_desc. The id field
> shares its space with:
>
> struct tdesc_arch_data *tdesc_data;
>
> not
>
> const struct target_desc *target_desc = nullptr;
>
> I can't explain really explain why this is designed this way, with a
> union, but it looks like "id" and "tdesc_data" are not used
> simultaneously. "id" is used between
> aarch64_linux_nat_target::thread_architecture and aarch64_gdbarch_init
> (or it is zero, the default value). "tdesc_data" is then used between
> aarch64_gdbarch_init and gdbarch_osabi_init and the osabi init handler.
Thanks for pointing that out. In that case the patch LGTM (with the
extra call removed once this is rebased).
Thanks,
Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] gdb: remove gdbarch_info_init
2021-06-28 9:16 ` Andrew Burgess
@ 2021-06-28 15:44 ` Simon Marchi
0 siblings, 0 replies; 12+ messages in thread
From: Simon Marchi @ 2021-06-28 15:44 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
On 2021-06-28 5:16 a.m., Andrew Burgess wrote:
> Thanks for pointing that out. In that case the patch LGTM (with the
> extra call removed once this is rebased).
>
> Thanks,
> Andrew
Thanks, will push shortly.
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-06-28 15:44 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-23 19:27 [PATCH 1/2] gdb: remove gdbarch_info::tdep_info Simon Marchi
2021-06-23 19:27 ` [PATCH 2/2] gdb: remove gdbarch_info_init Simon Marchi
2021-06-23 20:01 ` Luis Machado
2021-06-23 20:35 ` Simon Marchi
2021-06-23 21:40 ` Weimin Pan
2021-06-25 13:51 ` Simon Marchi
2021-06-25 20:55 ` Andrew Burgess
2021-06-28 2:11 ` Simon Marchi
2021-06-28 9:16 ` Andrew Burgess
2021-06-28 15:44 ` Simon Marchi
2021-06-25 20:24 ` [PATCH 1/2] gdb: remove gdbarch_info::tdep_info Andrew Burgess
2021-06-25 20:37 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).