[GLIBC][PATCH] Remove strdup inlines

[GLIBC][PATCH] Remove strdup inlines

[Wilco Dijkstra]

Remove the str(n)dup inlines from string/bits/string2.h.  Although inlining
calls with constant strings shows a small (~10%) performance gain, strdup is
typically used in error reporting code, so not performance critical.

Avoid linknamespace and localplt failures by redirecting str(n)dup to GCC
builtins.

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

	* string/string.h (strdup): Use builtin.
	(strndup): Likewise.
	* string/bits/string2.h (__strdup): Remove define.
	(strdup): Likewise.
	(__strndup): Likewise.
	(strndup): Likewise.
--

diff --git a/string/bits/string2.h b/string/bits/string2.h
index 03af22cc27a33c81f36d56ddc52fce0a5ea81ece..b0be5f6a49024a0bedfc37e9ec2c0356e0e4fa09 100644
--- a/string/bits/string2.h
+++ b/string/bits/string2.h
@@ -118,58 +118,6 @@
 #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.  */
-#ifdef __USE_MISC
-
-# define __need_malloc_and_calloc
-# include <stdlib.h>
-
-extern char *__strdup (const char *__string) __THROW __attribute_malloc__;
-# define __strdup(s) \
-  (__extension__ (__builtin_constant_p (s) && __string2_1bptr_p (s)	      \
-		  ? (((const char *) (s))[0] == '\0'			      \
-		     ? (char *) calloc ((size_t) 1, (size_t) 1)		      \
-		     : ({ size_t __len = strlen (s) + 1;		      \
-			  char *__retval = (char *) malloc (__len);	      \
-			  if (__retval != NULL)				      \
-			    __retval = (char *) memcpy (__retval, s, __len);  \
-			  __retval; }))					      \
-		  : __strdup (s)))
-
-# if defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8
-#  define strdup(s) __strdup (s)
-# endif
-
-
-extern char *__strndup (const char *__string, size_t __n)
-     __THROW __attribute_malloc__;
-# define __strndup(s, n) \
-  (__extension__ (__builtin_constant_p (s) && __string2_1bptr_p (s)	      \
-		  ? (((const char *) (s))[0] == '\0'			      \
-		     ? (char *) calloc ((size_t) 1, (size_t) 1)		      \
-		     : ({ size_t __len = strlen (s) + 1;		      \
-			  size_t __n = (n);				      \
-			  char *__retval;				      \
-			  if (__n < __len)				      \
-			    __len = __n + 1;				      \
-			  __retval = (char *) malloc (__len);		      \
-			  if (__retval != NULL)				      \
-			    {						      \
-			      __retval[__len - 1] = '\0';		      \
-			      __retval = (char *) memcpy (__retval, s,	      \
-							  __len - 1);	      \
-			    }						      \
-			  __retval; }))					      \
-		  : __strndup (s, n)))
-
-# ifdef __USE_XOPEN2K8
-#  define strndup(s, n) __strndup (s, n)
-# endif
-
-#endif /* Use misc. or use GNU.  */
-
 #ifndef _FORCE_INLINES
 # undef __STRING_INLINE
 #endif
diff --git a/string/string.h b/string/string.h
index b103e64912fe1904098e229ccb845bb2c5c10835..c251cc603a1dbb5bef469d4b71f90037612d36f0 100644
--- a/string/string.h
+++ b/string/string.h
@@ -636,6 +636,15 @@ extern char *basename (const char *__filename) __THROW __nonnull ((1));
 # endif
 #endif
 
+#if (defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8     \
+     || __GLIBC_USE (LIB_EXT2))
+# define strdup(s) __builtin_strdup (s)
+#endif
+
+#if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
+# define strndup(s,n) __builtin_strndup (s, n)
+#endif
+
 #if defined __USE_GNU && defined __OPTIMIZE__ \
     && defined __extern_always_inline && __GNUC_PREREQ (3,2)
 # if !defined _FORCE_INLINES && !defined _HAVE_STRING_ARCH_mempcpy

Re: [GLIBC][PATCH] Remove strdup inlines

[Zack Weinberg]

On Mon, Dec 12, 2016 at 7:00 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> Remove the str(n)dup inlines from string/bits/string2.h.  Although inlining
> calls with constant strings shows a small (~10%) performance gain, strdup is
> typically used in error reporting code, so not performance critical.

Looks good to me.

This is the sole user of __need_malloc_and_calloc, so please also remove the
logic implementing that from {include,stdlib}/stdlib.h.

zw

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Wilco Dijkstra]

Zack Weinberg wrote:
> On Mon, Dec 12, 2016 at 7:00 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> > Remove the str(n)dup inlines from string/bits/string2.h.  Although inlining
> > calls with constant strings shows a small (~10%) performance gain, strdup is
> > typically used in error reporting code, so not performance critical.
> 
> Looks good to me.
> 
> This is the sole user of __need_malloc_and_calloc, so please also remove the
> logic implementing that from {include,stdlib}/stdlib.h.

Good point, I've removed those defines too in v2:


Remove the str(n)dup inlines from string/bits/string2.h.  Although inlining
calls with constant strings shows a small (~10%) performance gain, strdup is
typically used in error reporting code, so not performance critical.
Remove the now unused __need_malloc_and_calloc related defines from stdlib.h.

Avoid linknamespace and localplt failures by redirecting str(n)dup to GCC
builtins.

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


	* include/stdlib.h (__need_malloc_and_calloc): Remove uses.
	(__Need_M_And_C) Remove define/undef.
	* stdlib/stdlib.h (__need_malloc_and_calloc): Remove uses.
	(__malloc_and_calloc_defined): Remove define.
	* string/string.h (strdup): Use builtin.
	(strndup): Likewise.
	* string/bits/string2.h (__strdup): Remove define.
	(strdup): Likewise.
	(__strndup): Likewise.
	(strndup): Likewise.

--
diff --git a/include/stdlib.h b/include/stdlib.h
index 352339e8595eb8229018cb27f7d2decf63f511c7..929cead59ae10afe03ae1286b72d8321f0ab2d90 100644
--- a/include/stdlib.h
+++ b/include/stdlib.h
@@ -1,16 +1,12 @@
 #ifndef _STDLIB_H
 
-#ifdef __need_malloc_and_calloc
-#define __Need_M_And_C
-#endif
-
 #ifndef _ISOMAC
 # include <stddef.h>
 #endif
 #include <stdlib/stdlib.h>
 
 /* Now define the internal interfaces.  */
