diff mbox

[v2] Remove strdup inlines

Message ID AM5PR0802MB26105C61918F323EC765968F83450@AM5PR0802MB2610.eurprd08.prod.outlook.com
State New
Headers show

Commit Message

Wilco Dijkstra Feb. 9, 2017, 3:35 p.m. UTC 
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.

--

Comments

Adhemerval Zanella Netto Feb. 9, 2017, 9:05 p.m. UTC | #1 
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
> 
> 
>     
>
Joseph Myers Feb. 9, 2017, 9:57 p.m. UTC | #2 
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.
diff mbox

Patch

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