Re: PATCH: Enable x86 XML target descriptions

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

On Wed, Feb 17, 2010 at 7:22 AM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> Date: Wed, 17 Feb 2010 06:58:20 -0800
>> From: "H.J. Lu" <hongjiu.lu@intel.com>
>>
>> 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?
>> >
>> > BTW, I have a followup patch to support al, ax and eax pseudo registers.
>> >
>> > Thanks.
>> >
>> >
>> > H.J.
>> > ----
>> > gdb/
>> >
>> > 2010-02-10  H.J. Lu  <hongjiu.lu@intel.com>
>> >
>> >     * amd64-linux-nat.c (_initialize_amd64_linux_nat): Set
>> >     to_read_description to i386_linux_read_description.
>> >     * i386-linux-nat.c (_initialize_i386_linux_nat): Likewise.
>> >
>> >     * amd64-linux-tdep.c (amd64_linux_register_name): Removed.
>> >     (amd64_linux_register_type): Likewise.
>> >     (amd64_linux_init_abi): Don't call set_gdbarch_num_regs,
>> >     set_gdbarch_register_name nor set_gdbarch_register_type.  Call
>> >     i386_linux_init_orig_eax and set_gdbarch_core_read_description.
>> >
>> >     * amd64-linux-tdep.h: Include "i386-linux-tdep.h".
>> >
>> >     * amd64-tdep.c (amd64_register_names): Make it global.
>> >     (amd64_register_name): Removed.
>> >     (amd64_register_type): Likewise.
>> >     (amd64_init_abi): Don't call set_gdbarch_num_regs,
>> >     set_gdbarch_register_name nor set_gdbarch_register_type.
>> >
>> >     * amd64-tdep.h (amd64_register_names): New.
>> >
>> >     * i386-linux-tdep.c: Include "amd64-tdep.h" and
>> >     * "amd64-linux-tdep.h" instead "i386-tdep.h" and
>> >     "i386-linux-tdep.h".  Include features/i386/i386-linux.c and
>> >     features/i386/amd64-linux.c.
>> >     (i386_linux_register_name): Support BFD64.
>> >     (i386_linux_register_type): New.
>> >     (i386_linux_init_orig_eax): Likewise.
>> >     (i386_linux_core_read_description): Likewise.
>> >     (i386_linux_read_description): Likewise.
>> >     (i386_linux_init_abi): Don't call set_gdbarch_num_regs nor
>> >     set_gdbarch_register_name.  Call i386_linux_init_orig_eax and
>> >     set_gdbarch_core_read_description.
>> >     (_initialize_i386_linux_tdep): Call initialize_tdesc_i386_linux
>> >     and initialize_tdesc_x86_64_linux.
>> >
>> >     * i386-linux-tdep.h (i386_linux_core_read_description): New.
>> >     (i386_linux_read_description): Likewise.
>> >     (i386_linux_init_orig_eax): Likewise.
>> >
>> >     * i386-tdep.c: Include "amd64-tdep.h" instead of "i386-tdep.h".
>> >     Include features/i386/i386.c and features/i386/amd64.c.
>> >     (i386_register_names): Make it const.
>> >     (i386_mmx_names): Likewise.
>> >     (i386_num_register_names): Removed.
>> >     (i386_register_name): Likewise.
>> >     (i386_eflags_type): Likewise.
>> >     (i386_mxcsr_type): Likewise.
>> >     (i386_sse_type): Likewise.
>> >     (i386_register_type): Likewise.
>> >     (i387_ext_type): Call tdesc_find_type instead of arch_float_type.
>> >     (i386_pseudo_register_name): New.
>> >     (i386_pseudo_register_type): Likewise.
>> >     (i386_mmx_type): Make it static.
>> >     (i386_gdbarch_init): Support both 32bit and 64bit x86 target
>> >     descriptions.  Don't call set_gdbarch_register_name nor
>> >     set_gdbarch_register_type.  Call set_tdesc_pseudo_register_type,
>> >     set_tdesc_pseudo_register_name and tdesc_use_registers.
>> >     (_initialize_i386_tdep): Call initialize_tdesc_i386 and
>> >     initialize_tdesc_x86_64.
>> >
>> >     * i386-tdep.h (gdbarch_tdep): Remove i386_eflags_type,
>> >     i386_mxcsr_type and i386_sse_type.
>> >     (i386_eflags_type): Removed.
>> >     (i386_mxcsr_type): Likewise.
>> >     (i386_mmx_type): Likewise.
>> >     (i386_sse_type): Likewise.
>> >     (i386_register_name): Likewise.
>> >
>> >     * regformats/i386/i386-linux.dat: Generated.
>> >     * regformats/i386/i386.dat: Likewise.
>> >     * regformats/i386/amd64-linux.dat: Likewise.
>> >     * regformats/i386/amd64.dat: Likewise.
>> >
>> >     * regformats/reg-i386-linux.dat: Removed.
>> >     * regformats/reg-i386.dat: Likewise.
>> >     * regformats/reg-x86-64-linux.dat: Likewise.
>> >     * regformats/reg-x86-64.dat: Likewise.
>> >
>> > gdb/gdbserver/
>> >
>> > 2010-02-10  H.J. Lu  <hongjiu.lu@intel.com>
>> >
>> >     * Makefile.in (clean): Replace reg-i386.c, reg-x86-64.c,
>> >     reg-i386-linux.c and reg-x86-64-linux.c with i386.c, amd64.c,
>> >     i386-linux.c and amd64-linux.c.
>> >     (reg-i386.o): Removed.
>> >     (reg-i386.c): Likewise.
>> >     (reg-i386-linux.o): Likewise.
>> >     (reg-i386-linux.c): Likewise.
>> >     (reg-x86-64.o): Likewise.
>> >     (reg-x86-64.c): Likewise.
>> >     (reg-x86-64-linux.o): Likewise.
>> >     (reg-x86-64-linux.c): Likewise.
>> >     (i386.o): New.
>> >     (i386.c): Likewise.
>> >     (i386-linux.o): Likewise.
>> >     (i386-linux.c): Likewise.
>> >     (amd64.o): Likewise.
>> >     (amd64.c): Likewise.
>> >     (amd64-linux.o): Likewise.
>> >     (amd64-linux.c): Likewise.
>> >
>> >     * configure.srv (srv_i386_regobj): New.
>> >     (srv_i386_linux_regobj): Likewise.
>> >     (srv_amd64_regobj): Likewise.
>> >     (srv_amd64_linux_regobj): Likewise.
>> >     (srv_i386_32bit_xmlfiles): Likewise.
>> >     (srv_i386_64bit_xmlfiles): Likewise.
>> >     (srv_i386_xmlfiles): Likewise.
>> >     (srv_amd64_xmlfiles): Likewise.
>> >     (srv_i386_linux_xmlfiles): Likewise.
>> >     (srv_amd64_linux_xmlfiles): Likewise.
>> >     (i[34567]86-*-cygwin*): Set srv_regobj to $srv_i386_regobj.  Set
>> >     srv_xmlfiles to $srv_i386_xmlfiles.
>> >     (i[34567]86-*-mingw32ce*): Likewise.
>> >     (i[34567]86-*-mingw*): Likewise.
>> >     (i[34567]86-*-nto*): Likewise.
>> >     (i[34567]86-*-linux*): Set srv_regobj to $srv_i386_linux_regobj
>> >     and $srv_amd64_linux_regobj.  Set srv_xmlfiles to
>> >     $srv_i386_linux_xmlfiles and $srv_amd64_linux_xmlfiles.
>> >     (x86_64-*-linux*): Likewise.
>> >
>> >     * linux-x86-low.c (init_registers_x86_64_linux): Removed.
>> >     (init_registers_amd64_linux): New.
>> >     (x86_arch_setup): Replace init_registers_x86_64_linux with
>> >     init_registers_amd64_linux.
>> >
>>
>> Any objections to enable XML target descriptions for x86? Any comments?
>
> Thanks for the reminder.
>
> I really dislike the fact that perfectly well understandable C code
> gets replaced by a complicated XML-based mechanism that I don't
> completely understand.
>
> So it is perfectly possible that there is some magic that I'm missing
> here, but I get the feeling that your diff breaks all native amd64 and
> i386 targets except for Linux.  I'm afraid that isn't acceptable.