-#if !defined __Need_M_And_C && !defined _ISOMAC
+#if !defined _ISOMAC
 # include <sys/stat.h>
 
 __BEGIN_DECLS
@@ -269,6 +265,4 @@ __END_DECLS
 
 #endif
 
-#undef __Need_M_And_C
-
 #endif  /* include/stdlib.h */
diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
index 48f9a9500e736a5d85e5eca2e511cff374e43226..b1696c7a8d82145f76760a49909e24c288c51263 100644
--- a/stdlib/stdlib.h
+++ b/stdlib/stdlib.h
@@ -26,15 +26,12 @@
 
 /* Get size_t, wchar_t and NULL from <stddef.h>.  */
 #define		__need_size_t
-#ifndef __need_malloc_and_calloc
-# define	__need_wchar_t
-# define	__need_NULL
-#endif
+#define		__need_wchar_t
+#define		__need_NULL
 #include <stddef.h>
 
 __BEGIN_DECLS
 
-#ifndef __need_malloc_and_calloc
 #define	_STDLIB_H	1
 
 #if (defined __USE_XOPEN || defined __USE_XOPEN2K8) && !defined _SYS_WAIT_H
@@ -434,10 +431,6 @@ extern int lcong48_r (unsigned short int __param[7],
 # endif	/* Use misc.  */
 #endif	/* Use misc or X/Open.  */
 
-#endif /* don't just need malloc and calloc */
-
-#ifndef __malloc_and_calloc_defined
-# define __malloc_and_calloc_defined
 __BEGIN_NAMESPACE_STD
 /* Allocate SIZE bytes of memory.  */
 extern void *malloc (size_t __size) __THROW __attribute_malloc__ __wur;
@@ -445,9 +438,7 @@ extern void *malloc (size_t __size) __THROW __attribute_malloc__ __wur;
 extern void *calloc (size_t __nmemb, size_t __size)
      __THROW __attribute_malloc__ __wur;
 __END_NAMESPACE_STD
-#endif
 
-#ifndef __need_malloc_and_calloc
 __BEGIN_NAMESPACE_STD
 /* Re-allocate the previously allocated block
    in PTR, making the new block SIZE bytes long.  */
@@ -944,9 +935,6 @@ extern int ttyslot (void) __THROW;
 # include <bits/stdlib-ldbl.h>
 #endif
 
-#endif /* don't just need malloc and calloc */
-#undef __need_malloc_and_calloc
-
 __END_DECLS
 
 #endif /* stdlib.h  */
diff --git a/string/bits/string2.h b/string/bits/string2.h
index 03af22cc27a33c81f36d56ddc52fce0a5ea81ece..b0be5f6a49024a0bedfc37e9ec2c0356e0e4fa09 100644
--- a/string/bits/string2.h
+++ b/string/bits/string2.h
@@ -118,58 +118,6 @@
 #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.  */
-#ifdef __USE_MISC
-
-# define __need_malloc_and_calloc
-# include <stdlib.h>
-
-extern char *__strdup (const char *__string) __THROW __attribute_malloc__;
-# define __strdup(s) \
-  (__extension__ (__builtin_constant_p (s) && __string2_1bptr_p (s)	      \
-		  ? (((const char *) (s))[0] == '\0'			      \
-		     ? (char *) calloc ((size_t) 1, (size_t) 1)		      \
-		     : ({ size_t __len = strlen (s) + 1;		      \
-			  char *__retval = (char *) malloc (__len);	      \
-			  if (__retval != NULL)				      \
-			    __retval = (char *) memcpy (__retval, s, __len);  \
-			  __retval; }))					      \
-		  : __strdup (s)))
-
-# if defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8
-#  define strdup(s) __strdup (s)
-# endif
-
-
-extern char *__strndup (const char *__string, size_t __n)
-     __THROW __attribute_malloc__;
-# define __strndup(s, n) \
-  (__extension__ (__builtin_constant_p (s) && __string2_1bptr_p (s)	      \
-		  ? (((const char *) (s))[0] == '\0'			      \
-		     ? (char *) calloc ((size_t) 1, (size_t) 1)		      \
-		     : ({ size_t __len = strlen (s) + 1;		      \
-			  size_t __n = (n);				      \
-			  char *__retval;				      \
-			  if (__n < __len)				      \
-			    __len = __n + 1;				      \
-			  __retval = (char *) malloc (__len);		      \
-			  if (__retval != NULL)				      \
-			    {						      \
-			      __retval[__len - 1] = '\0';		      \
-			      __retval = (char *) memcpy (__retval, s,	      \
-							  __len - 1);	      \
-			    }						      \
-			  __retval; }))					      \
-		  : __strndup (s, n)))
-
-# ifdef __USE_XOPEN2K8
-#  define strndup(s, n) __strndup (s, n)
-# endif
-
-#endif /* Use misc. or use GNU.  */
-
 #ifndef _FORCE_INLINES
 # undef __STRING_INLINE
 #endif
diff --git a/string/string.h b/string/string.h
index b103e64912fe1904098e229ccb845bb2c5c10835..c251cc603a1dbb5bef469d4b71f90037612d36f0 100644
--- a/string/string.h
+++ b/string/string.h
@@ -636,6 +636,15 @@ extern char *basename (const char *__filename) __THROW __nonnull ((1));
 # endif
 #endif
 
+#if (defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8     \
+     || __GLIBC_USE (LIB_EXT2))
+# define strdup(s) __builtin_strdup (s)
+#endif
+
+#if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
+#define strndup(s,n) __builtin_strndup (s, n)
+#endif
+
 #if defined __USE_GNU && defined __OPTIMIZE__ \
     && defined __extern_always_inline && __GNUC_PREREQ (3,2)
 # if !defined _FORCE_INLINES && !defined _HAVE_STRING_ARCH_mempcpy

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Florian Weimer]

On 12/12/2016 02:02 PM, Wilco Dijkstra wrote:
> Remove the str(n)dup inlines from string/bits/string2.h.  Although inlining
> calls with constant strings shows a small (~10%) performance gain, strdup is
> typically used in error reporting code, so not performance critical.

Where do the performance gains come from?

Is it from IFUNC selection for strlen and memcpy, which does not happen 
in libc.so?

I don't think it's true that strdup is mostly used for error reporting. 
Why do you think so?

Thanks,
Florian

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Wilco Dijkstra]

