[1/2] Only support ifunc in nptl/pt-vfork.c
diff mbox

Message ID 1400886936-16338-2-git-send-email-rth@twiddle.net
State New
Headers show

Commit Message

Richard Henderson May 23, 2014, 11:15 p.m. UTC
* nptl/pt-vfork.c: Error if !HAVE_IFUNC.
	[!HAVE_IFUNC] (vfork_compat): Remove.
	[!HAVE_IFUNC] (DEFINE_VFORK): Remove.
---
 nptl/pt-vfork.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

Comments

Roland McGrath May 24, 2014, 2:13 a.m. UTC | #1
> 	* nptl/pt-vfork.c: Error if !HAVE_IFUNC.
> 	[!HAVE_IFUNC] (vfork_compat): Remove.
> 	[!HAVE_IFUNC] (DEFINE_VFORK): Remove.

Yeah, but then you added it right back for aarch64.  I don't know what
about aarch64 makes you certain that the compiler will always do a proper
tail call there.  If it's something like that GCC is the only compiler for
that machine and GCC support for the machine only exists in GCC versions
that you're positive always do a proper tail call for this case at -O!=0
(since we don't support compiling libc with -O0 anyway) even with explicit
-fno-omit-frame-pointer (which people building libc are in fact free to
do), then similar facts are likely to exist about some other machines too.
So we might as well keep the shared code rather than having people start
copying and pasting your aarch64 version.  For paranoia about a compiler
that fails to do a proper tail call for a trivial argumentless function
tail-calling another argumentless function at -O1 (are there really any
such compilers that could compile libc?), we could make machines explicitly
define a macro to say it's OK or something like that.

> +/* Note! If the architecture doesn't support IFUNC, then we need an
> +   alternate target-specific mechanism to implement this.  So we just
> +   assume IFUNC here and require that the target override this file
> +   if necessary.  */
> +
> +#if !HAVE_IFUNC
> +# error
> +#endif

I suppose it doesn't much matter, but my preference is to always have some
text in an #error or #warning.  The nearby comment can be longer and of
course is what provides the real context.  But something like:

	# error must write pt-vfork for this machine or get IFUNC support!

makes it plain even to someone who looks at the build log without
consulting the source.


Thanks,
Roland
Richard Henderson May 24, 2014, 2:27 a.m. UTC | #2
On 05/23/2014 07:13 PM, Roland McGrath wrote:
>> 	* nptl/pt-vfork.c: Error if !HAVE_IFUNC.
>> 	[!HAVE_IFUNC] (vfork_compat): Remove.
>> 	[!HAVE_IFUNC] (DEFINE_VFORK): Remove.
> 
> Yeah, but then you added it right back for aarch64.  I don't know what
> about aarch64 makes you certain that the compiler will always do a proper
> tail call there.  If it's something like that GCC is the only compiler for
> that machine and GCC support for the machine only exists in GCC versions
> that you're positive always do a proper tail call for this case at -O!=0
> (since we don't support compiling libc with -O0 anyway) even with explicit
> -fno-omit-frame-pointer (which people building libc are in fact free to
> do), then similar facts are likely to exist about some other machines too.

For aarch64, gcc must be >= 4.8, since the port is quite new.

The target does not have a got/toc/gp type register that must be preserved in
some way (unlike e.g. alpha, mips, ppc), so the compiler can (and does) emit a
normal tail-call to the plt entry at -O1.

I did initially write this as an asm(""), but then I thought...

> So we might as well keep the shared code rather than having people start
> copying and pasting your aarch64 version.

... someone could make an informed decision to share the code via

#include <sysdeps/unix/sysv/linux/aarch64/pt-vfork.c>

instead of having it in the nptl/pt-vfork.c where a port could get in trouble
by default.

>> +#if !HAVE_IFUNC
>> +# error
>> +#endif
> 
> I suppose it doesn't much matter, but my preference is to always have some
> text in an #error or #warning.  The nearby comment can be longer and of
> course is what provides the real context.  But something like:
> 
> 	# error must write pt-vfork for this machine or get IFUNC support!
> 
> makes it plain even to someone who looks at the build log without
> consulting the source.

Fair enough.


r~
Roland McGrath May 24, 2014, 2:33 a.m. UTC | #3
> ... someone could make an informed decision to share the code via
> 
> #include <sysdeps/unix/sysv/linux/aarch64/pt-vfork.c>
> 
> instead of having it in the nptl/pt-vfork.c where a port could get in trouble
> by default.

Agreed.  That seems simpler than more internal flag macros.  I don't know
why I didn't think of that.  Make sure the longer comment in pt-vfork.c
that one sees for the !HAVE_IFUNC #error suggests this explicitly.


Thanks,
Roland

Patch
diff mbox

diff --git a/nptl/pt-vfork.c b/nptl/pt-vfork.c
index 81d1b71..8beb121 100644
--- a/nptl/pt-vfork.c
+++ b/nptl/pt-vfork.c
@@ -28,13 +28,20 @@ 
    vfork symbols in libpthread.so; so we define them using IFUNC to
    redirect to the libc function.  */
 
+/* Note! If the architecture doesn't support IFUNC, then we need an
+   alternate target-specific mechanism to implement this.  So we just
+   assume IFUNC here and require that the target override this file
+   if necessary.  */
+
+#if !HAVE_IFUNC
+# error
+#endif
+
 #if (SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_20) \
      || SHLIB_COMPAT (libpthread, GLIBC_2_1_2, GLIBC_2_20))
 
 extern __typeof (vfork) __libc_vfork;   /* Defined in libc.  */
 
-# ifdef HAVE_IFUNC
-
 attribute_hidden __attribute__ ((used))
 __typeof (vfork) *
 vfork_ifunc (void)
@@ -42,29 +49,16 @@  vfork_ifunc (void)
   return &__libc_vfork;
 }
 
-#  ifdef HAVE_ASM_SET_DIRECTIVE
-#   define DEFINE_VFORK(name) \
+# ifdef HAVE_ASM_SET_DIRECTIVE
+#  define DEFINE_VFORK(name) \
   asm (".set " #name ", vfork_ifunc\n" \
        ".globl " #name "\n" \
        ".type " #name ", %gnu_indirect_function");
-#  else
-#   define DEFINE_VFORK(name) \
+# else
+#  define DEFINE_VFORK(name) \
   asm (#name " = vfork_ifunc\n" \
        ".globl " #name "\n" \
        ".type " #name ", %gnu_indirect_function");
-#  endif
-
-# else
-
-attribute_hidden
-pid_t
-vfork_compat (void)
-{
-  return __libc_vfork ();
-}
-
-# define DEFINE_VFORK(name)     weak_alias (vfork_compat, name)
-
 # endif
 #endif