Don't use INTDEF/INTUSE in unwind-dw2-fde.c (bug 14132)
Commit Message
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
Ping. This patch
<https://sourceware.org/ml/libc-alpha/2014-10/msg00491.html> is pending
review.
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.
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).
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
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.
@@ -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));
}