[RFC,1/2] dlfcn: add long double redirect wrappers for dl*sym

Message ID 241f0fb69316e533e3fc5025bab74b40bb7ab5c9.1591302826.git.murphyp@linux.vnet.ibm.com
State Dropped
Headers
Series [RFC,1/2] dlfcn: add long double redirect wrappers for dl*sym |

Commit Message

Paul E. Murphy June 4, 2020, 8:35 p.m. UTC
  Using a non-default long double creates some odd issues when
attempting to dynamically load symbols.  I am not sure how
big an issue this really is, but it is worth putting forth
patches to have this discussion.  Users of the IEEE128 ldbl
will run into ABI problems if they attempt to load symbols
which are renamed (mangled via asm redirects).  A first
attempt here is to mangle lib{c,m,dl} symbol names which are
redirected via headers by wrapping dl*sym functions.  Given
that the renames are not always trivial and may grow with
future releases, this is something the end user should not
have to do.

Prepare to support mangling certain symbol names which can be
redirected depending on the choice of long double format the
caller object was built against.
---
 dlfcn/dlerror.c                               |  4 +--
 dlfcn/dlfcn.h                                 |  9 +++++
 dlfcn/dlsym.c                                 | 21 ++++++++----
 dlfcn/dlvsym.c                                | 22 ++++++++-----
 include/dlfcn.h                               | 10 ++++--
 sysdeps/ieee754/ldbl-128ibm-compat/Makefile   |  6 ++++
 sysdeps/ieee754/ldbl-128ibm-compat/Versions   |  7 ++++
 .../ldbl-128ibm-compat/ieee128-dlfcn.c        | 33 +++++++++++++++++++
 .../linux/powerpc/powerpc64/le/libdl.abilist  |  2 ++
 9 files changed, 94 insertions(+), 20 deletions(-)
 create mode 100644 sysdeps/ieee754/ldbl-128ibm-compat/ieee128-dlfcn.c
  

Comments

Joseph Myers June 5, 2020, 2:33 p.m. UTC | #1
These are certainly not the only functions for which there are 
header-based redirections depending on the options with which code is 
compiled, and nor is dlsym the only place with a use for such information 
about naming (the issue also applies to function calls from non-C 
languages, where GCC may need to know the name to handle built-in function 
calls correctly).  I wonder if there is some better way to automate 
extraction of information from the headers of what the redirections are, 
that both avoids duplicating a long list of function names and avoids 
needing to special-case one subset of redirections for dlsym.
  
Paul E Murphy June 8, 2020, 3:08 p.m. UTC | #2
On 6/5/20 9:33 AM, Joseph Myers wrote:
> These are certainly not the only functions for which there are
> header-based redirections depending on the options with which code is
> compiled, and nor is dlsym the only place with a use for such information
> about naming (the issue also applies to function calls from non-C
> languages, where GCC may need to know the name to handle built-in function
> calls correctly).  I wonder if there is some better way to automate
> extraction of information from the headers of what the redirections are,
> that both avoids duplicating a long list of function names and avoids
> needing to special-case one subset of redirections for dlsym.

We can try to make the C runtime play better.  There isn't much I can 
directly do for other projects which implement their own runtime, but 
they will need to consider some non-trivial strategy if they wish to 
support the IEEE128 format leveraging glibc.

There are a few other symbols which are redirected (the ISO C99 printf 
functions come to mind).  These patches should not change the existing 
behavior, though such symbols should be considered when mangling onto 
the ldbl128 abi.

Builtin calls are something GCC will need to handled, and this is 
brought up as part of PR 94630.

How many special cases for dlsym would there be today?  I could see 
three for ppc64le (ldbl == ibm, ldbl == dbl, ldbl == ieee128).  I have 
not seen this issue raised with ldbl == dbl redirects.

I would like to commit a patch to redirect dlsym/dlvsym in the event I 
need to backport a solution, is that objectionable?  Generating the 
mangles from headers would be non-trivial, do you think this could be 
done at build without additional built time dependencies?
  
