From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19441 invoked by alias); 22 Feb 2010 13:42:41 -0000 Received: (qmail 19406 invoked by uid 22791); 22 Feb 2010 13:42:39 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 22 Feb 2010 13:42:34 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3) with ESMTP id o1MDgTw1010150; Mon, 22 Feb 2010 14:42:29 +0100 (CET) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id o1MDgSZA029705; Mon, 22 Feb 2010 14:42:28 +0100 (CET) Date: Mon, 22 Feb 2010 13:42:00 -0000 Message-Id: <201002221342.o1MDgSZA029705@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: hjl.tools@gmail.com CC: hjl.tools@gmail.com, gdb-patches@sourceware.org In-reply-to: <20100218230135.GA17916@intel.com> (hongjiu.lu@intel.com) Subject: Re: PATCH: Enable x86 XML target descriptions References: <20100210200303.GA19632@lucon.org> <20100218054312.GA9022@lucon.org> <20100218153402.GA27929@lucon.org> <20100218230135.GA17916@intel.com> 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/msg00526.txt.bz2 > 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. 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? > > > > > > > > > > Here is the updated patch. I removed all BFD64 from i386 files. I > > > renamed I386_NUM_FREGS to I387_NUM_REGS and put it in i387-tdep.h. > > > I use it in amd64-tdep.c. I added a few fields to gdbarch_tdep so > > > that I can pass values from xxx_abit_init to i386_gdbarch_init. OK > > > 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. amd64_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: Running ./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 still need a bit more time to review the diff. I'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. However, 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) > return 0; > } > > +/* Get Linux/x86 target description from running target. */ > + > +static const struct target_desc * > +amd64_linux_read_description (struct target_ops *ops) > +{ > + if (gdbarch_ptr_bit (target_gdbarch) == 64) > + return tdesc_amd64_linux; > + else > + return tdesc_i386_linux; > +} > + This made me wonder what happens if you attach to a process without loading an executable first. Currently this works, since GDB can figure out what executable belongs to the the process and load the executable automatically. But 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. > @@ -1260,10 +1242,38 @@ amd64_linux_record_signal (struct gdbarch *gdbarch, > return 0; > } > > +/* 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. > @@ -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(). > + > + 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? > @@ -2112,87 +2107,22 @@ i386_return_value (struct gdbarch *gdbarch, struct type *func_type, > } > > > -/* Construct types for ISA-specific registers. */ > -struct type * > -i386_eflags_type (struct gdbarch *gdbarch) > -{ > - struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > - > - if (!tdep->i386_eflags_type) > - { > - struct type *type; > - > - type = arch_flags_type (gdbarch, "builtin_type_i386_eflags", 4); > - append_flags_type_flag (type, 0, "CF"); > - append_flags_type_flag (type, 1, NULL); > - append_flags_type_flag (type, 2, "PF"); > - append_flags_type_flag (type, 4, "AF"); > - append_flags_type_flag (type, 6, "ZF"); > - append_flags_type_flag (type, 7, "SF"); > - append_flags_type_flag (type, 8, "TF"); > - append_flags_type_flag (type, 9, "IF"); > - append_flags_type_flag (type, 10, "DF"); > - append_flags_type_flag (type, 11, "OF"); > - append_flags_type_flag (type, 14, "NT"); > - append_flags_type_flag (type, 16, "RF"); > - append_flags_type_flag (type, 17, "VM"); > - append_flags_type_flag (type, 18, "AC"); > - append_flags_type_flag (type, 19, "VIF"); > - append_flags_type_flag (type, 20, "VIP"); > - append_flags_type_flag (type, 21, "ID"); > - > - tdep->i386_eflags_type = type; > - } > - > - return tdep->i386_eflags_type; > -} > - > -struct type * > -i386_mxcsr_type (struct gdbarch *gdbarch) > -{ > - struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > - > - if (!tdep->i386_mxcsr_type) > - { > - struct type *type; > - > - type = arch_flags_type (gdbarch, "builtin_type_i386_mxcsr", 4); > - append_flags_type_flag (type, 0, "IE"); > - append_flags_type_flag (type, 1, "DE"); > - append_flags_type_flag (type, 2, "ZE"); > - append_flags_type_flag (type, 3, "OE"); > - append_flags_type_flag (type, 4, "UE"); > - append_flags_type_flag (type, 5, "PE"); > - append_flags_type_flag (type, 6, "DAZ"); > - append_flags_type_flag (type, 7, "IM"); > - append_flags_type_flag (type, 8, "DM"); > - append_flags_type_flag (type, 9, "ZM"); > - append_flags_type_flag (type, 10, "OM"); > - append_flags_type_flag (type, 11, "UM"); > - append_flags_type_flag (type, 12, "PM"); > - append_flags_type_flag (type, 15, "FZ"); > - > - tdep->i386_mxcsr_type = type; > - } > - > - return tdep->i386_mxcsr_type; > -} > - > struct type * > i387_ext_type (struct gdbarch *gdbarch) > { > struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > > if (!tdep->i387_ext_type) > - tdep->i387_ext_type > - = arch_float_type (gdbarch, -1, "builtin_type_i387_ext", > - floatformats_i387_ext); > + { > + tdep->i387_ext_type = tdesc_find_type (gdbarch, "i387_ext"); > + gdb_assert (tdep->i387_ext_type != NULL); > + } > > return tdep->i387_ext_type; > } > > /* Construct vector type for MMX registers. */ > -struct type * > +static struct type * > i386_mmx_type (struct gdbarch *gdbarch) > { > struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > @@ -2233,84 +2163,14 @@ i386_mmx_type (struct gdbarch *gdbarch) > return tdep->i386_mmx_type; > } > > -struct type * > -i386_sse_type (struct gdbarch *gdbarch) > -{ > - struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > - > - if (!tdep->i386_sse_type) > - { > - const struct builtin_type *bt = builtin_type (gdbarch); > - > - /* The type we're building is this: */ > -#if 0 > - union __gdb_builtin_type_vec128i > - { > - int128_t uint128; > - int64_t v2_int64[2]; > - int32_t v4_int32[4]; > - int16_t v8_int16[8]; > - int8_t v16_int8[16]; > - double v2_double[2]; > - float v4_float[4]; > - }; > -#endif > - > - struct type *t; > - > - t = arch_composite_type (gdbarch, > - "__gdb_builtin_type_vec128i", TYPE_CODE_UNION); > - append_composite_type_field (t, "v4_float", > - init_vector_type (bt->builtin_float, 4)); > - append_composite_type_field (t, "v2_double", > - init_vector_type (bt->builtin_double, 2)); > - append_composite_type_field (t, "v16_int8", > - init_vector_type (bt->builtin_int8, 16)); > - append_composite_type_field (t, "v8_int16", > - init_vector_type (bt->builtin_int16, 8)); > - append_composite_type_field (t, "v4_int32", > - init_vector_type (bt->builtin_int32, 4)); > - append_composite_type_field (t, "v2_int64", > - init_vector_type (bt->builtin_int64, 2)); > - append_composite_type_field (t, "uint128", bt->builtin_int128); > - > - TYPE_VECTOR (t) = 1; > - TYPE_NAME (t) = "builtin_type_vec128i"; > - tdep->i386_sse_type = t; > - } > - > - return tdep->i386_sse_type; > -} I didn't realize these functions would effectively be moved into target-descriptions.c. That feels wrong. > + /* Target description may be changed. */ > + tdesc = tdep->tdesc; > + > + if (! tdesc_has_registers (tdesc)) > + { > + xfree (tdep); > + gdbarch_free (gdbarch); > + return NULL; > + } > + > + /* 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"); > + > + if (feature_core == NULL || feature_vector == NULL) > + { > + xfree (tdep); > + gdbarch_free (gdbarch); > + return NULL; > + } > + > + valid_p = 1; > + > + num_regs = tdep->num_core_regs; > + for (i = 0; i < num_regs; i++) > + valid_p &= tdesc_numbered_register (feature_core, tdesc_data, i, > + tdep->register_names[i]); > + > + /* Need to include %mxcsr, so add one. */ > + num_regs += tdep->num_xmm_regs + 1; > + for (; i < num_regs; i++) > + valid_p &= tdesc_numbered_register (feature_vector, tdesc_data, i, > + tdep->register_names[i]); > + > + if (!valid_p) > + { > + tdesc_data_cleanup (tdesc_data); > + xfree (tdep); > + gdbarch_free (gdbarch); > + return NULL; > + } That's quite a bit of code to verify the sanity of the target description. I think that should go into a seperate function. > + > + tdesc_use_registers (gdbarch, tdesc, tdesc_data); > + > + /* Override gdbarch_register_reggroup_p set in tdesc_use_registers. */ > + set_gdbarch_register_reggroup_p (gdbarch, tdep->register_reggroup_p); Why is it necessary to do this?