[v2] Remove defines to builtins in string2.h

  As discussed in https://sourceware.org/ml/libc-alpha/2016-11/msg00625.html,
for C90 functions there is no need to use define to use the GCC builtin as
an optimization.  Also other headers already ensure (most) non-C90 symbols
are redirected to avoid name space clashes.

So remove the defines for strchr, __stpcpy, stpcpy, strncpy, strncat, strcspn,
strspn, strpbrk and strsep from string/bits/string2.h. The strncat define only
uses the #else as _USE_STRING_ARCH_strchr is never defined unless
_HAVE_STRING_ARCH_strncat is defined too.

elf/check-localplt and linknamespace test pass so the redirections weren't
required or useful.

ChangeLog:
2015-11-22  Wilco Dijkstra  <wdijkstr@arm.com>

	* string/bits/string2.h (strchr) Remove define.
	(stpcpy): Likewise.
	(__stpcpy): Likewise.
	(strncpy): Likewise.
	(strncat): Likewise.
	(strcspn): Likewise.
	(strspn): Likewise.
	(strpbrk): Likewise.
	(strsep): Likewise.
 --
  

On 11/22/2016 03:35 PM, Wilco Dijkstra wrote:
> As discussed in https://sourceware.org/ml/libc-alpha/2016-11/msg00625.html,
> for C90 functions there is no need to use define to use the GCC builtin as
> an optimization.  Also other headers already ensure (most) non-C90 symbols
> are redirected to avoid name space clashes.

> -#ifndef _HAVE_STRING_ARCH_strchr
> -# define strchr(s, c) __builtin_strchr (s, c)
> -#endif

Please remove the definition of _HAVE_STRING_ARCH_strchr as well (from 
sysdeps/x86/bits/string.h).

> -/* Copy SRC to DEST, returning pointer to final NUL byte.  */
> -#ifdef __USE_GNU
> -# ifndef _HAVE_STRING_ARCH_stpcpy
> -#  define __stpcpy(dest, src) __builtin_stpcpy (dest, src)
> -/* In glibc we use this function frequently but for namespace reasons
> -   we have to use the name `__stpcpy'.  */
> -#  define stpcpy(dest, src) __stpcpy (dest, src)
> -# endif
> -#endif

I'm surprised to see stpcpy in this patch because the first paragraph 
says this change is exclusively about C90 functions, and stpcpy isn't one.

> -/* Copy no more than N characters of SRC to DEST.  */
> -#ifndef _HAVE_STRING_ARCH_strncpy
> -# define strncpy(dest, src, n) __builtin_strncpy (dest, src, n)
> -#endif

_HAVE_STRING_ARCH_strncpy needs removal.

> -#ifndef _HAVE_STRING_ARCH_strncat
> -# ifdef _USE_STRING_ARCH_strchr

These two need to be removed as well.  I'm not listing the other feature 
macros which can be removed.

> -#if !defined _HAVE_STRING_ARCH_strsep
> -# ifdef __USE_MISC
> -#  define strsep(s, reject) __strsep (s, reject)
> -# endif
> -#endif

See above, not a C90 function.

Thanks,
Florian
  

Florian Weimer wrote:
> Please remove the definition of _HAVE_STRING_ARCH_strchr as well (from 
> sysdeps/x86/bits/string.h).

Sure I'll have a quick hunt for dead defines. There must be quite a few by now...

> I'm surprised to see stpcpy in this patch because the first paragraph 
> says this change is exclusively about C90 functions, and stpcpy isn't one.

Joseph's reply to my question made it clear the redirection is done elsewhere
in most cases, so that means we can remove the redirections of non-C90
symbols from string2.h as well:

> > Also other headers already ensure (most) non-C90 symbols
> > are redirected to avoid name space clashes.

I checked and there are no namespace issues introduced by my patch
(note strdup/strndup doesn't appear to redirect internally, so building with -Os
or removing the str(n)dup defines from string2.h causes a namespace bug).

Wilco
  

On 11/22/2016 05:34 PM, Wilco Dijkstra wrote:
> Florian Weimer wrote:
>> Please remove the definition of _HAVE_STRING_ARCH_strchr as well (from
>> sysdeps/x86/bits/string.h).
>
> Sure I'll have a quick hunt for dead defines. There must be quite a few by now...

