Message ID | 1400886936-16338-2-git-send-email-rth@twiddle.net |
---|---|
State | New |
Headers | show |
> * 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
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~
> ... 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
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