diff mbox

Don't use INTDEF/INTUSE in unwind-dw2-fde.c (bug 14132)

Message ID Pine.LNX.4.64.1410222356390.5108@digraph.polyomino.org.uk
State Committed
Headers show

Commit Message

Joseph Myers Oct. 22, 2014, 11:57 p.m. UTC
Continuing the removal of the obsolete INTDEF / INTUSE mechanism, this
patch replaces its use in unwind-dw2-fde.c with hidden_def and
hidden_proto.

Tested for x86.  This patch does result in code generation differences
(for some reason GCC decides to partition __register_frame_info_bases
after the patch).

2014-10-22  Joseph Myers  <joseph@codesourcery.com>

	[BZ #14132]
	* sysdeps/generic/unwind-dw2-fde.c
	(__register_frame_info_bases_internal): Do not declare.
	(__register_frame_info_table_bases_internal): Likewise.
	(__deregister_frame_info_bases_internal): Likewise.
	(__register_frame_info_bases): Declare and use hidden_proto before
	definition.  Use hidden_def instead of INTDEF.
	(__register_frame_info_table_bases): Likewise.
	(__deregister_frame_info_bases): Likewise.
	(__register_frame_info): Do not use INTUSE.
	(__register_frame): Likewise.
	(__register_frame_info_table): Likewise.
	(__register_frame_table): Likewise.
	(__deregister_frame_info): Likewise.
	(__deregister_frame): Likewise.

Comments

Joseph Myers Oct. 28, 2014, 1:21 p.m. UTC | #1
Ping.  This patch 
<https://sourceware.org/ml/libc-alpha/2014-10/msg00491.html> is pending 
review.
Carlos O'Donell Oct. 28, 2014, 3:39 p.m. UTC | #2
On 10/22/2014 07:57 PM, Joseph S. Myers wrote:
> Continuing the removal of the obsolete INTDEF / INTUSE mechanism, this
> patch replaces its use in unwind-dw2-fde.c with hidden_def and
> hidden_proto.
> 
> Tested for x86.  This patch does result in code generation differences
> (for some reason GCC decides to partition __register_frame_info_bases
> after the patch).
> 
> 2014-10-22  Joseph Myers  <joseph@codesourcery.com>
> 
> 	[BZ #14132]
> 	* sysdeps/generic/unwind-dw2-fde.c

<off topic>
Is this file shared with libgcc and should it be listed here?

https://sourceware.org/glibc/wiki/SharedSourceFiles

.. and while I'm thinking about it, should the soft-fp code be 
listed in our `SharedSourceFiles` wiki page?
</off topic>

> 	(__register_frame_info_bases_internal): Do not declare.
> 	(__register_frame_info_table_bases_internal): Likewise.
> 	(__deregister_frame_info_bases_internal): Likewise.
> 	(__register_frame_info_bases): Declare and use hidden_proto before
> 	definition.  Use hidden_def instead of INTDEF.
> 	(__register_frame_info_table_bases): Likewise.
> 	(__deregister_frame_info_bases): Likewise.
> 	(__register_frame_info): Do not use INTUSE.
> 	(__register_frame): Likewise.
> 	(__register_frame_info_table): Likewise.
> 	(__register_frame_table): Likewise.
> 	(__deregister_frame_info): Likewise.
> 	(__deregister_frame): Likewise.

OK to commit.

> diff --git a/sysdeps/generic/unwind-dw2-fde.c b/sysdeps/generic/unwind-dw2-fde.c
> index ba003a9..5be0816 100644
> --- a/sysdeps/generic/unwind-dw2-fde.c
> +++ b/sysdeps/generic/unwind-dw2-fde.c
> @@ -60,12 +60,15 @@ __libc_lock_define_initialized (static, object_mutex)
>  #define __gthread_mutex_lock(m) __libc_lock_lock (*(m))
>  #define __gthread_mutex_unlock(m) __libc_lock_unlock (*(m))
>  
> -void __register_frame_info_bases_internal (void *begin, struct object *ob,
> -					   void *tbase, void *dbase);
> -void __register_frame_info_table_bases_internal (void *begin,
> -						 struct object *ob,
> -						 void *tbase, void *dbase);
> -void *__deregister_frame_info_bases_internal (void *begin);
> +void __register_frame_info_bases (void *begin, struct object *ob,
> +				  void *tbase, void *dbase);
> +hidden_proto (__register_frame_info_bases)
> +void __register_frame_info_table_bases (void *begin,
> +					struct object *ob,
> +					void *tbase, void *dbase);
> +hidden_proto (__register_frame_info_table_bases)
> +void *__deregister_frame_info_bases (void *begin);
> +hidden_proto (__deregister_frame_info_bases)

OK.

>  
>  #else
>  
> @@ -122,12 +125,12 @@ __register_frame_info_bases (void *begin, struct object *ob,
>  
>    __gthread_mutex_unlock (&object_mutex);
>  }
> -INTDEF(__register_frame_info_bases)
> +hidden_def (__register_frame_info_bases)
>  
>  void
>  __register_frame_info (void *begin, struct object *ob)
>  {
> -  INTUSE(__register_frame_info_bases) (begin, ob, 0, 0);
> +  __register_frame_info_bases (begin, ob, 0, 0);
>  }
>  
>  void
> @@ -140,7 +143,7 @@ __register_frame (void *begin)
>      return;
>  
>    ob = (struct object *) malloc (sizeof (struct object));
> -  INTUSE(__register_frame_info_bases) (begin, ob, 0, 0);
> +  __register_frame_info_bases (begin, ob, 0, 0);
>  }
>  
>  /* Similar, but BEGIN is actually a pointer to a table of unwind entries
> @@ -167,19 +170,19 @@ __register_frame_info_table_bases (void *begin, struct object *ob,
>  
>    __gthread_mutex_unlock (&object_mutex);
>  }
> -INTDEF(__register_frame_info_table_bases)
> +hidden_def (__register_frame_info_table_bases)
>  
>  void
>  __register_frame_info_table (void *begin, struct object *ob)
>  {
> -  INTUSE(__register_frame_info_table_bases) (begin, ob, 0, 0);
> +  __register_frame_info_table_bases (begin, ob, 0, 0);
>  }
>  
>  void
>  __register_frame_table (void *begin)
>  {
>    struct object *ob = (struct object *) malloc (sizeof (struct object));
> -  INTUSE(__register_frame_info_table_bases) (begin, ob, 0, 0);
> +  __register_frame_info_table_bases (begin, ob, 0, 0);
>  }
>  
>  /* Called from crtbegin.o to deregister the unwind info for an object.  */
> @@ -243,12 +246,12 @@ __deregister_frame_info_bases (void *begin)
>    __gthread_mutex_unlock (&object_mutex);
>    return (void *) ob;
>  }
> -INTDEF(__deregister_frame_info_bases)
> +hidden_def (__deregister_frame_info_bases)
>  
>  void *
>  __deregister_frame_info (void *begin)
>  {
> -  return INTUSE(__deregister_frame_info_bases) (begin);
> +  return __deregister_frame_info_bases (begin);
>  }
>  
>  void
> @@ -256,7 +259,7 @@ __deregister_frame (void *begin)
>  {
>    /* If .eh_frame is empty, we haven't registered.  */
>    if (*(uword *) begin != 0)
> -    free (INTUSE(__deregister_frame_info_bases) (begin));
> +    free (__deregister_frame_info_bases (begin));
>  }
>  
>  
> 