Florian Weimer wrote:
> On 12/12/2016 02:02 PM, Wilco Dijkstra wrote:
> > Remove the str(n)dup inlines from string/bits/string2.h.  Although inlining
> > calls with constant strings shows a small (~10%) performance gain, strdup is
> > typically used in error reporting code, so not performance critical.
>
> Where do the performance gains come from?

The strlen is a constant and if small the memcpy can be expanded inline.

> Is it from IFUNC selection for strlen and memcpy, which does not happen 
> in libc.so?

These are direct calls.

> I don't think it's true that strdup is mostly used for error reporting. 
> Why do you think so?

Because strdup is not used in the hot loop of any application. strdup uses in 
GLIBC are related to error reporting and environment processing which is not
performance critical. Like strdupa and explicit use of unaligned accesses, all
this is premature micro optimization. Removing unnecessary string copies
(why is strdup of a constant string a useful idiom that should be optimized?)
or speeding up malloc would be more worthwhile.

Wilco

    

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Florian Weimer]

On 12/12/2016 04:39 PM, Wilco Dijkstra wrote:

>> I don't think it's true that strdup is mostly used for error reporting.
>> Why do you think so?
>
> Because strdup is not used in the hot loop of any application. strdup uses in
> GLIBC are related to error reporting and environment processing which is not
> performance critical. Like strdupa and explicit use of unaligned accesses, all
> this is premature micro optimization. Removing unnecessary string copies
> (why is strdup of a constant string a useful idiom that should be optimized?)
> or speeding up malloc would be more worthwhile.

Agreed.  Thanks for the explanation.

Florian

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

On 12 Dec 2016 13:02, Wilco Dijkstra wrote:
> --- a/string/string.h
> +++ b/string/string.h
>  
> +#if (defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8     \
> +     || __GLIBC_USE (LIB_EXT2))
> +# define strdup(s) __builtin_strdup (s)
> +#endif
> +
> +#if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
> +#define strndup(s,n) __builtin_strndup (s, n)
> +#endif

why do we need these at all ?  these seem like the sort of thing that
gcc should do automatically for us ?

2nd #define is missing an indent
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Wilco Dijkstra]

Mike Frysinger wrote:
> > +# define strdup(s) __builtin_strdup (s)

> why do we need these at all ?  these seem like the sort of thing that
> gcc should do automatically for us ?

strdup and strndup are used inside GLIBC and without these defines
I get lots of linknamespace and ocalplt failures. We do something similar
for __stpcpy in bits/string2.h. However here is this in include/string.h:

#if (!IS_IN (libc) || !defined SHARED) \
  && !defined NO_MEMPCPY_STPCPY_REDIRECT
/* Redirect calls to __builtin_mempcpy and __builtin_stpcpy to call
   __mempcpy and __stpcpy if not inlined.  */
extern __typeof (mempcpy) mempcpy __asm__ ("__mempcpy");
extern __typeof (stpcpy) stpcpy __asm__ ("__stpcpy");
#endif

Maybe strdup needs to be added here?

> 2nd #define is missing an indent

I'll fix that once we decide where the magic needs to go...

Wilco
    

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

On 12 Dec 2016 17:28, Wilco Dijkstra wrote:
> Mike Frysinger wrote:
> > > +# define strdup(s) __builtin_strdup (s)
> >
> > why do we need these at all ?  these seem like the sort of thing that
> > gcc should do automatically for us ?
> 
> strdup and strndup are used inside GLIBC and without these defines
> I get lots of linknamespace and ocalplt failures. We do something similar
> for __stpcpy in bits/string2.h. However here is this in include/string.h:
> 
> #if (!IS_IN (libc) || !defined SHARED) \
>   && !defined NO_MEMPCPY_STPCPY_REDIRECT
> /* Redirect calls to __builtin_mempcpy and __builtin_stpcpy to call
>    __mempcpy and __stpcpy if not inlined.  */
> extern __typeof (mempcpy) mempcpy __asm__ ("__mempcpy");
> extern __typeof (stpcpy) stpcpy __asm__ ("__stpcpy");
> #endif
> 
> Maybe strdup needs to be added here?

right -- if it's for internal needs only, include/string.h is the right
place to drop it in (with an explanation comment).  string/string.h is
the exported header and i don't think this define makes sense there.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Florian Weimer]

On 12/12/2016 07:39 PM, Mike Frysinger wrote:
> On 12 Dec 2016 17:28, Wilco Dijkstra wrote:
>> Mike Frysinger wrote:
>>>> +# define strdup(s) __builtin_strdup (s)
>>>
>>> why do we need these at all ?  these seem like the sort of thing that
>>> gcc should do automatically for us ?
>>
>> strdup and strndup are used inside GLIBC and without these defines
>> I get lots of linknamespace and ocalplt failures. We do something similar
>> for __stpcpy in bits/string2.h. However here is this in include/string.h:
>>
>> #if (!IS_IN (libc) || !defined SHARED) \
>>   && !defined NO_MEMPCPY_STPCPY_REDIRECT
>> /* Redirect calls to __builtin_mempcpy and __builtin_stpcpy to call
>>    __mempcpy and __stpcpy if not inlined.  */
>> extern __typeof (mempcpy) mempcpy __asm__ ("__mempcpy");
>> extern __typeof (stpcpy) stpcpy __asm__ ("__stpcpy");
>> #endif
>>
>> Maybe strdup needs to be added here?
>
> right -- if it's for internal needs only, include/string.h is the right
> place to drop it in (with an explanation comment).  string/string.h is
> the exported header and i don't think this define makes sense there.

Traditionally, we do not fix this up through header magic, we rewrite 
all the occurrences to use __ symbols.

I think Joseph did many such cleanups in the past.  He may have guidance 
what to do here, too.

Thanks,
Florian

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Joseph Myers]

On Mon, 12 Dec 2016, Wilco Dijkstra wrote:

> this is premature micro optimization. Removing unnecessary string copies
> (why is strdup of a constant string a useful idiom that should be optimized?)

It's useful to strdup constant strings e.g. to ensure that a string in a 
given context is always dynamically allocated, so that subsequent code can 
free it without needing to know where that particular value came from at 
runtime (if some code paths use a constant value, others use a value from 
elsewhere).  (That doesn't answer why it would be performance-relevant.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Joseph Myers]

On Mon, 12 Dec 2016, Mike Frysinger wrote:

> On 12 Dec 2016 13:02, Wilco Dijkstra wrote:
> > --- a/string/string.h
> > +++ b/string/string.h
> >  
> > +#if (defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8     \
> > +     || __GLIBC_USE (LIB_EXT2))
> > +# define strdup(s) __builtin_strdup (s)
> > +#endif
> > +
> > +#if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
> > +#define strndup(s,n) __builtin_strndup (s, n)
> > +#endif
> 
> why do we need these at all ?  these seem like the sort of thing that
> gcc should do automatically for us ?

