Re: PATCH: Enable x86 XML target descriptions

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

On Sun, Feb 28, 2010 at 12:30 PM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> 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.

It is sanity check for corrupted core dump. I will remove it since
you think it isn't needed.

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

I copied it from ppc_linux_init_abi. Gdb won't work if I don't reserve
a register number for orig_rax:

(gdb) r /bin/ls
Starting program:
/export/build/gnu/gdb-xstate/build-x86_64-linux/gdb/gdb /bin/ls
[Thread debugging using libthread_db enabled]
Detaching after fork from child process 7791.
GNU gdb (GDB) 7.1.50.20100228-cvs
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
/export/gnu/import/git/gdb-xstate/gdb/target-descriptions.c:1098:
internal-error: tdesc_use_registers: Assertion `VEC_length
(tdesc_arch_reg, data->arch_regs) <= num_regs' failed.

Maybe Daniel knows the answer.

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

The reason is i386_validate_tdesc_p is called by
i386_gdbarch_init after calling gdbarch_init_osabi,
which may set up a 64bit target description. If 64bit
target description has different feature name, it won't
find core and SSE registers.  If you don't like "i386"
in the feature name, I can change it to x86.

FWIW, BFD uses bfd_arch_i386 for both i386 and
amd64. I think that is where i386 comes from.


-- 
H.J.