Looks good to me.

Cheers,
Carlos.
Joseph Myers Oct. 28, 2014, 5:11 p.m. UTC | #3
On Tue, 28 Oct 2014, Carlos O'Donell wrote:

> > 2014-10-22  Joseph Myers  <joseph@codesourcery.com>
> > 
> > 	[BZ #14132]
> > 	* sysdeps/generic/unwind-dw2-fde.c
> 
> <off topic>
> Is this file shared with libgcc and should it be listed here?
> 
> https://sourceware.org/glibc/wiki/SharedSourceFiles

It's not shared in the sense of being useful to have a single version that 
works in both places.  This file is in glibc purely for binary 
compatibility with binaries built with old glibc and GCC versions (from 
before symbol versioning, when glibc accidentally re-exported various 
symbols from libgcc) - so there is no use in it supporting newer unwind 
info features generated by newer GCC (unwinding from thread cancellation 
uses the copy of the unwinder in shared libgcc, not this compat copy).  
But those unwinder files did originate from libgcc, yes.

It might be possible to eliminate those copies by making libc on affected 
architectures dlopen shared libgcc at startup and use the functions from 
there in the same way as done for thread cancellation, but that might not 
be desirable given most binaries wouldn't need it (really you'd want it to 
be dlopened only for binaries referencing those symbols; lazy opening is 
problematic for error reporting).

