From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1468 invoked by alias); 22 Feb 2010 14:17:39 -0000 Received: (qmail 1436 invoked by uid 22791); 22 Feb 2010 14:17:37 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,SARE_MSGID_LONG40 X-Spam-Check-By: sourceware.org Received: from mail-fx0-f216.google.com (HELO mail-fx0-f216.google.com) (209.85.220.216) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 22 Feb 2010 14:17:33 +0000 Received: by fxm8 with SMTP id 8so98587fxm.8 for ; Mon, 22 Feb 2010 06:17:29 -0800 (PST) MIME-Version: 1.0 Received: by 10.216.89.200 with SMTP id c50mr988422wef.164.1266848249685; Mon, 22 Feb 2010 06:17:29 -0800 (PST) In-Reply-To: <201002221342.o1MDgSZA029705@glazunov.sibelius.xs4all.nl> References: <20100210200303.GA19632@lucon.org> <20100218054312.GA9022@lucon.org> <20100218153402.GA27929@lucon.org> <20100218230135.GA17916@intel.com> <201002221342.o1MDgSZA029705@glazunov.sibelius.xs4all.nl> Date: Mon, 22 Feb 2010 14:17:00 -0000 Message-ID: <6dc9ffc81002220617o1d348e68hc918d434118cadcb@mail.gmail.com> Subject: Re: PATCH: Enable x86 XML target descriptions From: "H.J. Lu" To: Mark Kettenis Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-02/txt/msg00527.txt.bz2 On Mon, Feb 22, 2010 at 5:42 AM, Mark Kettenis wr= ote: >> Date: Thu, 18 Feb 2010 15:01:35 -0800 >> From: "H.J. Lu" >> >> 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. =A0I 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. =A0I >> > > renamed I386_NUM_FREGS to I387_NUM_REGS and put it in i387-tdep.h. >> > > I use it in amd64-tdep.c. =A0I added a few fields to gdbarch_tdep so >> > > that I can pass values from xxx_abit_init to i386_gdbarch_init. =A0OK >> > > 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. =A0amd64_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: > > =A0Running ./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. =A0I'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. =A0However, 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) >> =A0 =A0 =A0return 0; >> =A0} >> >> +/* Get Linux/x86 target description from running target. =A0*/ >> + >> +static const struct target_desc * >> +amd64_linux_read_description (struct target_ops *ops) >> +{ >> + =A0if (gdbarch_ptr_bit (target_gdbarch) =3D=3D 64) >> + =A0 =A0return tdesc_amd64_linux; >> + =A0else >> + =A0 =A0return tdesc_i386_linux; >> +} >> + > > This made me wonder what happens if you attach to a process without > loading an executable first. =A0Currently this works, since GDB can > figure out what executable belongs to the the process and load the > executable automatically. =A0But 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 *gdbar= ch, >> =A0 =A0return 0; >> =A0} >> >> +/* Get Linux/x86 target description from core dump. =A0*/ >> + >> +static const struct target_desc * >> +amd64_linux_core_read_description (struct gdbarch *gdbarch, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct tar= get_ops *target, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bfd *abfd) >> +{ >> + =A0asection *section =3D bfd_get_section_by_name (abfd, ".reg2"); >> + >> + =A0if (section =3D=3D NULL) >> + =A0 =A0return NULL; >> + >> + =A0switch (bfd_section_size (abfd, section)) >> + =A0 =A0{ >> + =A0 =A0case 0x200: >> + =A0 =A0 =A0/* Linux/x86-64. =A0*/ >> + =A0 =A0 =A0return tdesc_amd64_linux; >> + =A0 =A0default: >> + =A0 =A0 =A0return NULL; >> + =A0 =A0} >> +} > > This seems a bit odd to me. =A0I'd expect this function to just return > tdesc_amd64_linux unconditionally. The folowup patch for AVX will change it to xcr0 =3D i386_linux_core_read_xcr0 (gdbarch, target, abfd); switch (bfd_section_size (abfd, section)) { case 0x200: /* Linux/x86-64. */ if ((xcr0 & XSTATE_AVX_MASK) =3D=3D 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) >> >> =A0 =A0/* Add the %orig_rax register used for syscall restarting. =A0*/ >> =A0 =A0set_gdbarch_write_pc (gdbarch, amd64_linux_write_pc); >> + >> + =A0/* Reserve a number for orig_rax. =A0*/ >> =A0 =A0set_gdbarch_num_regs (gdbarch, AMD64_LINUX_NUM_REGS); >> - =A0set_gdbarch_register_name (gdbarch, amd64_linux_register_name); >> - =A0set_gdbarch_register_type (gdbarch, amd64_linux_register_type); >> - =A0set_gdbarch_register_reggroup_p (gdbarch, amd64_linux_register_regg= roup_p); > > Why do you need to set num_regs here, but not the register_name, > register_type and register_reggroup_p members? =A0All 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 =3D tdesc_numbered_register (feature, tdesc_data, AMD64_LINUX_ORIG_RAX_REGNUM, "orig_rax"); >> + >> + =A0if (! tdesc_has_registers (tdesc)) >> + =A0 =A0tdesc =3D tdesc_amd64_linux; >> + =A0tdep->tdesc =3D tdesc; >> + >> + =A0feature =3D 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 and so that i386_gdbarch_init can have /* Get core registers. */ feature_core =3D tdesc_find_feature (tdesc, "org.gnu.gdb.i386.core"); /* Get SSE registers. */ feature_vector =3D tdesc_find_feature (tdesc, "org.gnu.gdb.i386.sse"); after /* Hook in ABI-specific overrides, if they have been registered. */ info.tdep_info =3D (void *) tdesc_data; gdbarch_init_osabi (info, gdbarch); It will be odd for 64bit-linux.xml to have >> @@ -2112,87 +2107,22 @@ i386_return_value (struct gdbarch *gdbarch, stru= ct type *func_type, >> =A0} >> >> >> -/* Construct types for ISA-specific registers. =A0*/ >> -struct type * >> -i386_eflags_type (struct gdbarch *gdbarch) >> -{ >> - =A0struct gdbarch_tdep *tdep =3D gdbarch_tdep (gdbarch); >> - >> - =A0if (!tdep->i386_eflags_type) >> - =A0 =A0{ >> - =A0 =A0 =A0struct type *type; >> - >> - =A0 =A0 =A0type =3D arch_flags_type (gdbarch, "builtin_type_i386_eflag= s", 4); >> - =A0 =A0 =A0append_flags_type_flag (type, 0, "CF"); >> - =A0 =A0 =A0append_flags_type_flag (type, 1, NULL); >> - =A0 =A0 =A0append_flags_type_flag (type, 2, "PF"); >> - =A0 =A0 =A0append_flags_type_flag (type, 4, "AF"); >> - =A0 =A0 =A0append_flags_type_flag (type, 6, "ZF"); >> - =A0 =A0 =A0append_flags_type_flag (type, 7, "SF"); >> - =A0 =A0 =A0append_flags_type_flag (type, 8, "TF"); >> - =A0 =A0 =A0append_flags_type_flag (type, 9, "IF"); >> - =A0 =A0 =A0append_flags_type_flag (type, 10, "DF"); >> - =A0 =A0 =A0append_flags_type_flag (type, 11, "OF"); >> - =A0 =A0 =A0append_flags_type_flag (type, 14, "NT"); >> - =A0 =A0 =A0append_flags_type_flag (type, 16, "RF"); >> - =A0 =A0 =A0append_flags_type_flag (type, 17, "VM"); >> - =A0 =A0 =A0append_flags_type_flag (type, 18, "AC"); >> - =A0 =A0 =A0append_flags_type_flag (type, 19, "VIF"); >> - =A0 =A0 =A0append_flags_type_flag (type, 20, "VIP"); >> - =A0 =A0 =A0append_flags_type_flag (type, 21, "ID"); >> - >> - =A0 =A0 =A0tdep->i386_eflags_type =3D type; >> - =A0 =A0} >> - >> - =A0return tdep->i386_eflags_type; >> -} >> - >> -struct type * >> -i386_mxcsr_type (struct gdbarch *gdbarch) >> -{ >> - =A0struct gdbarch_tdep *tdep =3D gdbarch_tdep (gdbarch); >> - >> - =A0if (!tdep->i386_mxcsr_type) >> - =A0 =A0{ >> - =A0 =A0 =A0struct type *type; >> - >> - =A0 =A0 =A0type =3D arch_flags_type (gdbarch, "builtin_type_i386_mxcsr= ", 4); >> - =A0 =A0 =A0append_flags_type_flag (type, 0, "IE"); >> - =A0 =A0 =A0append_flags_type_flag (type, 1, "DE"); >> - =A0 =A0 =A0append_flags_type_flag (type, 2, "ZE"); >> - =A0 =A0 =A0append_flags_type_flag (type, 3, "OE"); >> - =A0 =A0 =A0append_flags_type_flag (type, 4, "UE"); >> - =A0 =A0 =A0append_flags_type_flag (type, 5, "PE"); >> - =A0 =A0 =A0append_flags_type_flag (type, 6, "DAZ"); >> - =A0 =A0 =A0append_flags_type_flag (type, 7, "IM"); >> - =A0 =A0 =A0append_flags_type_flag (type, 8, "DM"); >> - =A0 =A0 =A0append_flags_type_flag (type, 9, "ZM"); >> - =A0 =A0 =A0append_flags_type_flag (type, 10, "OM"); >> - =A0 =A0 =A0append_flags_type_flag (type, 11, "UM"); >> - =A0 =A0 =A0append_flags_type_flag (type, 12, "PM"); >> - =A0 =A0 =A0append_flags_type_flag (type, 15, "FZ"); >> - >> - =A0 =A0 =A0tdep->i386_mxcsr_type =3D type; >> - =A0 =A0} >> - >> - =A0return tdep->i386_mxcsr_type; >> -} >> - >> =A0struct type * >> =A0i387_ext_type (struct gdbarch *gdbarch) >> =A0{ >> =A0 =A0struct gdbarch_tdep *tdep =3D gdbarch_tdep (gdbarch); >> >> =A0 =A0if (!tdep->i387_ext_type) >> - =A0 =A0tdep->i387_ext_type >> - =A0 =A0 =A0=3D arch_float_type (gdbarch, -1, "builtin_type_i387_ext", >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0floatformats_i387_ext); >> + =A0 =A0{ >> + =A0 =A0 =A0tdep->i387_ext_type =3D tdesc_find_type (gdbarch, "i387_ext= "); >> + =A0 =A0 =A0gdb_assert (tdep->i387_ext_type !=3D NULL); >> + =A0 =A0} >> >> =A0 =A0return tdep->i387_ext_type; >> =A0} >> >> =A0/* Construct vector type for MMX registers. =A0*/ >> -struct type * >> +static struct type * >> =A0i386_mmx_type (struct gdbarch *gdbarch) >> =A0{ >> =A0 =A0struct gdbarch_tdep *tdep =3D gdbarch_tdep (gdbarch); >> @@ -2233,84 +2163,14 @@ i386_mmx_type (struct gdbarch *gdbarch) >> =A0 =A0return tdep->i386_mmx_type; >> =A0} >> >> -struct type * >> -i386_sse_type (struct gdbarch *gdbarch) >> -{ >> - =A0struct gdbarch_tdep *tdep =3D gdbarch_tdep (gdbarch); >> - >> - =A0if (!tdep->i386_sse_type) >> - =A0 =A0{ >> - =A0 =A0 =A0const struct builtin_type *bt =3D builtin_type (gdbarch); >> - >> - =A0 =A0 =A0/* The type we're building is this: */ >> -#if 0 >> - =A0 =A0 =A0union __gdb_builtin_type_vec128i >> - =A0 =A0 =A0{ >> - =A0 =A0 =A0 =A0int128_t uint128; >> - =A0 =A0 =A0 =A0int64_t v2_int64[2]; >> - =A0 =A0 =A0 =A0int32_t v4_int32[4]; >> - =A0 =A0 =A0 =A0int16_t v8_int16[8]; >> - =A0 =A0 =A0 =A0int8_t v16_int8[16]; >> - =A0 =A0 =A0 =A0double v2_double[2]; >> - =A0 =A0 =A0 =A0float v4_float[4]; >> - =A0 =A0 =A0}; >> -#endif >> - >> - =A0 =A0 =A0struct type *t; >> - >> - =A0 =A0 =A0t =3D arch_composite_type (gdbarch, >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"__gdb_builtin_= type_vec128i", TYPE_CODE_UNION); >> - =A0 =A0 =A0append_composite_type_field (t, "v4_float", >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0init_ve= ctor_type (bt->builtin_float, 4)); >> - =A0 =A0 =A0append_composite_type_field (t, "v2_double", >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0init_ve= ctor_type (bt->builtin_double, 2)); >> - =A0 =A0 =A0append_composite_type_field (t, "v16_int8", >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0init_ve= ctor_type (bt->builtin_int8, 16)); >> - =A0 =A0 =A0append_composite_type_field (t, "v8_int16", >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0init_ve= ctor_type (bt->builtin_int16, 8)); >> - =A0 =A0 =A0append_composite_type_field (t, "v4_int32", >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0init_ve= ctor_type (bt->builtin_int32, 4)); >> - =A0 =A0 =A0append_composite_type_field (t, "v2_int64", >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0init_ve= ctor_type (bt->builtin_int64, 2)); >> - =A0 =A0 =A0append_composite_type_field (t, "uint128", bt->builtin_int1= 28); >> - >> - =A0 =A0 =A0TYPE_VECTOR (t) =3D 1; >> - =A0 =A0 =A0TYPE_NAME (t) =3D "builtin_type_vec128i"; >> - =A0 =A0 =A0tdep->i386_sse_type =3D t; >> - =A0 =A0} >> - >> - =A0return tdep->i386_sse_type; >> -} > > I didn't realize these functions would effectively be moved into > target-descriptions.c. =A0That feels wrong. That is how target description works. All types used in XML are supported in target-descriptions.c. >> + =A0/* Target description may be changed. =A0*/ >> + =A0tdesc =3D tdep->tdesc; >> + >> + =A0if (! tdesc_has_registers (tdesc)) >> + =A0 =A0{ >> + =A0 =A0 =A0xfree (tdep); >> + =A0 =A0 =A0gdbarch_free (gdbarch); >> + =A0 =A0 =A0return NULL; >> + =A0 =A0} >> + >> + =A0/* Get core registers. =A0*/ >> + =A0feature_core =3D tdesc_find_feature (tdesc, "org.gnu.gdb.i386.core"= ); >> + >> + =A0/* Get SSE registers. =A0*/ >> + =A0feature_vector =3D tdesc_find_feature (tdesc, "org.gnu.gdb.i386.sse= "); >> + >> + =A0if (feature_core =3D=3D NULL || feature_vector =3D=3D NULL) >> + =A0 =A0{ >> + =A0 =A0 =A0xfree (tdep); >> + =A0 =A0 =A0gdbarch_free (gdbarch); >> + =A0 =A0 =A0return NULL; >> + =A0 =A0} >> + >> + =A0valid_p =3D 1; >> + >> + =A0num_regs =3D tdep->num_core_regs; >> + =A0for (i =3D 0; i < num_regs; i++) >> + =A0 =A0valid_p &=3D tdesc_numbered_register (feature_core, tdesc_data,= i, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 tdep->register_names[i]); >> + >> + =A0/* Need to include %mxcsr, so add one. =A0*/ >> + =A0num_regs +=3D tdep->num_xmm_regs + 1; >> + =A0for (; i < num_regs; i++) >> + =A0 =A0valid_p &=3D tdesc_numbered_register (feature_vector, tdesc_dat= a, i, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 tdep->register_names[i]); >> + >> + =A0if (!valid_p) >> + =A0 =A0{ >> + =A0 =A0 =A0tdesc_data_cleanup (tdesc_data); >> + =A0 =A0 =A0xfree (tdep); >> + =A0 =A0 =A0gdbarch_free (gdbarch); >> + =A0 =A0 =A0return NULL; >> + =A0 =A0} > > That's quite a bit of code to verify the sanity of the target description= . =A0I think that should go into a seperate function. I will do that. >> + >> + =A0tdesc_use_registers (gdbarch, tdesc, tdesc_data); >> + >> + =A0/* Override gdbarch_register_reggroup_p set in tdesc_use_registers.= =A0*/ >> + =A0set_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. --=20 H.J.