[2.25] powerpc: Avoid calling strncmp via PLT on GCC 7

  This patch only fixes the issue on elf/check-localplt.
In case you notice any other test failures when comparing strings on GCC
7, please refer to this patch:
https://gcc.gnu.org/ml/gcc-patches/2017-01/msg00744.html

-- 8< --

GCC 7 added support for a strncmp built-in for POWER7, generating PLT calls
to strncmp from libc.

2017-01-16  Tulio Magno Quites Machado Filho  <tuliom@linux.vnet.ibm.com>

	* sysdeps/powerpc/symbol-hacks.h: New file.  Enforce strncmp
	calls don't go through the PLT.
---
 sysdeps/powerpc/symbol-hacks.h | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 sysdeps/powerpc/symbol-hacks.h
  

On Mon, 2017-01-16 at 11:29 -0200, Tulio Magno Quites Machado Filho
wrote:
> This patch only fixes the issue on elf/check-localplt.
> In case you notice any other test failures when comparing strings on
> GCC
> 7, please refer to this patch:
> https://gcc.gnu.org/ml/gcc-patches/2017-01/msg00744.html

Tulio,
  I'll be posting an updated version of that patch shortly that
addresses the issues you were seeing. However it adds builtin expansion
of both strncmp and strcmp so I think your patch needs to add both to
powerpc/symbol-hacks.h. Here is what I get from check-localplt with
gcc7 plus this patch:

cat /home/sawdey/src/glibc/build/elf/check-localplt.out
Extra PLT reference: libc.so: strcmp
Extra PLT reference: libc.so: strncmp

Thanks, 
   Aaron

> 
> -- 8< --
> 
> GCC 7 added support for a strncmp built-in for POWER7, generating PLT
> calls
> to strncmp from libc.
> 
> 2017-01-16  Tulio Magno Quites Machado Filho  <tuliom@linux.vnet.ibm.
> com>
> 
> 	* sysdeps/powerpc/symbol-hacks.h: New file.  Enforce strncmp
> 	calls don't go through the PLT.
> ---
>  sysdeps/powerpc/symbol-hacks.h | 7 +++++++
>  1 file changed, 7 insertions(+)
>  create mode 100644 sysdeps/powerpc/symbol-hacks.h
> 
> diff --git a/sysdeps/powerpc/symbol-hacks.h b/sysdeps/powerpc/symbol-
> hacks.h
> new file mode 100644
> index 0000000..7558b18
> --- /dev/null
> +++ b/sysdeps/powerpc/symbol-hacks.h
> @@ -0,0 +1,7 @@
> +#include <sysdeps/generic/symbol-hacks.h>
> +
> +/* GCC 7.0 added support for a builtin strncmp that is used on POWER
> >= 7.  */
> +#if !defined __ASSEMBLER__ && IS_IN (libc) && defined SHARED \
> +    && defined _ARCH_PWR7
> +asm ("strncmp = __GI_strncmp");
> +#endif
  

* Tulio Magno Quites Machado Filho:

> +/* GCC 7.0 added support for a builtin strncmp that is used on POWER >= 7.  */
> +#if !defined __ASSEMBLER__ && IS_IN (libc) && defined SHARED \
> +    && defined _ARCH_PWR7
> +asm ("strncmp = __GI_strncmp");
> +#endif

Would it be safe to do this unconditionally, in the generic header?
  

On Mon, 16 Jan 2017, Aaron Sawdey wrote:

> Tulio,
>   I'll be posting an updated version of that patch shortly that
> addresses the issues you were seeing. However it adds builtin expansion
> of both strncmp and strcmp so I think your patch needs to add both to
> powerpc/symbol-hacks.h. Here is what I get from check-localplt with
> gcc7 plus this patch:
> 
> cat /home/sawdey/src/glibc/build/elf/check-localplt.out
> Extra PLT reference: libc.so: strcmp
> Extra PLT reference: libc.so: strncmp

On further consideration: I don't think addressing this in GCC should be 
hard, can't you just look up something like DECL_ASSEMBLER_NAME 
(builtin_decl_explicit (BUILT_IN_STRNCMP)) or similar and use that instead 
of hardcoded strncmp when generating code?
  

* Joseph Myers:

> On Mon, 16 Jan 2017, Aaron Sawdey wrote:
>
>> Tulio,
>>   I'll be posting an updated version of that patch shortly that
>> addresses the issues you were seeing. However it adds builtin expansion
>> of both strncmp and strcmp so I think your patch needs to add both to
>> powerpc/symbol-hacks.h. Here is what I get from check-localplt with
>> gcc7 plus this patch:
>> 
>> cat /home/sawdey/src/glibc/build/elf/check-localplt.out
>> Extra PLT reference: libc.so: strcmp
>> Extra PLT reference: libc.so: strncmp
>
> On further consideration: I don't think addressing this in GCC should be 
> hard, can't you just look up something like DECL_ASSEMBLER_NAME 
> (builtin_decl_explicit (BUILT_IN_STRNCMP)) or similar and use that instead 
> of hardcoded strncmp when generating code?

Right.  I forgot that GCC does exactly this for other builtins.  We
even have a configure check:

AC_CACHE_CHECK(for redirection of built-in functions, libc_cv_gcc_builtin_redirection, [dnl
cat > conftest.c <<\EOF
extern char *strstr (const char *, const char *) __asm ("my_strstr");
char *foo (const char *a, const char *b)
{
  return __builtin_strstr (a, b);
}
EOF
dnl
if AC_TRY_COMMAND([${CC-cc} -O3 -S conftest.c -o - | fgrep "my_strstr" > /dev/null]);
then
  libc_cv_gcc_builtin_redirection=yes
else
  libc_cv_gcc_builtin_redirection=no
fi
rm -f conftest* ])
if test "$libc_cv_gcc_builtin_redirection" = no; then
  AC_MSG_ERROR([support for the symbol redirection needed])
fi

So you are right that this should be fixed in GCC.
  

On Mon, 2017-01-16 at 23:58 +0100, Florian Weimer wrote:
> * Joseph Myers:
> 
> > On Mon, 16 Jan 2017, Aaron Sawdey wrote:
> > 
> > > Tulio,
> > >   I'll be posting an updated version of that patch shortly that
> > > addresses the issues you were seeing. However it adds builtin
> > > expansion
> > > of both strncmp and strcmp so I think your patch needs to add
> > > both to
> > > powerpc/symbol-hacks.h. Here is what I get from check-localplt
> > > with
> > > gcc7 plus this patch:
> > > 
> > > cat /home/sawdey/src/glibc/build/elf/check-localplt.out
> > > Extra PLT reference: libc.so: strcmp
> > > Extra PLT reference: libc.so: strncmp
> > 
> > On further consideration: I don't think addressing this in GCC
> > should be 
> > hard, can't you just look up something like DECL_ASSEMBLER_NAME 
> > (builtin_decl_explicit (BUILT_IN_STRNCMP)) or similar and use that
> > instead 
> > of hardcoded strncmp when generating code?

I really don't see how I'm supposed to discover that I need to use
__GI_strncmp in this instance. For example when I compile
misc/getttyent.c with this code in the expansion function:

printf("ASM: %s\n", IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME
(builtin_decl_explicit (BUILT_IN_STRNCMP))));

I get "ASM: strncmp". This is one of the places that is producing a plt
call to strncmp. The preprocessed source has this declaration for
strncmp:

extern int strncmp (const char *__s1, const char *__s2, size_t __n)
     __attribute__ ((__nothrow__ )) __attribute__ ((__pure__)) ;

I don't really see how gcc is suppose to be able to find this
__GI_strncmp name here.

> 
> Right.  I forgot that GCC does exactly this for other builtins.  We
> even have a configure check:
> 
> AC_CACHE_CHECK(for redirection of built-in functions,
> libc_cv_gcc_builtin_redirection, [dnl
> cat > conftest.c <<\EOF
> extern char *strstr (const char *, const char *) __asm ("my_strstr");
> char *foo (const char *a, const char *b)
> {
>   return __builtin_strstr (a, b);
> }
> EOF
> dnl
> if AC_TRY_COMMAND([${CC-cc} -O3 -S conftest.c -o - | fgrep
> "my_strstr" > /dev/null]);
> then
>   libc_cv_gcc_builtin_redirection=yes
> else
>   libc_cv_gcc_builtin_redirection=no
> fi
> rm -f conftest* ])
> if test "$libc_cv_gcc_builtin_redirection" = no; then
>   AC_MSG_ERROR([support for the symbol redirection needed])
> fi

This test case does work for strncmp:

extern int strncmp (const char *, const char *, unsigned long) __asm
("my_strncmp");
int foo (const char *a, const char *b)
{
  return __builtin_strncmp (a, b, 10);
}

In this case I get "ASM: *my_strncmp" from the debug code in the
compiler when it expands strncmp. But the preprocessed code for
getttyent.c does not have an __asm directive to tell me this.

> 
> So you are right that this should be fixed in GCC.

I'm starting to believe this is possible but I think the glibc code
doesn't give all the information needed for this yet. Also I can't
quite see why symbol-hacks.h redefinitions of memcmp/memmov/memset is
needed unless the compiler is not looking up this asm name elsewhere.

  Aaron
  

On Mon, 16 Jan 2017, Aaron Sawdey wrote:

> In this case I get "ASM: *my_strncmp" from the debug code in the
> compiler when it expands strncmp. But the preprocessed code for
> getttyent.c does not have an __asm directive to tell me this.

It should have such a directive, resulting from the

libc_hidden_builtin_proto (strncmp)

in include/string.h (that is, there should be a first strncmp declaration 
without the asm, and a later one with the asm).

> I'm starting to believe this is possible but I think the glibc code
> doesn't give all the information needed for this yet. Also I can't
> quite see why symbol-hacks.h redefinitions of memcmp/memmov/memset is
> needed unless the compiler is not looking up this asm name elsewhere.

Those are generated without reference to a built-in function at all, which 
is different from the case where a file actually calls strncmp.
  

Florian Weimer <fw@deneb.enyo.de> writes:

> [ text/plain ]
> * Tulio Magno Quites Machado Filho:
>
>> +/* GCC 7.0 added support for a builtin strncmp that is used on POWER >= 7.  */
>> +#if !defined __ASSEMBLER__ && IS_IN (libc) && defined SHARED \
>> +    && defined _ARCH_PWR7
>> +asm ("strncmp = __GI_strncmp");
>> +#endif
>
> Would it be safe to do this unconditionally, in the generic header?

Do you mean without _ARCH_PWR7?
If so, I think that's possible and in the generic header.

If this issue won't be fixed in GCC, I'll make that change here.