GCC can only do that in general for C90 functions.  Consider someone 
selecting strict POSIX with -std=c99 -D_POSIX_C_SOURCE=200809L; glibc 
knows these names are reserved in that context, GCC doesn't.  (Well, for 
str* there's strictly a general C reservation, but glibc doesn't take 
advantage of it.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Joseph Myers]

On Mon, 12 Dec 2016, Florian Weimer wrote:

> > > strdup and strndup are used inside GLIBC and without these defines
> > > I get lots of linknamespace and ocalplt failures. We do something similar
> > > for __stpcpy in bits/string2.h. However here is this in include/string.h:
> > > 
> > > #if (!IS_IN (libc) || !defined SHARED) \
> > >   && !defined NO_MEMPCPY_STPCPY_REDIRECT
> > > /* Redirect calls to __builtin_mempcpy and __builtin_stpcpy to call
> > >    __mempcpy and __stpcpy if not inlined.  */
> > > extern __typeof (mempcpy) mempcpy __asm__ ("__mempcpy");
> > > extern __typeof (stpcpy) stpcpy __asm__ ("__stpcpy");
> > > #endif
> > > 
> > > Maybe strdup needs to be added here?
> > 
> > right -- if it's for internal needs only, include/string.h is the right
> > place to drop it in (with an explanation comment).  string/string.h is
> > the exported header and i don't think this define makes sense there.
> 
> Traditionally, we do not fix this up through header magic, we rewrite all the
> occurrences to use __ symbols.
> 
> I think Joseph did many such cleanups in the past.  He may have guidance what
> to do here, too.

If we want to use a built-in function (at least potentially, want GCC to 
be able to understand the meanings of the calls), that means either call 
foo instead of __foo and then remap in the headers as above, or define 
__foo to use __builtin_foo and then still need that remapping for calls 
that don't end up being inlined.  If you call __foo directly and have no 
remapping via a macro to __builtin_foo, there is no possibility of GCC 
optimizing the calls (beyond any optimization implied by attributes on 
the declaration of __foo).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Andreas Schwab]

On Dez 12 2016, Joseph Myers <joseph@codesourcery.com> wrote:

> GCC can only do that in general for C90 functions.  Consider someone 
> selecting strict POSIX with -std=c99 -D_POSIX_C_SOURCE=200809L; glibc 
> knows these names are reserved in that context, GCC doesn't.  (Well, for 
> str* there's strictly a general C reservation, but glibc doesn't take 
> advantage of it.)

That reservation is only active when the associated header is included.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Joseph Myers]

On Tue, 13 Dec 2016, Andreas Schwab wrote:

> On Dez 12 2016, Joseph Myers <joseph@codesourcery.com> wrote:
> 
> > GCC can only do that in general for C90 functions.  Consider someone 
> > selecting strict POSIX with -std=c99 -D_POSIX_C_SOURCE=200809L; glibc 
> > knows these names are reserved in that context, GCC doesn't.  (Well, for 
> > str* there's strictly a general C reservation, but glibc doesn't take 
> > advantage of it.)
> 
> That reservation is only active when the associated header is included.

No, the reservation with external linkage doesn't depend on the header 
being included.  (C90 Amendment 1 did have such a limited reservation, 
depending on header inclusion somewhere in the program, for the new 
functions it added, but not any other version of standard C.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

On 12 Dec 2016 23:13, Joseph Myers wrote:
> On Mon, 12 Dec 2016, Mike Frysinger wrote:
> > On 12 Dec 2016 13:02, Wilco Dijkstra wrote:
> > > --- a/string/string.h
> > > +++ b/string/string.h
> > >  
> > > +#if (defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8     \
> > > +     || __GLIBC_USE (LIB_EXT2))
> > > +# define strdup(s) __builtin_strdup (s)
> > > +#endif
> > > +
> > > +#if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
> > > +#define strndup(s,n) __builtin_strndup (s, n)
> > > +#endif
> > 
> > why do we need these at all ?  these seem like the sort of thing that
> > gcc should do automatically for us ?
> 
> GCC can only do that in general for C90 functions.  Consider someone 
> selecting strict POSIX with -std=c99 -D_POSIX_C_SOURCE=200809L; glibc 
> knows these names are reserved in that context, GCC doesn't.  (Well, for 
> str* there's strictly a general C reservation, but glibc doesn't take 
> advantage of it.)

we aren't doing this for many other mem/str funcs.  why should we do it
for these two ?  we should be all in, or not do any.  imo, we should just
omit them and be done unless there is strong/compelling evidence to show
otherwise.  if the only point is to support old/uncommon config combos,
then that isn't a great reason imo.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Wilco Dijkstra]

Mike Frysinger wrote:    
> we aren't doing this for many other mem/str funcs.  why should we do it
> for these two ?  we should be all in, or not do any.  imo, we should just
> omit them and be done unless there is strong/compelling evidence to show
> otherwise.  if the only point is to support old/uncommon config combos,
> then that isn't a great reason imo.

A similar redirection is done for several other functions, including mempcpy, stpcpy
and bzero. The namespace issue only exists for non-C90 functions that are used
inside GLIBC, so a small subset of all supported functions.

While it would be possible to rename all uses in GLIBC to use "__" before the
name, that then means GCC no longer optimizes them. So you still want to do
some kind of redirection to builtins so they can be optimized by GCC.

An alternative would be to rewrite uses of mempcpy, bzero, stpcpy, bzero into their
C90 equivalents - a lot of these are not strictly necessary nor good for performance.

Wilco

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 891 bytes --]

On 13 Dec 2016 09:24, Wilco Dijkstra wrote:
> Mike Frysinger wrote:    
> > we aren't doing this for many other mem/str funcs.  why should we do it
> > for these two ?  we should be all in, or not do any.  imo, we should just
> > omit them and be done unless there is strong/compelling evidence to show
> > otherwise.  if the only point is to support old/uncommon config combos,
> > then that isn't a great reason imo.
> 
> A similar redirection is done for several other functions, including mempcpy, stpcpy
> and bzero. The namespace issue only exists for non-C90 functions that are used
> inside GLIBC, so a small subset of all supported functions.

i think you're conflating those here.  if you look closely, it's for C++
code only, and it's because the signature is different (const-vs-non-const
return).  it's not for the reason you're doing a #define here.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Carlos O'Donell]

