[v2] Remove defines to builtins in string2.h
Commit Message
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.
--
Comments
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
@@ -58,45 +58,6 @@
#endif
-#ifndef _HAVE_STRING_ARCH_strchr
-# define strchr(s, c) __builtin_strchr (s, c)
-#endif
-
-
-/* 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
-
-
-/* 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
-
-
-/* Append no more than N characters from SRC onto DEST. */
-#ifndef _HAVE_STRING_ARCH_strncat
-# ifdef _USE_STRING_ARCH_strchr
-# define strncat(dest, src, n) \
- (__extension__ ({ char *__dest = (dest); \
- __builtin_constant_p (src) && __builtin_constant_p (n) \
- ? (strlen (src) < ((size_t) (n)) \
- ? strcat (__dest, src) \
- : (*((char *) __mempcpy (strchr (__dest, '\0'), \
- src, n)) = '\0', __dest)) \
- : strncat (dest, src, n); }))
-# else
-# define strncat(dest, src, n) __builtin_strncat (dest, src, n)
-# endif
-#endif
-
-
/* Compare characters of S1 and S2. */
#ifndef _HAVE_STRING_ARCH_strcmp
# define strcmp(s1, s2) \
@@ -155,32 +116,6 @@
#endif
-/* Return the length of the initial segment of S which
- consists entirely of characters not in REJECT. */
-#ifndef _HAVE_STRING_ARCH_strcspn
-# define strcspn(s, reject) __builtin_strcspn (s, reject)
-#endif
-
-
-/* Return the length of the initial segment of S which
- consists entirely of characters in ACCEPT. */
-#ifndef _HAVE_STRING_ARCH_strspn
-# define strspn(s, accept) __builtin_strspn (s, accept)
-#endif
-
-
-/* Find the first occurrence in S of any character in ACCEPT. */
-#ifndef _HAVE_STRING_ARCH_strpbrk
-# define strpbrk(s, accept) __builtin_strpbrk (s, accept)
-#endif
-
-
-#if !defined _HAVE_STRING_ARCH_strsep
-# ifdef __USE_MISC
-# define strsep(s, reject) __strsep (s, reject)
-# endif
-#endif
-
/* We need the memory allocation functions for inline strdup().
Referring to stdlib.h (even minimally) is not allowed
in any of the tight standards compliant modes. */