This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PATCH: PR ld/5303: splay-tree doesn't support 64bit value on 32bit host


Hi HJ,

    I don't have right to check into the binutils source tree. So my
comments cannot be counted as a review :).

  There are other ways to fix this without affecting gcc. This is a
speed optimization.  The easiest way is to turn this off when spray
tree nodes cannot store 64-bit target addresses.  Currently there is
only one use of the arange set data structure inside lib BFD and there
is  logic to determine when this is turned on.  We can simply change
that.

   Another way to fix this is always allocate a structure to store
target VMA and have the spray tree storing pointers to structures
instead of storing VMA directly.  That uses a bit more memory and
cycles.  I can do the changes if we think this is a better approach.

-Doug

2007/12/4, H.J. Lu <hjl@lucon.org>:
> Hi Doug,
>
> Thanks for your comments.
>
> Although this bug doesn't affect gcc since gcc doesn't store 64bit
> values in splay-tree on 32bit hosts, splay-tree is shared by both gcc
> and binutils. Could someone from gcc review my patches to gcc and
> splay-tree? Mark, DJ, can you look into them?
>
> Thanks.
>
>
> H.J.
> ---
> On Mon, Dec 03, 2007 at 07:39:38AM -0800, Doug Kwan (w) wrote:
> > Hi H.J.
> >
> >    Thanks for fixing this. I was on vacation in the last 3 weeks and
> > out of the country. I only returned to work to day. My apology for not
> > having tested this on the 32-bit host/64-bit target in the first
> > place.
> >
> > -Doug
> >
> > 2007/11/14, H.J. Lu <hjl@lucon.org>:
> > > On Tue, Nov 13, 2007 at 03:32:38PM -0800, H.J. Lu wrote:
> > > > On Wed, Nov 14, 2007 at 09:05:08AM +1030, Alan Modra wrote:
> > > > > On Tue, Nov 13, 2007 at 09:18:45AM -0800, H.J. Lu wrote:
> > > > > > Current linker is broken on 32bit host with 64bit BFD. The problem
> > > > > > is splay-tree doesn't support 64bit value on bit host, but arange-set.c
> > > > > > uses splay-tree on bfd_vma. Fixing it isn't easy since splay-tree is
> > > > > > also used in gcc. We can't use #ifdef in splay-tree.h in gcc due
> > > > > > to lack of #ifdef in gengtype in gcc. This patch will abort the
> > > > > > linker if it detects such situation.
> > > > >
> > > > > Aborting isn't a solution.  I'll revert Doug's patch in a week or so
> > > > > if someone can't find a proper solution, eg. fallback to old scheme
> > > > > if splay tree support is inadeqaute.
> > > > >
> > > >
> > > > Here are 2 patches to update splay tree. The idea is to provide both
> > > > long and long long interfaces for splay-tree if they are different.
> > > > The default interface is the smallest one which can hold a pointer.
> > > > Gcc support is tricky since gengtype doesn't support #ifdef. I
> > > > have to use splay-tree-default.h and let gcc override it. I will
> > > > post a patch for arange-set.c if these 2 patches are approved.
> > > >
> > > > Thanks.
> > > >
> > > >
> > >
> > > Here is the patch for bfd.
> > >
> > >
> > > H.J.
> > > ----
> > > 2007-11-14  H.J. Lu  <hongjiu.lu@intel.com>
> > >
> > >         PR ld/5303
> > >         * arange-set.c (USE_SPLAY_TREE_LONG_LONG): Define if BFD64 is
> > >         defined.
> > >         Don't include "splay-tree.h".
> > >         (arange_set_delete_value_container): Add cast to
> > >         arange_set_uhostptr_t.
> > >         (arange_set_node_high): Likewise.
> > >         (arange_set_node_set_high): Likewise.
> > >         (arange_set_node_value): Likewise.
> > >         (arange_set_node_set_value): Likewise.
> > >         (arange_set_splay_tree_insert): Likewise.
> > >
> > >         * arange-set.h: Include "splay-tree.h".
> > >         (arange_set_uhostptr_t): Defined as splay_tree_uhostptr_t.
> > >
> > >         * configure.in: Check if long long exists.
> > >         * config.in: Regenerated.
> > >         * configure: Likewise.
> > >
> > > --- bfd/arange-set.c.vma        2007-09-21 09:16:17.000000000 -0700
> > > +++ bfd/arange-set.c    2007-11-13 06:46:18.000000000 -0800
> > > @@ -23,8 +23,12 @@
> > >  #include "bfd.h"
> > >  #include "libiberty.h"
> > >  #include "libbfd.h"
> > > +
> > > +#ifdef BFD64
> > > +#define USE_SPLAY_TREE_LONG_LONG
> > > +#endif
> > > +
> > >  #include "arange-set.h"
> > > -#include "splay-tree.h"
> > >
> > >  /* Implementation of an arange-set.  The set is implemented using the
> > >     splay tree support in libiberty.  The advantage of using this is
> > > @@ -142,7 +146,8 @@ arange_set_delete_value_container (splay
> > >  {
> > >    arange_value_container_t *container;
> > >
> > > -  container = (arange_value_container_t*) value;
> > > +  container
> > > +    = (arange_value_container_t*) (arange_set_uhostptr_t)value;
> > >    arange_set_delete_value (container->set, container->value);
> > >    arange_set_deallocate (container->set, container);
> > >  }
> > > @@ -255,7 +260,8 @@ arange_set_node_high (arange_set set, sp
> > >
> > >    if (set->value_p)
> > >      {
> > > -      container = (arange_value_container_t*) node->value;
> > > +      container
> > > +       = (arange_value_container_t*) (arange_set_uhostptr_t)node->value;
> > >        return container->high;
> > >      }
> > >
> > > @@ -271,7 +277,8 @@ arange_set_node_set_high (arange_set set
> > >
> > >    if (set->value_p)
> > >      {
> > > -      container = (arange_value_container_t*) node->value;
> > > +      container
> > > +       = (arange_value_container_t*) (arange_set_uhostptr_t)node->value;
> > >        container->high = address;
> > >      }
> > >    else
> > > @@ -296,7 +303,8 @@ arange_set_node_value (arange_set set, s
> > >
> > >    if (set->value_p)
> > >      {
> > > -      container = (arange_value_container_t*) node->value;
> > > +      container
> > > +       = (arange_value_container_t*) (arange_set_uhostptr_t)node->value;
> > >        return container->value;
> > >      }
> > >
> > > @@ -315,7 +323,8 @@ arange_set_node_set_value (arange_set se
> > >
> > >    if (set->value_p)
> > >      {
> > > -      container = (arange_value_container_t*) node->value;
> > > +      container
> > > +       = (arange_value_container_t*) (arange_set_uhostptr_t)node->value;
> > >        container->value = value;
> > >      }
> > >  }
> > > @@ -445,7 +454,7 @@ arange_set_splay_tree_insert (arange_set
> > >        container->set = set;
> > >
> > >        container->value = value;
> > > -      sp_value = (splay_tree_value) container;
> > > +      sp_value = (splay_tree_value) (arange_set_uhostptr_t)container;
> > >      }
> > >    else
> > >      sp_value = (splay_tree_value) high;
> > > --- bfd/arange-set.h.vma        2007-09-21 09:16:17.000000000 -0700
> > > +++ bfd/arange-set.h    2007-11-13 06:41:48.000000000 -0800
> > > @@ -75,16 +75,13 @@
> > >
> > >  #include "sysdep.h"
> > >  #include "bfd.h"
> > > +#include "splay-tree.h"
> > >
> > >  /* An arange_set is a pointer to an arange_set_s struct, whose implementation
> > >     is opaque to clients using the arange set.  */
> > >  typedef struct arange_set_s *arange_set;
> > >
> > > -#ifndef _WIN64
> > > -  typedef unsigned long int arange_set_uhostptr_t;
> > > -#else
> > > -  typedef unsigned long long arange_set_uhostptr_t;
> > > -#endif
> > > +typedef splay_tree_uhostptr_t arange_set_uhostptr_t;
> > >
> > >  /* Type of value attached to an arange.  This should be wide enough to be
> > >     converted from and back to any type without loss.  */
> > > --- bfd/config.in.vma   2007-10-30 11:48:31.000000000 -0700
> > > +++ bfd/config.in       2007-11-13 06:31:39.000000000 -0800
> > > @@ -105,6 +105,9 @@
> > >  /* Define to 1 if you have the <inttypes.h> header file. */
> > >  #undef HAVE_INTTYPES_H
> > >
> > > +/* Define to 1 if the system has the type `long long'. */
> > > +#undef HAVE_LONG_LONG
> > > +
> > >  /* Define if <sys/procfs.h> has lwpstatus_t. */
> > >  #undef HAVE_LWPSTATUS_T
> > >
> > > --- bfd/configure.in.vma        2007-10-30 11:48:31.000000000 -0700
> > > +++ bfd/configure.in    2007-11-13 06:31:22.000000000 -0800
> > > @@ -135,7 +135,7 @@ BFD_HOST_64_BIT=
> > >  BFD_HOST_U_64_BIT=
> > >  BFD_HOSTPTR_T="unsigned long"
> > >
> > > -AC_CHECK_SIZEOF(long long)
> > > +AC_CHECK_TYPES([long long], [AC_CHECK_SIZEOF(long long)])
> > >  AC_CHECK_SIZEOF(void *)
> > >  AC_CHECK_SIZEOF(long)
> > >
> > >
>

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]