On 12/12/2016 06:11 PM, Joseph Myers wrote:
> On Mon, 12 Dec 2016, Wilco Dijkstra wrote:
> 
>> this is premature micro optimization. Removing unnecessary string copies
>> (why is strdup of a constant string a useful idiom that should be optimized?)
> 
> It's useful to strdup constant strings e.g. to ensure that a string in a 
> given context is always dynamically allocated, so that subsequent code can 
> free it without needing to know where that particular value came from at 
> runtime (if some code paths use a constant value, others use a value from 
> elsewhere).  (That doesn't answer why it would be performance-relevant.)
 
Exactly. The only relevant use of strdup I have ever added to glibc is in
the ld.so.cache processing in dlopen to ensure that reetrant dlopen calls
can rely on their cached copies being unique (strdup'd) and freeable.

-- 
Cheers,
Carlos.

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Wilco Dijkstra]

Mike Frysinger wrote:
> On 13 Dec 2016 09:24, Wilco Dijkstra wrote:
> > Mike Frysinger wrote:    
> > > we aren't doing this for many other mem/str funcs.  why should we do it
> > > for these two ?  we should be all in, or not do any.  imo, we should just
> > > omit them and be done unless there is strong/compelling evidence to show
> > > otherwise.  if the only point is to support old/uncommon config combos,
> > > then that isn't a great reason imo.
> 
> > A similar redirection is done for several other functions, including mempcpy, stpcpy
> > and bzero. The namespace issue only exists for non-C90 functions that are used
> > inside GLIBC, so a small subset of all supported functions.
>
> i think you're conflating those here.  if you look closely, it's for C++
> code only, and it's because the signature is different (const-vs-non-const
> return).  it's not for the reason you're doing a #define here.

No I'm not talking about the C++ const inlines, I mean redirects to avoid namespace
issues (and to enable GCC to optimize builtins) for non-C90 functions that are used
inside GLIBC. Eg:

#if (!IS_IN (libc) || !defined SHARED) \
  && !defined NO_MEMPCPY_STPCPY_REDIRECT
/* Redirect calls to __builtin_mempcpy and __builtin_stpcpy to call
   __mempcpy and __stpcpy if not inlined.  */
extern __typeof (mempcpy) mempcpy __asm__ ("__mempcpy");
extern __typeof (stpcpy) stpcpy __asm__ ("__stpcpy");
#endif

# ifndef _ISOMAC
#  ifndef index
#   define index(s, c)  (strchr ((s), (c)))
#  endif
#  ifndef rindex
#   define rindex(s, c) (strrchr ((s), (c)))
#  endif
# endif

# ifdef __USE_MISC
#  define strsep(s, reject) __strsep (s, reject)
# endif

#  if defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8
#   define strdup(s) __strdup (s)
#  endif

#  ifdef __USE_XOPEN2K8
#   define strndup(s, n) __strndup (s, n)
#  endif

Removing these causes linknamespace failures.

The question is what is the best approach - copy the magic for mempcpy/stpcpy (which
means you avoid having to do a whole-GLIBC rename and still get compiler builtin 
optimization), use a define that redirects to a builtin (if it exists) or GLIBC internal name?

Note there are also plenty of calls to functions with underscores, eg. __stpcpy,
__mempcpy, __bzero, so we ideally need to either rename these to use the base name
or redirect to the builtin.

Wilco

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Zack Weinberg]

On Thu, Dec 15, 2016 at 8:16 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>
> No I'm not talking about the C++ const inlines, I mean redirects to avoid namespace
> issues (and to enable GCC to optimize builtins) for non-C90 functions that are used
> inside GLIBC. Eg:
>
> #if (!IS_IN (libc) || !defined SHARED) \
>   && !defined NO_MEMPCPY_STPCPY_REDIRECT
> /* Redirect calls to __builtin_mempcpy and __builtin_stpcpy to call
>    __mempcpy and __stpcpy if not inlined.  */
> extern __typeof (mempcpy) mempcpy __asm__ ("__mempcpy");
> extern __typeof (stpcpy) stpcpy __asm__ ("__stpcpy");
> #endif

I tend to think this is the best approach, since it doesn't involve
macros and doesn't require the use of mangled names in the source
code.  However, the NO_MEMPCPY_STPCPY_REDIRECT thing is a big mess and
we might not want to have more of that.

> # ifndef _ISOMAC
> #  ifndef index
> #   define index(s, c)  (strchr ((s), (c)))
> #  endif
> #  ifndef rindex
> #   define rindex(s, c) (strrchr ((s), (c)))
> #  endif
> # endif

Arguably we shouldn't be using these legacy names at all in glibc's
source.  Maybe they should be poisoned (#pragma GCC poison).

> Note there are also plenty of calls to functions with underscores, eg. __stpcpy,
> __mempcpy, __bzero, so we ideally need to either rename these to use the base name
> or redirect to the builtin.

I'd prefer to rename to the base name, but that's just for aesthetics.

zw

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Wilco Dijkstra]

Zack Weinberg wrote:
> On Thu, Dec 15, 2016 at 8:16 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> >
> > No I'm not talking about the C++ const inlines, I mean redirects to avoid namespace
> > issues (and to enable GCC to optimize builtins) for non-C90 functions that are used
> > inside GLIBC. Eg:
> >
> > #if (!IS_IN (libc) || !defined SHARED) \
> >   && !defined NO_MEMPCPY_STPCPY_REDIRECT
> > /* Redirect calls to __builtin_mempcpy and __builtin_stpcpy to call
> >    __mempcpy and __stpcpy if not inlined.  */
> > extern __typeof (mempcpy) mempcpy __asm__ ("__mempcpy");
> > extern __typeof (stpcpy) stpcpy __asm__ ("__stpcpy");
> > #endif
> 
> I tend to think this is the best approach, since it doesn't involve
> macros and doesn't require the use of mangled names in the source
> code.  However, the NO_MEMPCPY_STPCPY_REDIRECT thing is a big mess and
> we might not want to have more of that.

Is there anything that goes wrong if you declare



> # ifndef _ISOMAC
> #  ifndef index
> #   define index(s, c)  (strchr ((s), (c)))
> #  endif
> #  ifndef rindex
> #   define rindex(s, c) (strrchr ((s), (c)))
> #  endif
> # endif