Thanks.

>> I'm surprised to see stpcpy in this patch because the first paragraph
>> says this change is exclusively about C90 functions, and stpcpy isn't one.
>
> Joseph's reply to my question made it clear the redirection is done elsewhere
> in most cases, so that means we can remove the redirections of non-C90
> symbols from string2.h as well:
>
>>> Also other headers already ensure (most) non-C90 symbols
>>> are redirected to avoid name space clashes.

Ah, I found the reference to C90 symbols confusing.

Florian
  

On Tue, Nov 22, 2016 at 11:34 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>
> I checked and there are no namespace issues introduced by my patch
> (note strdup/strndup doesn't appear to redirect internally, so building with -Os
> or removing the str(n)dup defines from string2.h causes a namespace bug).

In the "no-string-opts" branch, I noticed a problem with
strsep/__strsep as well.  I sprinkled __ around where necessary to get
things compiling again, but internal redirections would probably be
better, so code can continue using the undecorated names.

zw
  

On Tue, 22 Nov 2016, Wilco Dijkstra wrote:

> > I'm surprised to see stpcpy in this patch because the first paragraph 
> > says this change is exclusively about C90 functions, and stpcpy isn't one.
> 
> Joseph's reply to my question made it clear the redirection is done elsewhere
> in most cases, so that means we can remove the redirections of non-C90
> symbols from string2.h as well:

My suggestion for stpcpy included:

  Thus, it should be possible just to call stpcpy directly everywhere in 
  glibc rather than __stpcpy, and rely on those redirections to ensure that 
  the calls actually end up being namespace-clean (but can be inlined if 
  appropriate as GCC knows about stpcpy as a built-in function in gnu11 
  mode).  Then the definition of __stpcpy in bits/string2.h should not be 
  needed for building glibc.

Unless you also change all calls to __stpcpy to call stpcpy directly, you 
are potentially losing built-in function optimizations by removing the 
__stpcpy macro.  glibc contains calls to __stpcpy as well as direct ones 
to stpcpy.  Because of the other redirections, it's safe to change 
__stpcpy calls to stpcpy calls in glibc as a preparatory patch.  Likewise 
__mempcpy.

I think you need to separate the case of

#define foo(x) __builtin_foo (x)

from all the other cases, where a more complicated macro expansion is 
involved (in which case we need to see if GCC has relevant optimizations 
or have a reason why they are appropriate to discard), or where the macro 
is __foo instead of foo.

Also, patches like this are the sort where it would be useful to make sure 
that installed stripped shared libraries and executables are unchanged by 
the patch, or to have a reason why it results in any changes to the 
generated code.
  

Joseph Myers wrote:

> Unless you also change all calls to __stpcpy to call stpcpy directly, you 
> are potentially losing built-in function optimizations by removing the 
> __stpcpy macro.  glibc contains calls to __stpcpy as well as direct ones 
> to stpcpy.  Because of the other redirections, it's safe to change 
> __stpcpy calls to stpcpy calls in glibc as a preparatory patch.  Likewise 
> __mempcpy.

Right, it does seem there are a few cases where __stpcpy may be optimized.
So for this patch I'll keep the #define __stpcpy until existing calls to __stpcpy
have been changed.

> I think you need to separate the case of
> 
> #define foo(x) __builtin_foo (x)
> 
> from all the other cases, where a more complicated macro expansion is 
> involved (in which case we need to see if GCC has relevant optimizations 
> or have a reason why they are appropriate to discard), or where the macro 
> is __foo instead of foo.

This patch doesn't remove any optimizations - as I mentioned in the description,
the strncat macro is unused as _USE_STRING_ARCH_strchr is always false
(and in the next version I'm removing that define as well).

> Also, patches like this are the sort where it would be useful to make sure 
> that installed stripped shared libraries and executables are unchanged by 
> the patch, or to have a reason why it results in any changes to the 
> generated code.

Yes my goal was no diff, and that's true for the new version that also removes
all unused string defines.

Wilco