Re: PATCH: Enable x86 XML target descriptions

[Mark Kettenis <mark.kettenis@xs4all.nl>]

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

?