Arguably we shouldn't be using these legacy names at all in glibc's
source.  Maybe they should be poisoned (#pragma GCC poison).

> Note there are also plenty of calls to functions with underscores, eg. __stpcpy,
> __mempcpy, __bzero, so we ideally need to either rename these to use the base name
> or redirect to the builtin.

I'd prefer to rename to the base name, but that's just for aesthetics.

zw
    

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Wilco Dijkstra]

Zack Weinberg wrote:
> On Thu, Dec 15, 2016 at 8:16 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> >
> > No I'm not talking about the C++ const inlines, I mean redirects to avoid namespace
> > issues (and to enable GCC to optimize builtins) for non-C90 functions that are used
> > inside GLIBC. Eg:
> >
> > #if (!IS_IN (libc) || !defined SHARED) \
> >   && !defined NO_MEMPCPY_STPCPY_REDIRECT
> > /* Redirect calls to __builtin_mempcpy and __builtin_stpcpy to call
> >    __mempcpy and __stpcpy if not inlined.  */
> > extern __typeof (mempcpy) mempcpy __asm__ ("__mempcpy");
> > extern __typeof (stpcpy) stpcpy __asm__ ("__stpcpy");
> > #endif
> 
> I tend to think this is the best approach, since it doesn't involve
> macros and doesn't require the use of mangled names in the source
> code.  However, the NO_MEMPCPY_STPCPY_REDIRECT thing is a big mess and
> we might not want to have more of that.

Is there anything that goes wrong if you declare: 

extern __typeof (mempcpy) mempcpy __asm__ ("__mempcpy");

in string/mempcpy.c? The definition uses __mempcpy, so the only thing I can
imagine is that the weak alias/hidden def defines might get confused. If so, would this:

extern __typeof (mempcpy) mempcpy __asm__ ("mempcpy");

fix it? That would avoid NO_MEMPCPY_STPCPY_REDIRECT and make it a lot
cleaner.

> # ifndef _ISOMAC
> #  ifndef index
> #   define index(s, c)  (strchr ((s), (c)))
> #  endif
> #  ifndef rindex
> #   define rindex(s, c) (strrchr ((s), (c)))
> #  endif
> # endif

> Arguably we shouldn't be using these legacy names at all in glibc's
> source.  Maybe they should be poisoned (#pragma GCC poison).

Isn't linknamespace failures good enough? We could remove those defines and
fix resulting failures, then any new use would be flagged anyway.

> > Note there are also plenty of calls to functions with underscores, eg. __stpcpy,
> > __mempcpy, __bzero, so we ideally need to either rename these to use the base name
> > or redirect to the builtin.
>
> I'd prefer to rename to the base name, but that's just for aesthetics.

That seems best indeed.

Wilco

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]

On 15 Dec 2016 13:16, Wilco Dijkstra wrote:
> Mike Frysinger wrote:
> > On 13 Dec 2016 09:24, Wilco Dijkstra wrote:
> > > Mike Frysinger wrote:    
> > > > we aren't doing this for many other mem/str funcs.  why should we do it
> > > > for these two ?  we should be all in, or not do any.  imo, we should just
> > > > omit them and be done unless there is strong/compelling evidence to show
> > > > otherwise.  if the only point is to support old/uncommon config combos,
> > > > then that isn't a great reason imo.
> > 
> > > A similar redirection is done for several other functions, including mempcpy, stpcpy
> > > and bzero. The namespace issue only exists for non-C90 functions that are used
> > > inside GLIBC, so a small subset of all supported functions.
> >
> > i think you're conflating those here.  if you look closely, it's for C++
> > code only, and it's because the signature is different (const-vs-non-const
> > return).  it's not for the reason you're doing a #define here.
> 
> No I'm not talking about the C++ const inlines, I mean redirects to avoid namespace
> issues (and to enable GCC to optimize builtins) for non-C90 functions that are used
> inside GLIBC. Eg:

i was getting hung up on the __builtin_xxx aspect.  the redirects you're
referring to don't involve those.  doing a redirect to a diff glibc symbol
is fine in the internal header.

i don't really have an opinion on the style (define-vs-alias).
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Wilco Dijkstra]

ping


From: Wilco Dijkstra
Sent: 12 December 2016 13:02
To: Zack Weinberg
Cc: libc-alpha@sourceware.org; nd
Subject: Re: [GLIBC][PATCH v2] Remove strdup inlines



Remove the str(n)dup inlines from string/bits/string2.h.  Although inlining
calls with constant strings shows a small (~10%) performance gain, strdup is
typically used in error reporting code, so not performance critical.
Remove the now unused __need_malloc_and_calloc related defines from stdlib.h.

Avoid linknamespace and localplt failures by redirecting str(n)dup to GCC
builtins.

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


        * include/stdlib.h (__need_malloc_and_calloc): Remove uses.
        (__Need_M_And_C) Remove define/undef.
        * stdlib/stdlib.h (__need_malloc_and_calloc): Remove uses.
        (__malloc_and_calloc_defined): Remove define.
        * string/string.h (strdup): Use builtin.
        (strndup): Likewise.
        * string/bits/string2.h (__strdup): Remove define.
        (strdup): Likewise.
        (__strndup): Likewise.
        (strndup): Likewise.

--
diff --git a/include/stdlib.h b/include/stdlib.h
index 352339e8595eb8229018cb27f7d2decf63f511c7..929cead59ae10afe03ae1286b72d8321f0ab2d90 100644
--- a/include/stdlib.h
+++ b/include/stdlib.h
@@ -1,16 +1,12 @@
 #ifndef _STDLIB_H
 
-#ifdef __need_malloc_and_calloc
-#define __Need_M_And_C
-#endif
-
 #ifndef _ISOMAC
 # include <stddef.h>
 #endif
 #include <stdlib/stdlib.h>
 
 /* Now define the internal interfaces.  */
-#if !defined __Need_M_And_C && !defined _ISOMAC
+#if !defined _ISOMAC
 # include <sys/stat.h>
 
 __BEGIN_DECLS
@@ -269,6 +265,4 @@ __END_DECLS
 
 #endif
 
-#undef __Need_M_And_C
-
 #endif  /* include/stdlib.h */
diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
index 48f9a9500e736a5d85e5eca2e511cff374e43226..b1696c7a8d82145f76760a49909e24c288c51263 100644
--- a/stdlib/stdlib.h
+++ b/stdlib/stdlib.h
@@ -26,15 +26,12 @@
 
 /* Get size_t, wchar_t and NULL from <stddef.h>.  */
 #define         __need_size_t