All the symbols conditioned on EXPORT_UNWIND_FIND_FDE in elf/Versions 
really ought to be compat symbols, although they aren't at present (see 
the note on the wiki todo list about reviewing exported symbols starting 
with '_' in general).

> .. and while I'm thinking about it, should the soft-fp code be 
> listed in our `SharedSourceFiles` wiki page?

Yes, it's shared, with the glibc version getting copied verbatim to 
libgcc.  I still need to implement one feature for the glibc version 
(FP_TO_INT_ROUND) before it has all the features needed to replace the old 
fork of the code in the Linux kernel (replacing the kernel version will 
also be a lot of work, given the various interface changes since it 
diverged over 10 years ago).
Rich Felker Oct. 28, 2014, 8 p.m. UTC | #4
On Tue, Oct 28, 2014 at 05:11:37PM +0000, Joseph S. Myers wrote:
> It might be possible to eliminate those copies by making libc on affected 
> architectures dlopen shared libgcc at startup and use the functions from 
> there in the same way as done for thread cancellation, but that might not 
> be desirable given most binaries wouldn't need it (really you'd want it to 
> be dlopened only for binaries referencing those symbols; lazy opening is 
> problematic for error reporting).

Doing that would be really backwards. What's needed is really the
opposite -- getting rid of the dlopen of libgcc entirely. Right now
pthread_cancel aborts the whole program if loading libgcc fails, and
loading can fail for all sorts of spurious reasons.

Rich
Joseph Myers Oct. 28, 2014, 10:41 p.m. UTC | #5
On Tue, 28 Oct 2014, Rich Felker wrote:

> On Tue, Oct 28, 2014 at 05:11:37PM +0000, Joseph S. Myers wrote:
> > It might be possible to eliminate those copies by making libc on affected 
> > architectures dlopen shared libgcc at startup and use the functions from 
> > there in the same way as done for thread cancellation, but that might not 
> > be desirable given most binaries wouldn't need it (really you'd want it to 
> > be dlopened only for binaries referencing those symbols; lazy opening is 
> > problematic for error reporting).
> 
> Doing that would be really backwards. What's needed is really the
> opposite -- getting rid of the dlopen of libgcc entirely. Right now
> pthread_cancel aborts the whole program if loading libgcc fails, and
> loading can fail for all sorts of spurious reasons.

As explained before, the dlopen helps avoid circular dependencies between 
glibc and GCC when bootstrapping - it's desirable to be able to build 
glibc with an initial bootstrap compiler whose libgcc was built without 
libc headers or shared libc (and so is static only with limited unwinding 
functionality) and have it be identical to one built after iterating a 
series of alternate GCC and glibc builds.  The obvious way to avoid that 
failure at pthread_cancel time is to do the dlopen at libpthread startup 
instead.
diff mbox

Patch