Joseph Myers June 8, 2020, 3:18 p.m. UTC | #3
On Mon, 8 Jun 2020, Paul E Murphy wrote:

> I would like to commit a patch to redirect dlsym/dlvsym in the event I need to
> backport a solution, is that objectionable?  Generating the mangles from

I think this is probably outside the scope of what dlsym/dlvsym should be 
doing at all.

It's definitely the case that if someone uses dlsym/dlvsym to look up 
symbols from their own libraries (not libc / libm), they must get exactly 
the symbols with those names and versions from their own libraries and not 
something remapped.  And if they use dlvsym at all, for any library, 
that's clearly referencing a precise symbol that should not be remapped; 
if the name and version referenced are for an IBM long double symbol, 
that's a perfectly valid way of accessing that IBM long double symbol in a 
program using IEEE binary128 long double and it would be wrong for glibc 
to suppose otherwise.  I tend to think that dlsym should be the same even 
when used on libc or libm: it should return the symbol (same version that 
a newly linked binary would get, see bug 14932) with that name in the 
dynamic ELF symbol table, with it being the application's job to map from 
C API names to the names required at the ELF level.
  
Florian Weimer June 8, 2020, 4:18 p.m. UTC | #4
* Paul E. Murphy via Libc-alpha:

> Using a non-default long double creates some odd issues when
> attempting to dynamically load symbols.  I am not sure how
> big an issue this really is, but it is worth putting forth
> patches to have this discussion.  Users of the IEEE128 ldbl
> will run into ABI problems if they attempt to load symbols
> which are renamed (mangled via asm redirects).  A first
> attempt here is to mangle lib{c,m,dl} symbol names which are
> redirected via headers by wrapping dl*sym functions.  Given
> that the renames are not always trivial and may grow with
> future releases, this is something the end user should not
> have to do.

We have seen such problems only with underlinking, not with dlsym.  But
that may be because for the other targets that underwent the transition,
version-less dlsym lookups point to the most recent version, which has
the default long double.

Thanks,
Florian
  

Patch

diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
index 0d38a37bf7..b94516af5b 100644
--- a/dlfcn/dlerror.c
+++ b/dlfcn/dlerror.c
@@ -261,8 +261,8 @@  static struct dlfcn_hook _dlfcn_hooks =
   {
     .dlopen = __dlopen,
     .dlclose = __dlclose,
-    .dlsym = __dlsym,
-    .dlvsym = __dlvsym,
+    .dlsym = __dlsym_internal,
+    .dlvsym = __dlvsym_internal,
     .dlerror = __dlerror,
     .dladdr = __dladdr,
     .dladdr1 = __dladdr1,
diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
index c629fc7f42..baf709375a 100644
--- a/dlfcn/dlfcn.h
+++ b/dlfcn/dlfcn.h
@@ -64,6 +64,10 @@  extern int dlclose (void *__handle) __THROWNL __nonnull ((1));
 extern void *dlsym (void *__restrict __handle,
 		    const char *__restrict __name) __THROW __nonnull ((2));
 
+#if __LDOUBLE_REDIRECTS_TO_FLOAT128_ABI == 1 && defined (__REDIRECT)
+__LDBL_REDIR_DECL (dlsym);
+#endif
+
 #ifdef __USE_GNU
 /* Like `dlopen', but request object to be allocated in a new namespace.  */
 extern void *dlmopen (Lmid_t __nsid, const char *__file, int __mode) __THROWNL;
@@ -74,6 +78,11 @@  extern void *dlvsym (void *__restrict __handle,
 		     const char *__restrict __name,
 		     const char *__restrict __version)
      __THROW __nonnull ((2, 3));
+
+#if __LDOUBLE_REDIRECTS_TO_FLOAT128_ABI == 1 && defined (__REDIRECT)
+__LDBL_REDIR_DECL (dlvsym);
+#endif
+
 #endif
 
 /* When any of the above functions fails, call this function
diff --git a/dlfcn/dlsym.c b/dlfcn/dlsym.c
index e9ee596021..5cf9730955 100644
--- a/dlfcn/dlsym.c
+++ b/dlfcn/dlsym.c
@@ -26,11 +26,20 @@ 
 void *
 dlsym (void *handle, const char *name)
 {
-  return __dlsym (handle, name, RETURN_ADDRESS (0));
+  return __dlsym_internal (handle, name, RETURN_ADDRESS (0));
 }
 
 #else
 
+# ifdef SHARED
+void *
+__dlsym (void *handle, const char *name)
+{
+  return __dlsym_internal (handle, name, RETURN_ADDRESS (0));
+}
+strong_alias (__dlsym, dlsym)
+# endif
+
 struct dlsym_args
 {
   /* The arguments to dlsym_doit.  */
@@ -52,15 +61,15 @@  dlsym_doit (void *a)
 
 
 void *
-__dlsym (void *handle, const char *name DL_CALLER_DECL)
+__dlsym_internal (void *handle, const char *name, void *dl_caller)
 {
 # ifdef SHARED
   if (!rtld_active ())
-    return _dlfcn_hook->dlsym (handle, name, DL_CALLER);
+    return _dlfcn_hook->dlsym (handle, name, dl_caller);
 # endif
 
   struct dlsym_args args;
-  args.who = DL_CALLER;
+  args.who = dl_caller;
   args.handle = handle;
   args.name = name;
 
@@ -73,7 +82,5 @@  __dlsym (void *handle, const char *name DL_CALLER_DECL)
 
   return result;
 }
-# ifdef SHARED
-strong_alias (__dlsym, dlsym)
-# endif
+
 #endif
diff --git a/dlfcn/dlvsym.c b/dlfcn/dlvsym.c
index 021697fb75..5aa9974104 100644
--- a/dlfcn/dlvsym.c
+++ b/dlfcn/dlvsym.c
@@ -27,11 +27,20 @@  void *
 weak_function
 dlvsym (void *handle, const char *name, const char *version_str)
 {
-  return __dlvsym (handle, name, version_str, RETURN_ADDRESS (0));
+  return __dlvsym_internal (handle, name, version_str, RETURN_ADDRESS (0));
 }
 
 #else
 
+# ifdef SHARED
+void *
+__dlvsym (void *handle, const char *name, const char *version_str)
+{
+  return __dlvsym_internal (handle, name, version_str, RETURN_ADDRESS (0));
+}
+weak_alias (__dlvsym, dlvsym)
+# endif
+
 struct dlvsym_args
 {
   /* The arguments to dlvsym_doit.  */
@@ -54,18 +63,18 @@  dlvsym_doit (void *a)
 }
 
 void *
-__dlvsym (void *handle, const char *name, const char *version_str
-	  DL_CALLER_DECL)
+__dlvsym_internal (void *handle, const char *name, const char *version_str,
+	  void *dl_caller)
 {
 # ifdef SHARED
   if (!rtld_active ())
-    return _dlfcn_hook->dlvsym (handle, name, version_str, DL_CALLER);
+    return _dlfcn_hook->dlvsym (handle, name, version_str, dl_caller);
 # endif
 
   struct dlvsym_args args;
   args.handle = handle;
   args.name = name;
-  args.who = DL_CALLER;
+  args.who = dl_caller;
   args.version = version_str;
 
   /* Protect against concurrent loads and unloads.  */
@@ -77,7 +86,4 @@  __dlvsym (void *handle, const char *name, const char *version_str
 
   return result;
 }
-# ifdef SHARED
-weak_alias (__dlvsym, dlvsym)
-# endif
 #endif
diff --git a/include/dlfcn.h b/include/dlfcn.h
index 93dd369ab1..3eeb6b556b 100644
--- a/include/dlfcn.h
+++ b/include/dlfcn.h
@@ -131,10 +131,14 @@  extern void *__dlmopen (Lmid_t nsid, const char *file, int mode DL_CALLER_DECL)
      attribute_hidden;
 extern int __dlclose (void *handle)
      attribute_hidden;
-extern void *__dlsym (void *handle, const char *name DL_CALLER_DECL)
+extern void *__dlsym (void *handle, const char *name)
      attribute_hidden;
-extern void *__dlvsym (void *handle, const char *name, const char *version
-		       DL_CALLER_DECL)
+extern void *__dlsym_internal (void *handle, const char *name, void *dl_caller)
+     attribute_hidden;
+extern void *__dlvsym (void *handle, const char *name, const char *version)
+     attribute_hidden;
+extern void *__dlvsym_internal (void *handle, const char *name,
+				const char *version, void *dl_caller)
      attribute_hidden;
 extern char *__dlerror (void)
      attribute_hidden;
diff --git a/sysdeps/ieee754/ldbl-128ibm-compat/Makefile b/sysdeps/ieee754/ldbl-128ibm-compat/Makefile
index 7b42746c34..9abe1d7e2a 100644
--- a/sysdeps/ieee754/ldbl-128ibm-compat/Makefile
+++ b/sysdeps/ieee754/ldbl-128ibm-compat/Makefile
@@ -344,3 +344,9 @@  obj-suf-foreach = $(foreach suf,$(all-object-suffixes),$(subst ^,$(suf),$(1)))
 # Explicitly add -mabi=ibm-long-double to required files.
 $(call obj-suf-foreach,$(ldbl-ibm128-files)): \
   sysdep-CFLAGS += -mabi=ibmlongdouble
+
+ifeq ($(subdir),dlfcn)
+  libdl-routines += ieee128-dlfcn
+  CFLAGS-dlsym.c += -mabi=ibmlongdouble
+  CFLAGS-dlvsym.c += -mabi=ibmlongdouble
+endif
diff --git a/sysdeps/ieee754/ldbl-128ibm-compat/Versions b/sysdeps/ieee754/ldbl-128ibm-compat/Versions
index 07e6a5b08a..fe644f79f4 100644
--- a/sysdeps/ieee754/ldbl-128ibm-compat/Versions
+++ b/sysdeps/ieee754/ldbl-128ibm-compat/Versions
@@ -245,3 +245,10 @@  libc {
     __qfcvtieee128_r;
   }
 }
+
+libdl {
+  LDBL_IBM128_VERSION {
+    __dlsymieee128;
+    __dlvsymieee128;
+  }
+}
diff --git a/sysdeps/ieee754/ldbl-128ibm-compat/ieee128-dlfcn.c b/sysdeps/ieee754/ldbl-128ibm-compat/ieee128-dlfcn.c
new file mode 100644
index 0000000000..8b06c42cf7
--- /dev/null
+++ b/sysdeps/ieee754/ldbl-128ibm-compat/ieee128-dlfcn.c
@@ -0,0 +1,33 @@ 
+/* Look up a versioned symbol in a shared object loaded by `dlopen'
+   with name mangling for ieee128 redirects.
+
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <dlfcn.h>
+
+void *
+__dlsymieee128 (void *handle, const char *name)
+{
+  return __dlsym_internal (handle, name, RETURN_ADDRESS (0));
+}
+
+void *
+__dlvsymieee128 (void *handle, const char *name, const char *version_str)
+{
+  return __dlvsym_internal (handle, name, version_str, RETURN_ADDRESS (0));
+}
diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libdl.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libdl.abilist
index 36340d5dd8..a95c377a24 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libdl.abilist
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libdl.abilist
@@ -7,3 +7,5 @@  GLIBC_2.17 dlmopen F
 GLIBC_2.17 dlopen F
 GLIBC_2.17 dlsym F
 GLIBC_2.17 dlvsym F
+GLIBC_2.32 __dlsymieee128 F
+GLIBC_2.32 __dlvsymieee128 F