-#ifndef __need_malloc_and_calloc
-# define       __need_wchar_t
-# define       __need_NULL
-#endif
+#define                __need_wchar_t
+#define                __need_NULL
 #include <stddef.h>
 
 __BEGIN_DECLS
 
-#ifndef __need_malloc_and_calloc
 #define _STDLIB_H       1
 
 #if (defined __USE_XOPEN || defined __USE_XOPEN2K8) && !defined _SYS_WAIT_H
@@ -434,10 +431,6 @@ extern int lcong48_r (unsigned short int __param[7],
 # endif /* Use misc.  */
 #endif  /* Use misc or X/Open.  */
 
-#endif /* don't just need malloc and calloc */
-
-#ifndef __malloc_and_calloc_defined
-# define __malloc_and_calloc_defined
 __BEGIN_NAMESPACE_STD
 /* Allocate SIZE bytes of memory.  */
 extern void *malloc (size_t __size) __THROW __attribute_malloc__ __wur;
@@ -445,9 +438,7 @@ extern void *malloc (size_t __size) __THROW __attribute_malloc__ __wur;
 extern void *calloc (size_t __nmemb, size_t __size)
      __THROW __attribute_malloc__ __wur;
 __END_NAMESPACE_STD
-#endif
 
-#ifndef __need_malloc_and_calloc
 __BEGIN_NAMESPACE_STD
 /* Re-allocate the previously allocated block
    in PTR, making the new block SIZE bytes long.  */
@@ -944,9 +935,6 @@ extern int ttyslot (void) __THROW;
 # include <bits/stdlib-ldbl.h>
 #endif
 
-#endif /* don't just need malloc and calloc */
-#undef __need_malloc_and_calloc
-
 __END_DECLS
 
 #endif /* stdlib.h  */
diff --git a/string/bits/string2.h b/string/bits/string2.h
index 03af22cc27a33c81f36d56ddc52fce0a5ea81ece..b0be5f6a49024a0bedfc37e9ec2c0356e0e4fa09 100644
--- a/string/bits/string2.h
+++ b/string/bits/string2.h
@@ -118,58 +118,6 @@
 #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.  */
-#ifdef __USE_MISC
-
-# define __need_malloc_and_calloc
-# include <stdlib.h>
-
-extern char *__strdup (const char *__string) __THROW __attribute_malloc__;
-# define __strdup(s) \
-  (__extension__ (__builtin_constant_p (s) && __string2_1bptr_p (s)          \
-                 ? (((const char *) (s))[0] == '\0'                           \
-                    ? (char *) calloc ((size_t) 1, (size_t) 1)                \
-                    : ({ size_t __len = strlen (s) + 1;               \
-                         char *__retval = (char *) malloc (__len);            \
-                         if (__retval != NULL)                                \
-                           __retval = (char *) memcpy (__retval, s, __len);  \
-                         __retval; }))                                        \
-                 : __strdup (s)))
-
-# if defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8
-#  define strdup(s) __strdup (s)
-# endif
-
-
-extern char *__strndup (const char *__string, size_t __n)
-     __THROW __attribute_malloc__;
-# define __strndup(s, n) \
-  (__extension__ (__builtin_constant_p (s) && __string2_1bptr_p (s)          \
-                 ? (((const char *) (s))[0] == '\0'                           \
-                    ? (char *) calloc ((size_t) 1, (size_t) 1)                \
-                    : ({ size_t __len = strlen (s) + 1;               \
-                         size_t __n = (n);                                    \
-                         char *__retval;                                      \
-                         if (__n < __len)                                     \
-                           __len = __n + 1;                                   \
-                         __retval = (char *) malloc (__len);                  \
-                         if (__retval != NULL)                                \
-                           {                                                  \
-                             __retval[__len - 1] = '\0';                      \
-                             __retval = (char *) memcpy (__retval, s,         \
-                                                         __len - 1);          \
-                           }                                                  \
-                         __retval; }))                                        \
-                 : __strndup (s, n)))
-
-# ifdef __USE_XOPEN2K8
-#  define strndup(s, n) __strndup (s, n)
-# endif
-
-#endif /* Use misc. or use GNU.  */
-
 #ifndef _FORCE_INLINES
 # undef __STRING_INLINE
 #endif
diff --git a/string/string.h b/string/string.h
index b103e64912fe1904098e229ccb845bb2c5c10835..c251cc603a1dbb5bef469d4b71f90037612d36f0 100644
--- a/string/string.h
+++ b/string/string.h
@@ -636,6 +636,15 @@ extern char *basename (const char *__filename) __THROW __nonnull ((1));
 # endif
 #endif
 
+#if (defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8     \
+     || __GLIBC_USE (LIB_EXT2))
+# define strdup(s) __builtin_strdup (s)
+#endif
+
+#if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
+#define strndup(s,n) __builtin_strndup (s, n)
+#endif
+
 #if defined __USE_GNU && defined __OPTIMIZE__ \
     && defined __extern_always_inline && __GNUC_PREREQ (3,2)
 # if !defined _FORCE_INLINES && !defined _HAVE_STRING_ARCH_mempcpy


    

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Adhemerval Zanella]

LGTM with some comments below:

On 09/02/2017 13:35, Wilco Dijkstra wrote:
> diff --git a/string/bits/string2.h b/string/bits/string2.h
> index 03af22cc27a33c81f36d56ddc52fce0a5ea81ece..b0be5f6a49024a0bedfc37e9ec2c0356e0e4fa09 100644
> --- a/string/bits/string2.h
> +++ b/string/bits/string2.h
> @@ -118,58 +118,6 @@
>  #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.  */
> -#ifdef __USE_MISC
> -
> -# define __need_malloc_and_calloc
> -# include <stdlib.h>
> -

I think this patch is based on other ones, since this does not apply on
master.

> diff --git a/string/string.h b/string/string.h
> index b103e64912fe1904098e229ccb845bb2c5c10835..c251cc603a1dbb5bef469d4b71f90037612d36f0 100644
> --- a/string/string.h
> +++ b/string/string.h
> @@ -636,6 +636,15 @@ extern char *basename (const char *__filename) __THROW __nonnull ((1));
>  # endif
>  #endif
>  
> +#if (defined __USE_XOPEN_EXTENDED || defined __USE_XOPEN2K8     \
> +     || __GLIBC_USE (LIB_EXT2))
> +# define strdup(s) __builtin_strdup (s)
> +#endif

This is not suffice to avoid static link namespace issues, since gcc usually
issue a strdup for __builtin_strdup.  The conform tests shows a lot of issues
for both strdup and strndup.  I think a better option would just to redirect
it to __strdup and __strndup (since they are exported on libc.so).