Can you try my patch on non-Linux x86 native targets?

> Also, you should split i386_linux_register_name(),
> i386_linix_register_type() i386_linux_init_orig_eax() into seperate
> i386 and amd64 versions, and get rid of all the #ifdef BFD64's in the
> *-tdep.c files.  There has been a clear split between 32-bit and
> 64-bit versions of the *-tdep.c code and this diff blurrs that.

I am not sure it is easy to do, especially for register description.
I have an impression that tdesc_use_registers should only be called
once per arch. i386 and amd64 belong to the same bfd_arch_i386.
That is how it is used in other places:

arm-tdep.c:      tdesc_use_registers (gdbarch, info.target_desc, tdesc_data);
m68k-tdep.c:    tdesc_use_registers (gdbarch, info.target_desc, tdesc_data);
mips-tdep.c:      tdesc_use_registers (gdbarch, info.target_desc, tdesc_data);
rs6000-tdep.c:  tdesc_use_registers (gdbarch, tdesc, tdesc_data);
s390-tdep.c:  tdesc_use_registers (gdbarch, tdesc, tdesc_data);

There is only single call to tdesc_use_registers for 32bit/64bit mips/ppc.
All other XML described tdep files support both 32bit/64bit:

[hjl@gnu-6 gdb]$ ls mips*-tdep.c rs6000*-tdep.c ppc*-tdep.c
mips64obsd-tdep.c  mipsnbsd-tdep.c   ppcnbsd-tdep.c   rs6000-aix-tdep.c
mips-irix-tdep.c   mips-tdep.c       ppcobsd-tdep.c   rs6000-tdep.c
mips-linux-tdep.c  ppc-linux-tdep.c  ppc-sysv-tdep.c

I can try to minimize BFD64. But I am afraid that I can't totally avoid it.
Is that acceptable?

> I can't comment on the gdbserver bits; that is Daniel's territory.
>

Daniel, can you comment on gdbserver changes?

Thanks.

-- 
H.J.