Re: PATCH: Enable x86 XML target descriptions

Mark Kettenis
2010-02-28T20:30:52Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3887 bytes --]

> Date: Mon, 22 Feb 2010 06:17:29 -0800
> From: "H.J. Lu" <hjl.tools@gmail.com>
>
> >> +/* 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.

Even with that, checking the size of the .reg2 section makes no sense
to me.

> >> @@ -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");

Sorry, but I don't understand this. How does checking a register in
the target description care about the number of registers set in the
gdbarch we're building?

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

Which seems wrong to me. Both the core registers and the SSE
registers are different in 64-bit mode. But perhaps Daniel can shed
some light on how these features are supposed to be used?

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

Exactly. So why is it ok for 64bit-sse.xml to have

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

?