Also for strdup I think __USE_XOPEN2K8 is not required, accordingly to manual
it is defined for

 _SVID_SOURCE || _BSD_SOURCE || _XOPEN_SOURCE >= 500 || _XOPEN_SOURCE && _XOPEN_SOURCE_EXTENDED

And __USE_XOPEN_EXTENDED will be set for _XOPEN_SOURCE >= 500 or if
_XOPEN_SOURCE_EXTENDED is defined.  At least all required defined are
suffice on conform testcases for just 

#if (defined __USE_XOPEN_EXTENDED || __GLIBC_USE (LIB_EXT2))

> +
> +#if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
> +#define strndup(s,n) __builtin_strndup (s, n)
> +#endif
> +
>  #if defined __USE_GNU && defined __OPTIMIZE__ \
>      && defined __extern_always_inline && __GNUC_PREREQ (3,2)
>  # if !defined _FORCE_INLINES && !defined _HAVE_STRING_ARCH_mempcpy
> 
> 
>     
> 

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Joseph Myers]

On Thu, 9 Feb 2017, Adhemerval Zanella wrote:

> Also for strdup I think __USE_XOPEN2K8 is not required, accordingly to manual
> it is defined for
> 
>  _SVID_SOURCE || _BSD_SOURCE || _XOPEN_SOURCE >= 500 || _XOPEN_SOURCE && _XOPEN_SOURCE_EXTENDED

strdup was XSI-shaded in the 2001 edition of POSIX but moved to CX-shaded 
in the 2008 edition.  That is, it should be declared for 
_POSIX_C_SOURCE=200809L.  That is, the existing __USE_XOPEN2K8 condition 
(which means non-XSI 2008 edition of POSIX) is right.

> And __USE_XOPEN_EXTENDED will be set for _XOPEN_SOURCE >= 500 or if
> _XOPEN_SOURCE_EXTENDED is defined.  At least all required defined are
> suffice on conform testcases for just 

This indicates you've found a bug in the conform/ expectations - strdup 
should be required there for POSIX2008.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Adhemerval Zanella]

On 09/02/2017 19:57, Joseph Myers wrote:
> On Thu, 9 Feb 2017, Adhemerval Zanella wrote:
> 
>> Also for strdup I think __USE_XOPEN2K8 is not required, accordingly to manual
>> it is defined for
>>
>>  _SVID_SOURCE || _BSD_SOURCE || _XOPEN_SOURCE >= 500 || _XOPEN_SOURCE && _XOPEN_SOURCE_EXTENDED
> 
> strdup was XSI-shaded in the 2001 edition of POSIX but moved to CX-shaded 
> in the 2008 edition.  That is, it should be declared for 
> _POSIX_C_SOURCE=200809L.  That is, the existing __USE_XOPEN2K8 condition 
> (which means non-XSI 2008 edition of POSIX) is right.

Indeed, I was in fact confused my documentation and the manpages confirmed that
it is true for glibc 2.12 and forward.  In any case, I think both these
macros should be removed [2].

[2] https://sourceware.org/ml/libc-alpha/2017-02/msg00201.html

> 
>> And __USE_XOPEN_EXTENDED will be set for _XOPEN_SOURCE >= 500 or if
>> _XOPEN_SOURCE_EXTENDED is defined.  At least all required defined are
>> suffice on conform testcases for just 
> 
> This indicates you've found a bug in the conform/ expectations - strdup 
> should be required there for POSIX2008.
> 

Right, so if I think the condition on conform/data/string.h-data should be:

diff --git a/conform/data/string.h-data b/conform/data/string.h-data
index 39fa76c..b7c2784 100644
--- a/conform/data/string.h-data
+++ b/conform/data/string.h-data
@@ -27,7 +27,7 @@ function int strcoll_l (const char*, const char*, locale_t)
 #endif
 function {char*} strcpy (char*, const char*)
 function size_t strcspn (const char*, const char*)
-#if !defined ISO && !defined ISO99 & !defined ISO11 && !defined XPG3 && !defined POSIX && !defined POSIX2008
+#if !defined ISO && !defined ISO99 & !defined ISO11 && !defined XPG3 && !defined POSIX && defined POSIX2008
 function {char*} strdup (const char*)
 #endif
 function {char*} strerror (int)

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Joseph Myers]

On Thu, 9 Feb 2017, Adhemerval Zanella wrote:

> Right, so if I think the condition on conform/data/string.h-data should be:
> 
> diff --git a/conform/data/string.h-data b/conform/data/string.h-data
> index 39fa76c..b7c2784 100644
> --- a/conform/data/string.h-data
> +++ b/conform/data/string.h-data
> @@ -27,7 +27,7 @@ function int strcoll_l (const char*, const char*, locale_t)
>  #endif
>  function {char*} strcpy (char*, const char*)
>  function size_t strcspn (const char*, const char*)
> -#if !defined ISO && !defined ISO99 & !defined ISO11 && !defined XPG3 && !defined POSIX && !defined POSIX2008
> +#if !defined ISO && !defined ISO99 & !defined ISO11 && !defined XPG3 && !defined POSIX && defined POSIX2008

No, just remove the && !defined POSIX2008.  Exactly one of the macros for 
individual standards will always be defined when processing the conform/ 
data.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [GLIBC][PATCH v2] Remove strdup inlines

[Adhemerval Zanella]

On 09/02/2017 20:45, Joseph Myers wrote:
> On Thu, 9 Feb 2017, Adhemerval Zanella wrote:
> 
>> Right, so if I think the condition on conform/data/string.h-data should be:
>>
>> diff --git a/conform/data/string.h-data b/conform/data/string.h-data
>> index 39fa76c..b7c2784 100644
>> --- a/conform/data/string.h-data
>> +++ b/conform/data/string.h-data
>> @@ -27,7 +27,7 @@ function int strcoll_l (const char*, const char*, locale_t)
>>  #endif
>>  function {char*} strcpy (char*, const char*)
>>  function size_t strcspn (const char*, const char*)
>> -#if !defined ISO && !defined ISO99 & !defined ISO11 && !defined XPG3 && !defined POSIX && !defined POSIX2008
>> +#if !defined ISO && !defined ISO99 & !defined ISO11 && !defined XPG3 && !defined POSIX && defined POSIX2008
> 
> No, just remove the && !defined POSIX2008.  Exactly one of the macros for 
> individual standards will always be defined when processing the conform/ 
> data.
> 

Right, I will prepare the patch.