From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13781 invoked by alias); 20 May 2012 21:39:07 -0000 Received: (qmail 13773 invoked by uid 22791); 20 May 2012 21:39:05 -0000 X-SWARE-Spam-Status: No, hits=-2.9 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,T_RP_MATCHES_RCVD 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; Sun, 20 May 2012 21:38:52 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3) with ESMTP id q4KLcYbN028869; Sun, 20 May 2012 23:38:34 +0200 (CEST) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3/Submit) id q4KLcWBf011913; Sun, 20 May 2012 23:38:32 +0200 (CEST) Date: Sun, 20 May 2012 21:39:00 -0000 Message-Id: <201205202138.q4KLcWBf011913@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: hjl.tools@gmail.com CC: brobecker@adacore.com, gdb-patches@sourceware.org In-reply-to: (hjl.tools@gmail.com) Subject: Re: Three weeks to branching (gdb 7.5 release) References: <20120511181737.GP29339@adacore.com> <201205202043.q4KKhRGw022215@glazunov.sibelius.xs4all.nl> 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: 2012-05/txt/msg00754.txt.bz2 > Date: Sun, 20 May 2012 14:24:46 -0700 > From: "H.J. Lu" > > On Sun, May 20, 2012 at 1:43 PM, Mark Kettenis wrote: > >> Date: Sun, 20 May 2012 08:40:26 -0700 > >> From: "H.J. Lu" > >> > >> On Fri, May 11, 2012 at 11:43 AM, H.J. Lu wrote: > >> > On Fri, May 11, 2012 at 11:17 AM, Joel Brobecker wrote: > >> >> Hello, > >> >> > >> >> Just a quick heads up: The current tentative date for branching > >> >> GDB (7.5 release) is Mon Jun 4th, which is a little over three weeks > >> >> away. > >> >> > >> >> I've created a wiki page for known issues that need to be fixed > >> >> before then: > >> >> > >> >>    http://sourceware.org/gdb/wiki/GDB_7.5_Release > >> >> > >> >> When you add an issue, please make sure you add a name so we know > >> >> who is coordinating the effort.  If you don't know who can work > >> >> on it, please just post the issue here, and we'll try to find some > >> >> help. > >> >> > >> >> I only know of one issue, which is a noticeable performance degradation > >> >> that was reported a while ago: > >> >> > >> > > >> > I'd like to merge x32 into GDB 7.5.  My x32 change is on hjl/x32/master > >> > branch at > >> > > >> > http://sourceware.org/git/?p=gdb.git;a=summary > >> > > >> > The current diff only has 864 lines.  One patch: > >> > > >> > http://sourceware.org/ml/gdb-patches/2012-05/msg00097.html > >> > > >> > isn't reviewed yet.  I will open a meta bug for x32 integration. > >> > > >> > >> I opened: > >> > >> http://sourceware.org/bugzilla/show_bug.cgi?id=14099 > >> > >> Thanks for help from everyone.  The full GDBserver x32 support > >> as well as partial GDB x32 support have been checked in.  The > >> remaining patches are: > >> > >> http://sourceware.org/ml/gdb-patches/2012-04/msg00195.html > >> http://sourceware.org/ml/gdb-patches/2012-04/msg00191.html > >> http://sourceware.org/ml/gdb-patches/2012-05/msg00744.html > >> http://sourceware.org/ml/gdb-patches/2012-05/msg00531.html > >> http://sourceware.org/ml/gdb-patches/2012-05/msg00533.html > >> http://sourceware.org/ml/gdb-patches/2012-05/msg00489.html > >> http://sourceware.org/ml/gdb-patches/2012-05/msg00438.html > >> http://sourceware.org/ml/gdb-patches/2012-05/msg00097.html > >> > >> I would appreciate help to get them reviewed and approved. > > > > As I wrote before, I don't think adding lots if if-statements is the > > proper way to add a new ABI to GDB.  The proper way is to do it like > > the diff below.  In that diff, I'm not entirely confident that calling > > amd64_linux_init_abi() from amd64_x32_linux_init_abi() makes all that > > much sense.  For example the amd64_linux_record_tdep stuff probably > > isn't right for the x32 ABI.  But at least this will give us a > > starting point where we won't end up adding > > > >  if (gdbarch_ptr_bit (gdbarch) == 32) > >    { > >      ... > >    } > > Please take a look at > > http://sourceware.org/ml/gdb-patches/2012-04/msg00195.html > > It doesn't add any (gdbarch_ptr_bit (gdbarch) == 32). It just changes > it to bits_per_word. I add one "gdbarch_ptr_bit (gdbarch) == 32" in > amd64_linux_sigtramp_start and I will remove them from > > http://sourceware.org/ml/gdb-patches/2012-05/msg00744.html Well, I'm also thining about the future here. Stuff you haven't addressed yet in your diff series and stuff that will be added later. > > Index: amd64-linux-tdep.c > > =================================================================== > > RCS file: /cvs/src/src/gdb/amd64-linux-tdep.c,v > > retrieving revision 1.50 > > diff -u -p -r1.50 amd64-linux-tdep.c > > --- amd64-linux-tdep.c  12 May 2012 08:54:03 -0000      1.50 > > +++ amd64-linux-tdep.c  20 May 2012 20:31:53 -0000 > > @@ -1543,6 +1543,24 @@ amd64_linux_init_abi (struct gdbarch_inf > > > >   tdep->i386_syscall_record = amd64_linux_syscall_record; > >  } > > + > > +static void > > +amd64_x32_linux_init_abi(struct gdbarch_info info, struct gdbarch *gdbarch) > > +{ > > +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > > +  const struct target_desc *tdesc = info.target_desc; > > + > > +  amd64_linux_init_abi (info, gdbarch); > > +  amd64_x32_init_abi (info, gdbarch); > > + > > +  if (! tdesc_has_registers (tdesc)) > > +    tdesc = tdesc_amd64_linux; > > I assume you meant tdesc_x32_linux here. The problem is > when we reach here, if (! tdesc_has_registers (tdesc)) will always > be false since tdep->tdesc has been set by amd64_linux_init_abi. > > > +  tdep->tdesc = tdesc; > > + > > +  /* GNU/Linux uses SVR4-style shared libraries.  */ > > +  set_solib_svr4_fetch_link_map_offsets > > +    (gdbarch, svr4_ilp32_fetch_link_map_offsets); > > +} > > > > > >  /* Provide a prototype to silence -Wmissing-prototypes.  */ > > @@ -1553,6 +1571,8 @@ _initialize_amd64_linux_tdep (void) > >  { > >   gdbarch_register_osabi (bfd_arch_i386, bfd_mach_x86_64, > >                          GDB_OSABI_LINUX, amd64_linux_init_abi); > > +  gdbarch_register_osabi (bfd_arch_i386, bfd_mach_x64_32, > > +                         GDB_OSABI_LINUX, amd64_x32_linux_init_abi); > > > >   /* Initialize the Linux target description.  */ > >   initialize_tdesc_amd64_linux (); > > Index: amd64-tdep.c > > =================================================================== > > RCS file: /cvs/src/src/gdb/amd64-tdep.c,v > > retrieving revision 1.104 > > diff -u -p -r1.104 amd64-tdep.c > > --- amd64-tdep.c        14 May 2012 18:56:40 -0000      1.104 > > +++ amd64-tdep.c        20 May 2012 20:31:54 -0000 > > @@ -258,7 +258,8 @@ static const char *amd64_word_names[] = > >  static const char *amd64_dword_names[] = > >  { > >   "eax", "ebx", "ecx", "edx", "esi", "edi", "ebp", "esp", > > -  "r8d", "r9d", "r10d", "r11d", "r12d", "r13d", "r14d", "r15d" > > +  "r8d", "r9d", "r10d", "r11d", "r12d", "r13d", "r14d", "r15d", > > +  "eip" > >  }; > > > >  /* Return the name of register REGNUM.  */ > > @@ -2729,6 +2730,43 @@ amd64_init_abi (struct gdbarch_info info > >   set_gdbarch_stap_parse_special_token (gdbarch, > >                                        i386_stap_parse_special_token); > >  } > > + > > + > > +static struct type * > > +amd64_x32_pseudo_register_type (struct gdbarch *gdbarch, int regnum) > > +{ > > +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > > + > > +  switch (regnum - tdep->eax_regnum) > > +    { > > +    case AMD64_RBP_REGNUM:     /* %ebp */ > > +    case AMD64_RSP_REGNUM:     /* %esp */ > > +      return builtin_type (gdbarch)->builtin_data_ptr; > > +    case AMD64_RIP_REGNUM:     /* %eip */ > > +      return builtin_type (gdbarch)->builtin_func_ptr; > > +    } > > + > > +  return i386_pseudo_register_type (gdbarch, regnum); > > +} > > + > > +void > > +amd64_x32_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch) > > +{ > > +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > > +  const struct target_desc *tdesc = info.target_desc; > > + > > +  amd64_init_abi (info, gdbarch); > > + > > +  if (! tdesc_has_registers (tdesc)) > > +    tdesc = tdesc_x32; > > Again, " if (! tdesc_has_registers (tdesc))" will always false > since tdep->tdesc has been set in amd64_init_abi. I don't think that's true. Here tdesc comes from info.target_desc, not tdep->tdesc, so the check should still do the right thing. In fact it must do the right thing, since amd64_linux_init_abi() does the same thing already.