This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] x86-64: Implement strcpy family IFUNC selectors in C
On 11/06/2017 20:33, H.J. Lu wrote:
> On Sun, Jun 11, 2017 at 12:46 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Sun, Jun 11, 2017 at 11:22 AM, Florian Weimer
>> <fweimer@gapps.redhat.com> wrote:
>>> On 06/11/2017 03:50 PM, H.J. Lu wrote:
>>>> All internal calls within libc.so can use IFUNC on x86-64 since unlike
>>>> x86, x86-64 doesn't need to reserve a register to make a PLT call.
>>>
>>> Shouldn't this rather say, “because, unlike x86, x86-64 supports PC-relative
>>> addressing to access the GOT entry”? That's probably the root cause of the
>> Both are true. The end result is there is no extra needed for call
>> via PLT.
>>
>>> performance difference.
>>>
> Here is the patch with updated commit message. I also renamed
> ifunc-strcpy.h to ifunc-unaligned-ssse3.h so that it can be used for
> strcat.
>
> Any other comments?
>
LGTM with just some remarks below.
> +
> +# include <sysdep.h>
> +# define __stpcpy __stpcpy_sse2
> +
> +# undef weak_alias
> +# define weak_alias(__stpcpy, stpcpy)
> +# undef libc_hidden_def
> +# define libc_hidden_def(__stpcpy)
> +# undef libc_hidden_builtin_def
> +# define libc_hidden_builtin_def(stpcpy)
> +#endif
> +
> +#define USE_AS_STPCPY
> +#include "../stpcpy.S"
I would recommend use full path.
> diff --git a/sysdeps/x86_64/multiarch/stpcpy.S b/sysdeps/x86_64/multiarch/stpcpy.S
> deleted file mode 100644
> index ee81ab6..0000000
> --- a/sysdeps/x86_64/multiarch/stpcpy.S
> +++ /dev/null
> @@ -1,9 +0,0 @@
> -/* Multiple versions of stpcpy
> - All versions must be listed in ifunc-impl-list.c. */
> -#define USE_AS_STPCPY
> -#define STRCPY __stpcpy
> -#include "strcpy.S"
> -
> -weak_alias (__stpcpy, stpcpy)
> -libc_hidden_def (__stpcpy)
> -libc_hidden_builtin_def (stpcpy)
> diff --git a/sysdeps/x86_64/multiarch/stpcpy.c b/sysdeps/x86_64/multiarch/stpcpy.c
> new file mode 100644
> index 0000000..75c158d
> --- /dev/null
> +++ b/sysdeps/x86_64/multiarch/stpcpy.c
> @@ -0,0 +1,40 @@
> +/* Multiple versions of stpcpy.
> + All versions must be listed in ifunc-impl-list.c.
> + Copyright (C) 2017 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +/* Define multiple versions only for the definition in libc. */
> +#if IS_IN (libc)
> +# define _HAVE_STRING_ARCH_stpcpy 1
I think we will need to revise this later, since the idea is to remove string2.h.
> +# define stpcpy __redirect_stpcpy
> +# define __stpcpy __redirect___stpcpy
> +# include <string.h>
> +# undef stpcpy
> +# undef __stpcpy
> +
> +# define SYMBOL_NAME stpcpy
> +# include "ifunc-unaligned-ssse3.h"
> +
> +libc_ifunc_redirected (__redirect_stpcpy, __stpcpy, IFUNC_SELECTOR ());
> +# ifdef SHARED
> +weak_alias (__stpcpy, stpcpy)
> +__hidden_ver1 (__stpcpy, __GI___stpcpy, __redirect___stpcpy)
> + __attribute__((visibility ("hidden")));
> +__hidden_ver1 (stpcpy, __GI_stpcpy, __redirect_stpcpy)
> + __attribute__((visibility ("hidden")));
> +# endif
> +#endif
> diff --git a/sysdeps/x86_64/multiarch/stpncpy-c.c b/sysdeps/x86_64/multiarch/stpncpy-c.c
> index 2fde77d..62f69b5 100644
> --- a/sysdeps/x86_64/multiarch/stpncpy-c.c
> +++ b/sysdeps/x86_64/multiarch/stpncpy-c.c
> @@ -1,8 +1,7 @@
> #define STPNCPY __stpncpy_sse2
> -#ifdef SHARED
> +#undef weak_alias
> +#define weak_alias(__stpncpy, stpncpy)
> #undef libc_hidden_def
> -#define libc_hidden_def(name) \
> - __hidden_ver1 (__stpncpy_sse2, __GI___stpncpy, __stpncpy_sse2);
> -#endif
> +#define libc_hidden_def(stpncpy)
>
> -#include "stpncpy.c"
> +#include <string/stpncpy.c>
Shouldn't we add a copyright header on this file (same for sysdeps/x86_64/multiarch/strncpy-c.c)?
> diff --git a/sysdeps/x86_64/multiarch/stpncpy.S b/sysdeps/x86_64/multiarch/stpncpy.S
> deleted file mode 100644
> index 2698ca6..0000000
> --- a/sysdeps/x86_64/multiarch/stpncpy.S
> +++ /dev/null
> @@ -1,8 +0,0 @@
> -/* Multiple versions of stpncpy
> - All versions must be listed in ifunc-impl-list.c. */
> -#define STRCPY __stpncpy
> -#define USE_AS_STPCPY
> -#define USE_AS_STRNCPY
> -#include "strcpy.S"
> -
> -weak_alias (__stpncpy, stpncpy)
> diff --git a/sysdeps/x86_64/multiarch/stpncpy.c b/sysdeps/x86_64/multiarch/stpncpy.c
> new file mode 100644
> index 0000000..872ff75
> --- /dev/null
> +++ b/sysdeps/x86_64/multiarch/stpncpy.c
> @@ -0,0 +1,37 @@
> +/* Multiple versions of stpncpy.
> + All versions must be listed in ifunc-impl-list.c.
> + Copyright (C) 2017 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +/* Define multiple versions only for the definition in libc. */
> +#if IS_IN (libc)
> +# define stpncpy __redirect_stpncpy
> +# define __stpncpy __redirect___stpncpy
> +# include <string.h>
> +# undef stpncpy
> +# undef __stpncpy
> +
> +# define SYMBOL_NAME stpncpy
> +# include "ifunc-unaligned-ssse3.h"
> +
> +libc_ifunc_redirected (__redirect_stpncpy, __stpncpy, IFUNC_SELECTOR ());
> +# ifdef SHARED
> +weak_alias (__stpncpy, stpncpy)
> +__hidden_ver1 (__stpncpy, __GI___stpncpy, __redirect___stpncpy)
> + __attribute__((visibility ("hidden")));
> +# endif
> +#endif
> diff --git a/sysdeps/x86_64/multiarch/strcpy-sse2.S b/sysdeps/x86_64/multiarch/strcpy-sse2.S
> new file mode 100644
> index 0000000..44b910d
> --- /dev/null
> +++ b/sysdeps/x86_64/multiarch/strcpy-sse2.S
> @@ -0,0 +1,28 @@
> +/* strcpy optimized with SSE2.
> + Copyright (C) 2017 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#if IS_IN (libc)
> +
> +# include <sysdep.h>
> +# define strcpy __strcpy_sse2
> +
> +# undef libc_hidden_builtin_def
> +# define libc_hidden_builtin_def(strcpy)
> +#endif
> +
> +#include "../strcpy.S"
I would recommend use full path as well.