Re: [PATCH 3/3] libgcc: Implement TARGET_LIBGCC_REMOVE_DSO_HANDLE

[Jeff Law <law@redhat.com>]

On Wed, 2019-12-11 at 11:49 +0000, Jozef Lawrynowicz wrote:
> On Mon, 9 Dec 2019 13:05:22 +0000
> Jozef Lawrynowicz <jozef.l@mittosystems.com> wrote:
> 
> > On Sat, 07 Dec 2019 11:27:54 -0700
> > Jeff Law <law@redhat.com> wrote:
> > 
> > > On Wed, 2019-11-06 at 16:19 +0000, Jozef Lawrynowicz wrote:  
> > > > From 7bc0971d2936ebe71e7b7d3d805cf1bbf9f9f5af Mon Sep 17
> > > > 00:00:00 2001
> > > > From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
> > > > Date: Mon, 4 Nov 2019 17:38:13 +0000
> > > > Subject: [PATCH 3/3] libgcc: Implement
> > > > TARGET_LIBGCC_REMOVE_DSO_HANDLE
> > > > 
> > > > gcc/ChangeLog:
> > > > 
> > > > 2019-11-06  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
> > > > 
> > > > 	* doc/tm.texi: Regenerate.
> > > > 	* doc/tm.texi.in: Define
> > > > TARGET_LIBGCC_REMOVE_DSO_HANDLE.
> > > > 
> > > > libgcc/ChangeLog:
> > > > 
> > > > 2019-11-06  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
> > > > 
> > > > 	* crtstuff.c: Don't declare __dso_handle if
> > > > 	TARGET_LIBGCC_REMOVE_DSO_HANDLE is defined.    
> > > Presumably you'll switch this on for your bare elf target
> > > configuration?  
> > 
> > Yep that's the plan. I originally didn't include the target changes
> > in
> > this patch since other target changes (disabling __cxa_atexit) were
> > required for
> > the removal of __dso_handle to be OK.
> > 
> > > Are there other things, particularly related to shared library
> > > support,
> > > that we wouldn't need to use as well?  The reason I ask is I'm
> > > trying
> > > to figure out if REMOVE_DSO_HANDLE is the right name or if we
> > > should
> > > generalize it to a name that indicates shared libraries in
> > > general
> > > aren't supported on the target.  
> > 
> > CRTSTUFFS_O is defined when compiling shared versions of
> > crt{begin,end} and
> > handles an extra case in crtstuff.c where there's some shared
> > library related
> > functionality we don't need on MSP430.
> > 
> > But when CRTSTUFFS_O is undefined __dso_handle is still declared -
> > to 0. The
> > comment gives some additional insight:
> > 
> > /* Declare the __dso_handle variable.  It should have a unique
> > value  
> >    in every shared-object; in a main program its value is
> > zero.  The  
> >    object should in any case be protected.  This means the
> > instance   
> >    in one DSO or the main program is not used in another
> > object.  The 
> >    dynamic linker takes care of
> > this.  */                             
> > 
> > I haven't noticed any further shared library-related bloat coming
> > from libgcc.
> > 
> > I think a better way of solving this problem is just to check
> > DEFAULT_USE_CXA_ATEXIT rather than adding this new macro. If
> > __cxa_atexit is
> > not enabled then as far as I understand __dso_handle serves no
> > purpose.
> > DEFAULT_USE_CXA_ATEXIT is defined at configure time for any targets
> > that want
> > __cxa_atexit support.
> > 
> > A quick bootstrap and test of dg.exp on x86_64-pc-linux-gnu shows
> > no issues
> > with the following:
> > 
> > > diff --git a/libgcc/crtstuff.c b/libgcc/crtstuff.c
> > > index ae6328d317d..349f8191e61 100644
> > > --- a/libgcc/crtstuff.c
> > > +++ b/libgcc/crtstuff.c
> > > @@ -340,8 +340,10 @@ extern void *__dso_handle __attribute__
> > > ((__visibility__ ("hidden")));
> > >  #ifdef CRTSTUFFS_O
> > >  void *__dso_handle = &__dso_handle;
> > >  #else
> > > +#if DEFAULT_USE_CXA_ATEXIT
> > >  void *__dso_handle = 0;
> > >  #endif
> > > +#endif
> > >  
> > >  /* The __cxa_finalize function may not be available so we use
> > > only a
> > >     weak declaration.  */  
> > 
> > I'll put that patch through some more rigorous testing.
> 
> Successfully bootstrapped and regtested the attached patch for
> x86_64-pc-linux-gnu (where DEFAULT_USE_CXA_ATEXIT is set to 1) and
> the proposed
> msp430-elfbare target (where DEFAULT_USE_CXA_ATEXIT is set to 0).
> 
> > libgcc/ChangeLog:
> > 
> > 2019-12-11  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
> > 
> > 	* crtstuff.c: Declare __dso_handle only if
> > DEFAULT_USE_CXA_ATEXIT is
> > 	true.
OK
jeff

ps.  Sorry about the formatting.  Had to switch MUAs last week and
still haven't gotten all the kinks worked out.