Re: PATCH: Enable x86 XML target descriptions

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Mon, Feb 22, 2010 at 5:42 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> 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 will do that.

> 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.

How do you attach to a process without loading an executable first?
Gdb has to load something to debug. Do you have a testcase? I will
fix it if it doesn't work already.

>> @@ -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.

The folowup patch for AVX will change it to

  xcr0 = i386_linux_core_read_xcr0 (gdbarch, target, abfd);
  switch (bfd_section_size (abfd, section))
    {
    case 0x200:
      /* Linux/x86-64.  */
      if ((xcr0 & XSTATE_AVX_MASK) == XSTATE_AVX_MASK)
        return tdesc_amd64_avx_linux;
      else
        return tdesc_amd64_linux;
    default:
      return NULL;
    }

Other OSes will need a similar change to support AVX.

>> @@ -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().

tdesc_use_registers is called after amd64_linux_init_abi. At this
point, num_regs is incorrect for Linux. We need to update it for

  valid_p = tdesc_numbered_register (feature, tdesc_data,
                                     AMD64_LINUX_ORIG_RAX_REGNUM,
                                     "orig_rax");

>> +
>> + ?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?

64bit-core.xml and 64bit-sse.xml have

<feature name="org.gnu.gdb.i386.core">

and

<feature name="org.gnu.gdb.i386.sse">

so that i386_gdbarch_init can have

  /* 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");

after

  /* Hook in ABI-specific overrides, if they have been registered.  */
  info.tdep_info = (void *) tdesc_data;
  gdbarch_init_osabi (info, gdbarch);

It will be odd for 64bit-linux.xml to have

<feature name="org.gnu.gdb.i386.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.

That is how target description works. All types used in XML are
supported in target-descriptions.c.

>> + ?/* 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.

I will do that.

>> +
>> + ?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?
>

i386_gdbarch_init calls

1. gdbarch_init_osabi
2.  tdesc_use_registers, which calls

set_gdbarch_register_reggroup_p (gdbarch, tdesc_register_reggroup_p);

If you want to change register_reggroup_p in gdbarch_init_osabi, you
have to store it in tdep and call set_gdbarch_register_reggroup_p after
tdesc_use_registers.

Thanks.


-- 
H.J.