diff --git a/sysdeps/generic/unwind-dw2-fde.c b/sysdeps/generic/unwind-dw2-fde.c
index ba003a9..5be0816 100644
--- a/sysdeps/generic/unwind-dw2-fde.c
+++ b/sysdeps/generic/unwind-dw2-fde.c
@@ -60,12 +60,15 @@  __libc_lock_define_initialized (static, object_mutex)
 #define __gthread_mutex_lock(m) __libc_lock_lock (*(m))
 #define __gthread_mutex_unlock(m) __libc_lock_unlock (*(m))
 
-void __register_frame_info_bases_internal (void *begin, struct object *ob,
-					   void *tbase, void *dbase);
-void __register_frame_info_table_bases_internal (void *begin,
-						 struct object *ob,
-						 void *tbase, void *dbase);
-void *__deregister_frame_info_bases_internal (void *begin);
+void __register_frame_info_bases (void *begin, struct object *ob,
+				  void *tbase, void *dbase);
+hidden_proto (__register_frame_info_bases)
+void __register_frame_info_table_bases (void *begin,
+					struct object *ob,
+					void *tbase, void *dbase);
+hidden_proto (__register_frame_info_table_bases)
+void *__deregister_frame_info_bases (void *begin);
+hidden_proto (__deregister_frame_info_bases)
 
 #else
 
@@ -122,12 +125,12 @@  __register_frame_info_bases (void *begin, struct object *ob,
 
   __gthread_mutex_unlock (&object_mutex);
 }
-INTDEF(__register_frame_info_bases)
+hidden_def (__register_frame_info_bases)
 
 void
 __register_frame_info (void *begin, struct object *ob)
 {
-  INTUSE(__register_frame_info_bases) (begin, ob, 0, 0);
+  __register_frame_info_bases (begin, ob, 0, 0);
 }
 
 void
@@ -140,7 +143,7 @@  __register_frame (void *begin)
     return;
 
   ob = (struct object *) malloc (sizeof (struct object));
-  INTUSE(__register_frame_info_bases) (begin, ob, 0, 0);
+  __register_frame_info_bases (begin, ob, 0, 0);
 }
 
 /* Similar, but BEGIN is actually a pointer to a table of unwind entries
@@ -167,19 +170,19 @@  __register_frame_info_table_bases (void *begin, struct object *ob,
 
   __gthread_mutex_unlock (&object_mutex);
 }
-INTDEF(__register_frame_info_table_bases)
+hidden_def (__register_frame_info_table_bases)
 
 void
 __register_frame_info_table (void *begin, struct object *ob)
 {
-  INTUSE(__register_frame_info_table_bases) (begin, ob, 0, 0);
+  __register_frame_info_table_bases (begin, ob, 0, 0);
 }
 
 void
 __register_frame_table (void *begin)
 {
   struct object *ob = (struct object *) malloc (sizeof (struct object));
-  INTUSE(__register_frame_info_table_bases) (begin, ob, 0, 0);
+  __register_frame_info_table_bases (begin, ob, 0, 0);
 }
 
 /* Called from crtbegin.o to deregister the unwind info for an object.  */
@@ -243,12 +246,12 @@  __deregister_frame_info_bases (void *begin)
   __gthread_mutex_unlock (&object_mutex);
   return (void *) ob;
 }
-INTDEF(__deregister_frame_info_bases)
+hidden_def (__deregister_frame_info_bases)
 
 void *
 __deregister_frame_info (void *begin)
 {
-  return INTUSE(__deregister_frame_info_bases) (begin);
+  return __deregister_frame_info_bases (begin);
 }
 
 void
@@ -256,7 +259,7 @@  __deregister_frame (void *begin)
 {
   /* If .eh_frame is empty, we haven't registered.  */
   if (*(uword *) begin != 0)
-    free (INTUSE(__deregister_frame_info_bases) (begin));
+    free (__deregister_frame_info_bases (begin));
 }