Re: Three weeks to branching (gdb 7.5 release)

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

> Date: Sun, 20 May 2012 14:24:46 -0700
> From: "H.J. Lu" <hjl.tools@gmail.com>
> 
> On Sun, May 20, 2012 at 1:43 PM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >> Date: Sun, 20 May 2012 08:40:26 -0700
> >> From: "H.J. Lu" <hjl.tools@gmail.com>
> >>
> >> On Fri, May 11, 2012 at 11:43 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> >> > On Fri, May 11, 2012 at 11:17 AM, Joel Brobecker <brobecker@adacore.com> 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.