This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: PATCH: Enable x86 XML target descriptions
> Date: Thu, 18 Feb 2010 15:01:35 -0800
> From: "H.J. Lu" <hongjiu.lu@intel.com>
>
> On Thu, Feb 18, 2010 at 07:34:02AM -0800, H.J. Lu wrote:
> > On Wed, Feb 17, 2010 at 09:43:12PM -0800, H.J. Lu wrote:
> > > On Wed, Feb 10, 2010 at 12:03:03PM -0800, H.J. Lu wrote:
> > > > Hi,
> > > >
> > > > This patch enables x86 XML target descriptions. I used
> > > > i386_linux_init_orig_eax to support the old gdbserver which doesn't
> > > > have XML target descriptions.
> > > >
> > > > The register description processing is handled in i386_gdbarch_init
> > > > for 32bit/64bit as well as all ABIs to avoid code duplication and
> > > > unnecessary complexity.
> > > >
> > > > OK to install?
> > > >
> > >
> > > Here is the updated patch. I removed all BFD64 from i386 files. I
> > > renamed I386_NUM_FREGS to I387_NUM_REGS and put it in i387-tdep.h.
> > > I use it in amd64-tdep.c. I added a few fields to gdbarch_tdep so
> > > that I can pass values from xxx_abit_init to i386_gdbarch_init. OK
> > > to install?
> > >
> > > Thanks.
> > >
> > >
> >
> > A small update. I added I386_MXCSR_REGNUM so that "i387-tdep.h"
> > isn't added for amd64-linux-nat.c, amd64-linux-tdep.c and
> > i386-linux-tdep.c. OK to install?
> >
>
> A small bug fix. amd64_linux_read_description should return
> tdesc_i386_linux for 32bit.
I tested a GDB with this diff on OpenBSD/i386 and only noticed one
regression:
Running ./gdb.xml/tdesc-regs.exp ...
-PASS: gdb.xml/tdesc-regs.exp: set tdesc file single-reg.xml
+FAIL: gdb.xml/tdesc-regs.exp: set tdesc file single-reg.xml
To me it looks like this test needs to be adjusted now that the i386
target has tdesc support.
I still need a bit more time to review the diff. I'm sorry, but the
tdesc stuff is something I'm simply not familiar with, so it takes
some time to understand the implications of this diff. However, below
are some questions/issues I ran into already.
> diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
> index 5c9e558..8038555 100644
> --- a/gdb/amd64-linux-nat.c
> +++ b/gdb/amd64-linux-nat.c
> @@ -674,6 +674,17 @@ amd64_linux_siginfo_fixup (struct siginfo *native, gdb_byte *inf, int direction)
> return 0;
> }
>
> +/* Get Linux/x86 target description from running target. */
> +
> +static const struct target_desc *
> +amd64_linux_read_description (struct target_ops *ops)
> +{
> + if (gdbarch_ptr_bit (target_gdbarch) == 64)
> + return tdesc_amd64_linux;
> + else
> + return tdesc_i386_linux;
> +}
> +
This made me wonder what happens if you attach to a process without
loading an executable first. Currently this works, since GDB can
figure out what executable belongs to the the process and load the
executable automatically. But I fear a chicken & egg problem here:
the gdbarch is derviced from the tdesc, but in order to determine the
tdesc you need a gdbarch.
> @@ -1260,10 +1242,38 @@ amd64_linux_record_signal (struct gdbarch *gdbarch,
> return 0;
> }
>
> +/* Get Linux/x86 target description from core dump. */
> +
> +static const struct target_desc *
> +amd64_linux_core_read_description (struct gdbarch *gdbarch,
> + struct target_ops *target,
> + bfd *abfd)
> +{
> + asection *section = bfd_get_section_by_name (abfd, ".reg2");
> +
> + if (section == NULL)
> + return NULL;
> +
> + switch (bfd_section_size (abfd, section))
> + {
> + case 0x200:
> + /* Linux/x86-64. */
> + return tdesc_amd64_linux;
> + default:
> + return NULL;
> + }
> +}
This seems a bit odd to me. I'd expect this function to just return
tdesc_amd64_linux unconditionally.
> @@ -1282,10 +1292,31 @@ amd64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>
> /* Add the %orig_rax register used for syscall restarting. */
> set_gdbarch_write_pc (gdbarch, amd64_linux_write_pc);
> +
> + /* Reserve a number for orig_rax. */
> set_gdbarch_num_regs (gdbarch, AMD64_LINUX_NUM_REGS);
> - set_gdbarch_register_name (gdbarch, amd64_linux_register_name);
> - set_gdbarch_register_type (gdbarch, amd64_linux_register_type);
> - set_gdbarch_register_reggroup_p (gdbarch, amd64_linux_register_reggroup_p);
Why do you need to set num_regs here, but not the register_name,
register_type and register_reggroup_p members? All four are set by
tdesc_use_registers().
> +
> + if (! tdesc_has_registers (tdesc))
> + tdesc = tdesc_amd64_linux;
> + tdep->tdesc = tdesc;
> +
> + feature = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.linux");
Shouldn't that be org.gnu.gdb.amd64.linux?
> @@ -2112,87 +2107,22 @@ i386_return_value (struct gdbarch *gdbarch, struct type *func_type,
> }
>
>
> -/* Construct types for ISA-specific registers. */
> -struct type *
> -i386_eflags_type (struct gdbarch *gdbarch)
> -{
> - struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> -
> - if (!tdep->i386_eflags_type)
> - {
> - struct type *type;
> -
> - type = arch_flags_type (gdbarch, "builtin_type_i386_eflags", 4);
> - append_flags_type_flag (type, 0, "CF");
> - append_flags_type_flag (type, 1, NULL);
> - append_flags_type_flag (type, 2, "PF");
> - append_flags_type_flag (type, 4, "AF");
> - append_flags_type_flag (type, 6, "ZF");
> - append_flags_type_flag (type, 7, "SF");
> - append_flags_type_flag (type, 8, "TF");
> - append_flags_type_flag (type, 9, "IF");
> - append_flags_type_flag (type, 10, "DF");
> - append_flags_type_flag (type, 11, "OF");
> - append_flags_type_flag (type, 14, "NT");
> - append_flags_type_flag (type, 16, "RF");
> - append_flags_type_flag (type, 17, "VM");
> - append_flags_type_flag (type, 18, "AC");
> - append_flags_type_flag (type, 19, "VIF");
> - append_flags_type_flag (type, 20, "VIP");
> - append_flags_type_flag (type, 21, "ID");
> -
> - tdep->i386_eflags_type = type;
> - }
> -
> - return tdep->i386_eflags_type;
> -}
> -
> -struct type *
> -i386_mxcsr_type (struct gdbarch *gdbarch)
> -{
> - struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> -
> - if (!tdep->i386_mxcsr_type)
> - {
> - struct type *type;
> -
> - type = arch_flags_type (gdbarch, "builtin_type_i386_mxcsr", 4);
> - append_flags_type_flag (type, 0, "IE");
> - append_flags_type_flag (type, 1, "DE");
> - append_flags_type_flag (type, 2, "ZE");
> - append_flags_type_flag (type, 3, "OE");
> - append_flags_type_flag (type, 4, "UE");
> - append_flags_type_flag (type, 5, "PE");
> - append_flags_type_flag (type, 6, "DAZ");
> - append_flags_type_flag (type, 7, "IM");
> - append_flags_type_flag (type, 8, "DM");
> - append_flags_type_flag (type, 9, "ZM");
> - append_flags_type_flag (type, 10, "OM");
> - append_flags_type_flag (type, 11, "UM");
> - append_flags_type_flag (type, 12, "PM");
> - append_flags_type_flag (type, 15, "FZ");
> -
> - tdep->i386_mxcsr_type = type;
> - }
> -
> - return tdep->i386_mxcsr_type;
> -}
> -
> struct type *
> i387_ext_type (struct gdbarch *gdbarch)
> {
> struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>
> if (!tdep->i387_ext_type)
> - tdep->i387_ext_type
> - = arch_float_type (gdbarch, -1, "builtin_type_i387_ext",
> - floatformats_i387_ext);
> + {
> + tdep->i387_ext_type = tdesc_find_type (gdbarch, "i387_ext");
> + gdb_assert (tdep->i387_ext_type != NULL);
> + }
>
> return tdep->i387_ext_type;
> }
>
> /* Construct vector type for MMX registers. */
> -struct type *
> +static struct type *
> i386_mmx_type (struct gdbarch *gdbarch)
> {
> struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> @@ -2233,84 +2163,14 @@ i386_mmx_type (struct gdbarch *gdbarch)
> return tdep->i386_mmx_type;
> }
>
> -struct type *
> -i386_sse_type (struct gdbarch *gdbarch)
> -{
> - struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> -
> - if (!tdep->i386_sse_type)
> - {
> - const struct builtin_type *bt = builtin_type (gdbarch);
> -
> - /* The type we're building is this: */
> -#if 0
> - union __gdb_builtin_type_vec128i
> - {
> - int128_t uint128;
> - int64_t v2_int64[2];
> - int32_t v4_int32[4];
> - int16_t v8_int16[8];
> - int8_t v16_int8[16];
> - double v2_double[2];
> - float v4_float[4];
> - };
> -#endif
> -
> - struct type *t;
> -
> - t = arch_composite_type (gdbarch,
> - "__gdb_builtin_type_vec128i", TYPE_CODE_UNION);
> - append_composite_type_field (t, "v4_float",
> - init_vector_type (bt->builtin_float, 4));
> - append_composite_type_field (t, "v2_double",
> - init_vector_type (bt->builtin_double, 2));
> - append_composite_type_field (t, "v16_int8",
> - init_vector_type (bt->builtin_int8, 16));
> - append_composite_type_field (t, "v8_int16",
> - init_vector_type (bt->builtin_int16, 8));
> - append_composite_type_field (t, "v4_int32",
> - init_vector_type (bt->builtin_int32, 4));
> - append_composite_type_field (t, "v2_int64",
> - init_vector_type (bt->builtin_int64, 2));
> - append_composite_type_field (t, "uint128", bt->builtin_int128);
> -
> - TYPE_VECTOR (t) = 1;
> - TYPE_NAME (t) = "builtin_type_vec128i";
> - tdep->i386_sse_type = t;
> - }
> -
> - return tdep->i386_sse_type;
> -}
I didn't realize these functions would effectively be moved into
target-descriptions.c. That feels wrong.
> + /* Target description may be changed. */
> + tdesc = tdep->tdesc;
> +
> + if (! tdesc_has_registers (tdesc))
> + {
> + xfree (tdep);
> + gdbarch_free (gdbarch);
> + return NULL;
> + }
> +
> + /* Get core registers. */
> + feature_core = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.core");
> +
> + /* Get SSE registers. */
> + feature_vector = tdesc_find_feature (tdesc, "org.gnu.gdb.i386.sse");
> +
> + if (feature_core == NULL || feature_vector == NULL)
> + {
> + xfree (tdep);
> + gdbarch_free (gdbarch);
> + return NULL;
> + }
> +
> + valid_p = 1;
> +
> + num_regs = tdep->num_core_regs;
> + for (i = 0; i < num_regs; i++)
> + valid_p &= tdesc_numbered_register (feature_core, tdesc_data, i,
> + tdep->register_names[i]);
> +
> + /* Need to include %mxcsr, so add one. */
> + num_regs += tdep->num_xmm_regs + 1;
> + for (; i < num_regs; i++)
> + valid_p &= tdesc_numbered_register (feature_vector, tdesc_data, i,
> + tdep->register_names[i]);
> +
> + if (!valid_p)
> + {
> + tdesc_data_cleanup (tdesc_data);
> + xfree (tdep);
> + gdbarch_free (gdbarch);
> + return NULL;
> + }
That's quite a bit of code to verify the sanity of the target description. I think that should go into a seperate function.
> +
> + tdesc_use_registers (gdbarch, tdesc, tdesc_data);
> +
> + /* Override gdbarch_register_reggroup_p set in tdesc_use_registers. */
> + set_gdbarch_register_reggroup_p (gdbarch, tdep->register_reggroup_p);
Why is it necessary to do this?