Re: PATCH: Enable x86 XML target descriptions

["H.J. Lu" <hjl.tools@gmail.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.