Use libc_hidden_proto / _def for hidden wchar ifunc symbols.

Message ID 8f9ec268-7792-1b86-aee4-ec3aa5878108@linux.vnet.ibm.com
State New
Headers show
Series
  • Use libc_hidden_proto / _def for hidden wchar ifunc symbols.
Related show

Commit Message

Stefan Liebler Nov. 13, 2017, 10:16 a.m.
Hi,

on s390 (31bit) various debug/tst-*chk* testcases are failing as the 
tests are ending with a segmentation fault.

One test is e.g. calling wcsnrtombs in debug/tst-chk1.c:1549.
The function wcsnrtombs itself calls __wcsnlen. This function is called 
via PLT! The PLT-stub itself loads the address from GOT (r12 is assumed 
to be the GOT-pointer). In this case the loaded address is zero and the 
following branch leads to the segmentation fault.

Due to the attribute-hidden in commit 
44af8a32c341672b5160fdc2839767e9a837ad26 "Mark internal wchar functions 
with attribute_hidden [BZ #18822]" for e.g. the __wcsnlen function, r12 
is not loaded with the GOT-pointer in wcsnrtombs.

On s390x (64bit), this __wcsnlen call is also using the PLT-stub. But it 
is not failing as the GOT-pointer is setup with larl-instruction by the 
PLT-stub itself.
Note: On s390x/s390, __wcsnlen is an IFUNC symbol.

On x86_64, __wcsnlen is also an IFUNC symbol and is called via PLT, too.

Further IFUNC symbols on s390 which were marked as hidden by the 
mentioned commit are: __wcscat, __wcsncpy, __wcpncpy, __wcschrnul.

This patch adds the libc_hidden_proto / libc_hidden_def construct.
Then the __GI_* symbols are the default-ifunc-variants which can be 
called without PLT.

Okay to commit?

Bye.
Stefan

ChangeLog:

	* include/wchar.h (__wcsnlen, __wcscat, __wcsncpy, __wcpncpy,
	__wcschrnul): Add libc_hidden_proto.
	* wcsmbs/wcsnlen.c (__wcsnlen): Add libc_hidden_def.
	* wcsmbs/wcscat.c (__wcscat): Likewise.
	* wcsmbs/wcschrnul.c (__wcschrnul): Likewise.
	* wcsmbs/wcsncpy.c (__wcsncpy): Likewise.
	* wcsmbs/wcpncpy.c (__wcpncpy): Likewise.
	* sysdeps/x86_64/multiarch/wcsnlen-c.c (__wcsnlen): Add libc_hidden_def
	which aliases to default ifunc-variant.
	* sysdeps/s390/multiarch/wcsnlen-c.c (__wcsnlen):  Likewise.
	* sysdeps/s390/multiarch/wcscat-c.c (__wcscat): Likewise.
	* sysdeps/s390/multiarch/wcschrnul-c.c  (__wcschrnul): Likewise.
	* sysdeps/s390/multiarch/wcsncpy-c.c (__wcsncpy): Likewise.
	* sysdeps/s390/multiarch/wcpncpy-c.c (__wcpncpy): Likewise.
	* sysdeps/s390/multiarch/wcsnlen.c (__wcsnlen): Redirect symbol in
	header and use s390_vx_libc_ifunc_redirected instead of
	s390_vx_libc_ifunc.
	* sysdeps/s390/multiarch/wcscat.c (__wcscat): Likewise.
	* sysdeps/s390/multiarch/wcschrnul.c (__wcschrnul): Likewise.
	* sysdeps/s390/multiarch/wcsncpy.c (__wcsncpy): Likewise.
	* sysdeps/s390/multiarch/wcpncpy.c (__wcpncpy): Likewise.

Comments

Florian Weimer Nov. 13, 2017, 11:55 a.m. | #1
On 11/13/2017 11:16 AM, Stefan Liebler wrote:
> This patch adds the libc_hidden_proto / libc_hidden_def construct.
> Then the __GI_* symbols are the default-ifunc-variants which can be 
> called without PLT.

attribute_hidden and *_hidden_{proto,def} conflict on some 
architectures.  You need to remove attribute_hidden as part of this change.

Thanks,
Florian
H.J. Lu Nov. 13, 2017, 1:58 p.m. | #2
On Mon, Nov 13, 2017 at 3:55 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 11/13/2017 11:16 AM, Stefan Liebler wrote:
>>
>> This patch adds the libc_hidden_proto / libc_hidden_def construct.
>> Then the __GI_* symbols are the default-ifunc-variants which can be called
>> without PLT.
>
>
> attribute_hidden and *_hidden_{proto,def} conflict on some architectures.
> You need to remove attribute_hidden as part of this change.

That is true.  On i686, a hidden IFUNC function inside libc.so must be compiled
with -fPIC via PLT since EBX must be loaded with GOT first.   This
isn't an issue
for x86-64 since PLT uses PC-relative addressing.  In this case, we
should remove
hidden attribute, instead of using __GI_* symbols, if we sill want to use IFUNC
inside libc.so.

Now I have question, is there a way to apply attribute_hidden to a function
depending on architecture? For example, we remove attribute_hidden
from __wcsnlen, __wcscat, __wcsncpy, __wcpncpy,  __wcschrnul in headers
under include/.   For x86, we mark them hidden in a header file under
sysdeps/x86?
Stefan Liebler Nov. 13, 2017, 5:11 p.m. | #3
On 11/13/2017 12:55 PM, Florian Weimer wrote:
> On 11/13/2017 11:16 AM, Stefan Liebler wrote:
>> This patch adds the libc_hidden_proto / libc_hidden_def construct.
>> Then the __GI_* symbols are the default-ifunc-variants which can be 
>> called without PLT.
> 
> attribute_hidden and *_hidden_{proto,def} conflict on some 
> architectures.  You need to remove attribute_hidden as part of this change.
> 
> Thanks,
> Florian
> 

Thanks for this hint.
This is an updated version which removes the attribute_hidden, too.

Bye
Stefan
commit 9c8af17c56134cb1cde6a3adc3aa7737c49cbeff
Author: Stefan Liebler <stli@linux.vnet.ibm.com>
Date:   Mon Nov 13 13:46:50 2017 +0100

    Use libc_hidden_proto / _def for hidden wchar ifunc symbols.
    
    On s390 (31bit) various debug/tst-*chk* testcases are failing as the tests
    are ending with a segmentation fault.
    
    One test is e.g. calling wcsnrtombs in debug/tst-chk1.c:1549.
    The function wcsnrtombs itself calls __wcsnlen. This function is called via
    PLT! The PLT-stub itself loads the address from GOT (r12 is assumed to be
    the GOT-pointer). In this case the loaded address is zero and the following
    branch leads to the segmentation fault.
    
    Due to the attribute-hidden in commit 44af8a32c341672b5160fdc2839767e9a837ad26
    "Mark internal wchar functions with attribute_hidden [BZ #18822]"
    for e.g. the __wcsnlen function, r12 is not loaded with the GOT-pointer
    in wcsnrtombs.
    
    On s390x (64bit), this __wcsnlen call is also using the PLT-stub. But it is
    not failing as the GOT-pointer is setup with larl-instruction by the PLT-stub
    itself.
    Note: On s390x/s390, __wcsnlen is an IFUNC symbol.
    
    On x86_64, __wcsnlen is also an IFUNC symbol and is called via PLT, too.
    
    Further IFUNC symbols on s390 which were marked as hidden by the mentioned
    commit are: __wcscat, __wcsncpy, __wcpncpy, __wcschrnul.
    
    This patch adds the libc_hidden_proto / libc_hidden_def construct and
    removes the attribute_hidden as it conflicts with libc_hidden* construct on
    some architectures. Then the __GI_* symbols are the default-ifunc-variants
    which can be called without PLT.
    
    ChangeLog:
    
            * include/wchar.h (__wcsnlen, __wcscat, __wcsncpy, __wcpncpy,
            __wcschrnul): Add libc_hidden_proto, Remove attribute_hidden.
            * wcsmbs/wcsnlen.c (__wcsnlen): Add libc_hidden_def.
            * wcsmbs/wcscat.c (__wcscat): Likewise.
            * wcsmbs/wcschrnul.c (__wcschrnul): Likewise.
            * wcsmbs/wcsncpy.c (__wcsncpy): Likewise.
            * wcsmbs/wcpncpy.c (__wcpncpy): Likewise.
            * sysdeps/x86_64/multiarch/wcsnlen-c.c (__wcsnlen): Add libc_hidden_def
            which aliases to default ifunc-variant.
            * sysdeps/s390/multiarch/wcsnlen-c.c (__wcsnlen):  Likewise.
            * sysdeps/s390/multiarch/wcscat-c.c (__wcscat): Likewise.
            * sysdeps/s390/multiarch/wcschrnul-c.c  (__wcschrnul): Likewise.
            * sysdeps/s390/multiarch/wcsncpy-c.c (__wcsncpy): Likewise.
            * sysdeps/s390/multiarch/wcpncpy-c.c (__wcpncpy): Likewise.
            * sysdeps/s390/multiarch/wcsnlen.c (__wcsnlen): Redirect symbol in
            header and use s390_vx_libc_ifunc_redirected instead of
            s390_vx_libc_ifunc.
            * sysdeps/s390/multiarch/wcscat.c (__wcscat): Likewise.
            * sysdeps/s390/multiarch/wcschrnul.c (__wcschrnul): Likewise.
            * sysdeps/s390/multiarch/wcsncpy.c (__wcsncpy): Likewise.
            * sysdeps/s390/multiarch/wcpncpy.c (__wcpncpy): Likewise.

diff --git a/include/wchar.h b/include/wchar.h
index 24b2eaa..1efc33b 100644
--- a/include/wchar.h
+++ b/include/wchar.h
@@ -152,9 +152,10 @@ extern int __wcsncasecmp (const wchar_t *__s1, const wchar_t *__s2,
      __attribute_pure__;
 extern size_t __wcslen (const wchar_t *__s) __attribute_pure__;
 extern size_t __wcsnlen (const wchar_t *__s, size_t __maxlen)
-     attribute_hidden __attribute_pure__;
-extern wchar_t *__wcscat (wchar_t *dest, const wchar_t *src)
-     attribute_hidden;
+     __attribute_pure__;
+libc_hidden_proto (__wcsnlen)
+extern wchar_t *__wcscat (wchar_t *dest, const wchar_t *src);
+libc_hidden_proto (__wcscat)
 extern wint_t __btowc (int __c) attribute_hidden;
 extern int __mbsinit (const __mbstate_t *__ps);
 extern size_t __mbrtowc (wchar_t *__restrict __pwc,
@@ -182,11 +183,12 @@ extern size_t __wcsnrtombs (char *__restrict __dst,
 			    __mbstate_t *__restrict __ps)
      attribute_hidden;
 extern wchar_t *__wcsncpy (wchar_t *__restrict __dest,
-			   const wchar_t *__restrict __src, size_t __n)
-     attribute_hidden;
+			   const wchar_t *__restrict __src, size_t __n);
+libc_hidden_proto (__wcsncpy)
 extern wchar_t *__wcpcpy (wchar_t *__dest, const wchar_t *__src);
 extern wchar_t *__wcpncpy (wchar_t *__dest, const wchar_t *__src,
-			   size_t __n) attribute_hidden;
+			   size_t __n);
+libc_hidden_proto (__wcpncpy)
 extern wchar_t *__wmemcpy (wchar_t *__s1, const wchar_t *s2,
 			   size_t __n) attribute_hidden;
 extern wchar_t *__wmempcpy (wchar_t *__restrict __s1,
@@ -195,7 +197,8 @@ extern wchar_t *__wmempcpy (wchar_t *__restrict __s1,
 extern wchar_t *__wmemmove (wchar_t *__s1, const wchar_t *__s2,
 			    size_t __n) attribute_hidden;
 extern wchar_t *__wcschrnul (const wchar_t *__s, wchar_t __wc)
-     attribute_hidden __attribute_pure__;
+     __attribute_pure__;
+libc_hidden_proto (__wcschrnul)
 
 extern wchar_t *__wmemset_chk (wchar_t *__s, wchar_t __c, size_t __n,
 			       size_t __ns) __THROW;
diff --git a/sysdeps/s390/multiarch/wcpncpy-c.c b/sysdeps/s390/multiarch/wcpncpy-c.c
index 7cb834b..b02e40a 100644
--- a/sysdeps/s390/multiarch/wcpncpy-c.c
+++ b/sysdeps/s390/multiarch/wcpncpy-c.c
@@ -21,5 +21,11 @@
 
 # include <wchar.h>
 extern __typeof (__wcpncpy) __wcpncpy_c;
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcpncpy_c, __GI___wcpncpy, __wcpncpy_c);
+# endif /* SHARED */
 # include <wcsmbs/wcpncpy.c>
+libc_hidden_def (__wcpncpy)
 #endif
diff --git a/sysdeps/s390/multiarch/wcpncpy.c b/sysdeps/s390/multiarch/wcpncpy.c
index 1a19a99..a902787 100644
--- a/sysdeps/s390/multiarch/wcpncpy.c
+++ b/sysdeps/s390/multiarch/wcpncpy.c
@@ -17,10 +17,12 @@
    <http://www.gnu.org/licenses/>.  */
 
 #if defined HAVE_S390_VX_ASM_SUPPORT && IS_IN (libc)
+# define __wcpncpy __redirect___wcpncpy
 # include <wchar.h>
+# undef __wcpncpy
 # include <ifunc-resolve.h>
 
-s390_vx_libc_ifunc (__wcpncpy)
+s390_vx_libc_ifunc_redirected (__redirect___wcpncpy, __wcpncpy)
 weak_alias (__wcpncpy, wcpncpy)
 
 #else
diff --git a/sysdeps/s390/multiarch/wcscat-c.c b/sysdeps/s390/multiarch/wcscat-c.c
index ef7c000..3ac6110 100644
--- a/sysdeps/s390/multiarch/wcscat-c.c
+++ b/sysdeps/s390/multiarch/wcscat-c.c
@@ -21,5 +21,11 @@
 
 # include <wchar.h>
 extern __typeof (__wcscat) __wcscat_c;
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcscat_c, __GI___wcscat, __wcscat_c);
+# endif /* SHARED */
 # include <wcsmbs/wcscat.c>
+libc_hidden_def (__wcscat)
 #endif
diff --git a/sysdeps/s390/multiarch/wcscat.c b/sysdeps/s390/multiarch/wcscat.c
index 2e7fded..3f1219e 100644
--- a/sysdeps/s390/multiarch/wcscat.c
+++ b/sysdeps/s390/multiarch/wcscat.c
@@ -17,10 +17,12 @@
    <http://www.gnu.org/licenses/>.  */
 
 #if defined HAVE_S390_VX_ASM_SUPPORT && IS_IN (libc)
+# define __wcscat __redirect___wcscat
 # include <wchar.h>
+# undef __wcscat
 # include <ifunc-resolve.h>
 
-s390_vx_libc_ifunc (__wcscat)
+s390_vx_libc_ifunc_redirected (__redirect___wcscat, __wcscat)
 weak_alias (__wcscat, wcscat)
 
 #else
diff --git a/sysdeps/s390/multiarch/wcschrnul-c.c b/sysdeps/s390/multiarch/wcschrnul-c.c
index 095ea12..209a305 100644
--- a/sysdeps/s390/multiarch/wcschrnul-c.c
+++ b/sysdeps/s390/multiarch/wcschrnul-c.c
@@ -21,5 +21,11 @@
 
 # include <wchar.h>
 extern __typeof (__wcschrnul) __wcschrnul_c;
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcschrnul_c, __GI___wcschrnul, __wcschrnul_c);
+# endif /* SHARED */
 # include <wcsmbs/wcschrnul.c>
+libc_hidden_def (__wcschrnul)
 #endif
diff --git a/sysdeps/s390/multiarch/wcschrnul.c b/sysdeps/s390/multiarch/wcschrnul.c
index f01ea9f..c112cfb 100644
--- a/sysdeps/s390/multiarch/wcschrnul.c
+++ b/sysdeps/s390/multiarch/wcschrnul.c
@@ -17,10 +17,12 @@
    <http://www.gnu.org/licenses/>.  */
 
 #if defined HAVE_S390_VX_ASM_SUPPORT && IS_IN (libc)
+# define __wcschrnul __redirect___wcschrnul
 # include <wchar.h>
+# undef __wcschrnul
 # include <ifunc-resolve.h>
 
-s390_vx_libc_ifunc (__wcschrnul)
+s390_vx_libc_ifunc_redirected (__redirect___wcschrnul, __wcschrnul)
 weak_alias (__wcschrnul, wcschrnul)
 
 #else
diff --git a/sysdeps/s390/multiarch/wcsncpy-c.c b/sysdeps/s390/multiarch/wcsncpy-c.c
index 32ec8ff..d896220 100644
--- a/sysdeps/s390/multiarch/wcsncpy-c.c
+++ b/sysdeps/s390/multiarch/wcsncpy-c.c
@@ -21,5 +21,11 @@
 
 # include <wchar.h>
 extern __typeof (__wcsncpy) __wcsncpy_c;
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcsncpy_c, __GI___wcsncpy, __wcsncpy_c);
+# endif /* SHARED */
 # include <wcsmbs/wcsncpy.c>
+libc_hidden_def (__wcsncpy)
 #endif
diff --git a/sysdeps/s390/multiarch/wcsncpy.c b/sysdeps/s390/multiarch/wcsncpy.c
index 6e1e8f0..e08a82f 100644
--- a/sysdeps/s390/multiarch/wcsncpy.c
+++ b/sysdeps/s390/multiarch/wcsncpy.c
@@ -17,10 +17,12 @@
    <http://www.gnu.org/licenses/>.  */
 
 #if defined HAVE_S390_VX_ASM_SUPPORT && IS_IN (libc)
+# define __wcsncpy __redirect___wcsncpy
 # include <wchar.h>
+# undef __wcsncpy
 # include <ifunc-resolve.h>
 
-s390_vx_libc_ifunc (__wcsncpy)
+s390_vx_libc_ifunc_redirected (__redirect___wcsncpy, __wcsncpy)
 weak_alias (__wcsncpy, wcsncpy)
 
 #else
diff --git a/sysdeps/s390/multiarch/wcsnlen-c.c b/sysdeps/s390/multiarch/wcsnlen-c.c
index e86ca65..c6bc0cc 100644
--- a/sysdeps/s390/multiarch/wcsnlen-c.c
+++ b/sysdeps/s390/multiarch/wcsnlen-c.c
@@ -21,5 +21,11 @@
 
 # include <wchar.h>
 extern __typeof (__wcsnlen) __wcsnlen_c;
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcsnlen_c, __GI___wcsnlen, __wcsnlen_c);
+# endif /* SHARED */
 # include <wcsmbs/wcsnlen.c>
+libc_hidden_def (__wcsnlen)
 #endif
diff --git a/sysdeps/s390/multiarch/wcsnlen.c b/sysdeps/s390/multiarch/wcsnlen.c
index 54486b9..df29343 100644
--- a/sysdeps/s390/multiarch/wcsnlen.c
+++ b/sysdeps/s390/multiarch/wcsnlen.c
@@ -17,10 +17,12 @@
    <http://www.gnu.org/licenses/>.  */
 
 #if defined HAVE_S390_VX_ASM_SUPPORT && IS_IN (libc)
+# define __wcsnlen __redirect___wcsnlen
 # include <wchar.h>
+# undef __wcsnlen
 # include <ifunc-resolve.h>
 
-s390_vx_libc_ifunc (__wcsnlen)
+s390_vx_libc_ifunc_redirected (__redirect___wcsnlen, __wcsnlen)
 weak_alias (__wcsnlen, wcsnlen)
 
 #else
diff --git a/sysdeps/x86_64/multiarch/wcsnlen-c.c b/sysdeps/x86_64/multiarch/wcsnlen-c.c
index e1ec7cf..a2d1187 100644
--- a/sysdeps/x86_64/multiarch/wcsnlen-c.c
+++ b/sysdeps/x86_64/multiarch/wcsnlen-c.c
@@ -4,6 +4,13 @@
 # define WCSNLEN __wcsnlen_sse2
 
 extern __typeof (wcsnlen) __wcsnlen_sse2;
+
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcsnlen_sse2, __GI___wcsnlen, __wcsnlen_sse2);
+libc_hidden_def (__wcsnlen)
+# endif /* SHARED */
 #endif
 
 #include "wcsmbs/wcsnlen.c"
diff --git a/wcsmbs/wcpncpy.c b/wcsmbs/wcpncpy.c
index 42c17ea..8b49d8b 100644
--- a/wcsmbs/wcpncpy.c
+++ b/wcsmbs/wcpncpy.c
@@ -83,5 +83,6 @@ __wcpncpy (wchar_t *dest, const wchar_t *src, size_t n)
 }
 
 #ifndef WCPNCPY
+libc_hidden_def (__wcpncpy)
 weak_alias (__wcpncpy, wcpncpy)
 #endif
diff --git a/wcsmbs/wcscat.c b/wcsmbs/wcscat.c
index d567b73..1f9187b 100644
--- a/wcsmbs/wcscat.c
+++ b/wcsmbs/wcscat.c
@@ -49,5 +49,6 @@ __wcscat (wchar_t *dest, const wchar_t *src)
   return dest;
 }
 #ifndef WCSCAT
+libc_hidden_def (__wcscat)
 weak_alias (__wcscat, wcscat)
 #endif
diff --git a/wcsmbs/wcschrnul.c b/wcsmbs/wcschrnul.c
index 67ddb1a..fed10b3 100644
--- a/wcsmbs/wcschrnul.c
+++ b/wcsmbs/wcschrnul.c
@@ -34,5 +34,6 @@ __wcschrnul (const wchar_t *wcs, const wchar_t wc)
   return (wchar_t *) wcs;
 }
 #ifndef WCSCHRNUL
+libc_hidden_def (__wcschrnul)
 weak_alias (__wcschrnul, wcschrnul)
 #endif
diff --git a/wcsmbs/wcsncpy.c b/wcsmbs/wcsncpy.c
index 27bfb0d..45175ee 100644
--- a/wcsmbs/wcsncpy.c
+++ b/wcsmbs/wcsncpy.c
@@ -84,5 +84,6 @@ __wcsncpy (wchar_t *dest, const wchar_t *src, size_t n)
   return s;
 }
 #ifndef WCSNCPY
+libc_hidden_def (__wcsncpy)
 weak_alias (__wcsncpy, wcsncpy)
 #endif
diff --git a/wcsmbs/wcsnlen.c b/wcsmbs/wcsnlen.c
index 21bb6db..cb9aff8 100644
--- a/wcsmbs/wcsnlen.c
+++ b/wcsmbs/wcsnlen.c
@@ -46,5 +46,6 @@ __wcsnlen (const wchar_t *s, size_t maxlen)
   return len;
 }
 #ifndef WCSNLEN
+libc_hidden_def (__wcsnlen)
 weak_alias (__wcsnlen, wcsnlen)
 #endif
Stefan Liebler Nov. 13, 2017, 5:11 p.m. | #4
On 11/13/2017 02:58 PM, H.J. Lu wrote:
> On Mon, Nov 13, 2017 at 3:55 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 11/13/2017 11:16 AM, Stefan Liebler wrote:
>>>
>>> This patch adds the libc_hidden_proto / libc_hidden_def construct.
>>> Then the __GI_* symbols are the default-ifunc-variants which can be called
>>> without PLT.
>>
>>
>> attribute_hidden and *_hidden_{proto,def} conflict on some architectures.
>> You need to remove attribute_hidden as part of this change.
> 
> That is true.  On i686, a hidden IFUNC function inside libc.so must be compiled
> with -fPIC via PLT since EBX must be loaded with GOT first.   This
> isn't an issue
> for x86-64 since PLT uses PC-relative addressing.  In this case, we
> should remove
> hidden attribute, instead of using __GI_* symbols, if we sill want to use IFUNC
> inside libc.so.
On s390/s390x, just removing attribute_hidden in wchar.h is fine, too. 
Then r12 will be setup with the GOT-pointer and IFUNC will be used 
inside libc.so.

What was your original intention? Getting rid of "PLT" or "PLT and 
IFUNC" for calls inside libc.so?

> 
> Now I have question, is there a way to apply attribute_hidden to a function
> depending on architecture? For example, we remove attribute_hidden
> from __wcsnlen, __wcscat, __wcsncpy, __wcpncpy,  __wcschrnul in headers
> under include/.   For x86, we mark them hidden in a header file under
> sysdeps/x86?
I don't know if duplicating the wchar.h file is such a good idea.

Bye
Stefan
H.J. Lu Nov. 13, 2017, 5:30 p.m. | #5
On Mon, Nov 13, 2017 at 9:11 AM, Stefan Liebler <stli@linux.vnet.ibm.com> wrote:
> On 11/13/2017 02:58 PM, H.J. Lu wrote:
>>
>> On Mon, Nov 13, 2017 at 3:55 AM, Florian Weimer <fweimer@redhat.com>
>> wrote:
>>>
>>> On 11/13/2017 11:16 AM, Stefan Liebler wrote:
>>>>
>>>>
>>>> This patch adds the libc_hidden_proto / libc_hidden_def construct.
>>>> Then the __GI_* symbols are the default-ifunc-variants which can be
>>>> called
>>>> without PLT.
>>>
>>>
>>>
>>> attribute_hidden and *_hidden_{proto,def} conflict on some architectures.
>>> You need to remove attribute_hidden as part of this change.
>>
>>
>> That is true.  On i686, a hidden IFUNC function inside libc.so must be
>> compiled
>> with -fPIC via PLT since EBX must be loaded with GOT first.   This
>> isn't an issue
>> for x86-64 since PLT uses PC-relative addressing.  In this case, we
>> should remove
>> hidden attribute, instead of using __GI_* symbols, if we sill want to use
>> IFUNC
>> inside libc.so.
>
> On s390/s390x, just removing attribute_hidden in wchar.h is fine, too. Then
> r12 will be setup with the GOT-pointer and IFUNC will be used inside
> libc.so.
>
> What was your original intention? Getting rid of "PLT" or "PLT and IFUNC"
> for calls inside libc.so?

The original intention is to remove PLT.  But it doesn't work for targets which
need to set up a special register for PLT which is required by IFUNC.

>>
>> Now I have question, is there a way to apply attribute_hidden to a
>> function
>> depending on architecture? For example, we remove attribute_hidden
>> from __wcsnlen, __wcscat, __wcsncpy, __wcpncpy,  __wcschrnul in headers
>> under include/.   For x86, we mark them hidden in a header file under
>> sysdeps/x86?
>
> I don't know if duplicating the wchar.h file is such a good idea.
>

Can we add

#include <wcharP.h>

to include/wchar.h and add a dummy sysdeps/generic/wcharP.h?  Then
I can add sysdeps/x86_64/wcharP.h to hide __wcsnlen, __wcscat, __wcsncpy,
_wcpncpy,  __wcschrnul
H.J. Lu Nov. 13, 2017, 8:13 p.m. | #6
On Mon, Nov 13, 2017 at 5:58 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Nov 13, 2017 at 3:55 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 11/13/2017 11:16 AM, Stefan Liebler wrote:
>>>
>>> This patch adds the libc_hidden_proto / libc_hidden_def construct.
>>> Then the __GI_* symbols are the default-ifunc-variants which can be called
>>> without PLT.
>>
>>
>> attribute_hidden and *_hidden_{proto,def} conflict on some architectures.
>> You need to remove attribute_hidden as part of this change.
>
> That is true.  On i686, a hidden IFUNC function inside libc.so must be compiled
> with -fPIC via PLT since EBX must be loaded with GOT first.   This
> isn't an issue
> for x86-64 since PLT uses PC-relative addressing.  In this case, we
> should remove
> hidden attribute, instead of using __GI_* symbols, if we sill want to use IFUNC
> inside libc.so.
>

BTW, i386 linker checks invalid PLT usage for IFUNC:

[hjl@gnu-6 tmp]$ cat x.s
.text
.globl bar
.type bar, @function
bar:
jmp foo
.size bar, .-bar
.hidden foo
.type foo, %gnu_indirect_function
.globl foo
foo:
ret
.size foo, .-foo
[hjl@gnu-6 tmp]$ gcc -m32 -c x.s
[hjl@gnu-6 tmp]$ ld -m elf_i386 -shared x.o
ld: x.o: unsupported non-PIC call to IFUNC `foo'
ld: final link failed: Nonrepresentable section on output
[hjl@gnu-6 tmp]$ gcc  -c x.s
[hjl@gnu-6 tmp]$ ld  -shared x.o
[hjl@gnu-6 tmp]$

since i386 uses a different relocation,  R_386_PLT32, for PLT.    We can
catch this issue at link-time.  Can you update 390 linker to do something
similar?  The relevant linker commit is

commit 74437ea28fb611d4c88077b486fd7c0a8b4c2a25
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Mon Aug 29 08:12:59 2016 -0700

    i386: Issue an error on non-PIC call to IFUNC in PIC object

    On i386, IFUNC function must be called via PLT.  Since PLT in PIC
    object uses EBX register, R_386_PLT32 relocation must be used to
    call IFUNC function even when IFUNC function is defined locally.
    Linker should issue an error when R_386_PC32 relocation is used
    to call IFUNC function.

    Since PR ld/19784 tests doesn't use PLT relocation to local IFUNC
    function, they are moved to the x86-64 test directory.

    bfd/

            PR ld/14961
            PR ld/20515
            * elf32-i386.c (elf_i386_check_relocs): Issue an error when
            R_386_PC32 relocation is used to call IFUNC function in PIC
            object.
Stefan Liebler Nov. 14, 2017, 12:45 p.m. | #7
On 11/13/2017 09:13 PM, H.J. Lu wrote:
> On Mon, Nov 13, 2017 at 5:58 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Mon, Nov 13, 2017 at 3:55 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> On 11/13/2017 11:16 AM, Stefan Liebler wrote:
>>>>
>>>> This patch adds the libc_hidden_proto / libc_hidden_def construct.
>>>> Then the __GI_* symbols are the default-ifunc-variants which can be called
>>>> without PLT.
>>>
>>>
>>> attribute_hidden and *_hidden_{proto,def} conflict on some architectures.
>>> You need to remove attribute_hidden as part of this change.
>>
>> That is true.  On i686, a hidden IFUNC function inside libc.so must be compiled
>> with -fPIC via PLT since EBX must be loaded with GOT first.   This
>> isn't an issue
>> for x86-64 since PLT uses PC-relative addressing.  In this case, we
>> should remove
>> hidden attribute, instead of using __GI_* symbols, if we sill want to use IFUNC
>> inside libc.so.
>>
> 
> BTW, i386 linker checks invalid PLT usage for IFUNC:
> 
> [hjl@gnu-6 tmp]$ cat x.s
> .text
> .globl bar
> .type bar, @function
> bar:
> jmp foo
> .size bar, .-bar
> .hidden foo
> .type foo, %gnu_indirect_function
> .globl foo
> foo:
> ret
> .size foo, .-foo
> [hjl@gnu-6 tmp]$ gcc -m32 -c x.s
> [hjl@gnu-6 tmp]$ ld -m elf_i386 -shared x.o
> ld: x.o: unsupported non-PIC call to IFUNC `foo'
> ld: final link failed: Nonrepresentable section on output
> [hjl@gnu-6 tmp]$ gcc  -c x.s
> [hjl@gnu-6 tmp]$ ld  -shared x.o
> [hjl@gnu-6 tmp]$
> 
> since i386 uses a different relocation,  R_386_PLT32, for PLT.    We can
> catch this issue at link-time.  Can you update 390 linker to do something
> similar?  The relevant linker commit is
> 

Thanks for this hint. I've forwarded this mail to Andreas Krebbel. He 
will have a look into binutils.

> commit 74437ea28fb611d4c88077b486fd7c0a8b4c2a25
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date:   Mon Aug 29 08:12:59 2016 -0700
> 
>      i386: Issue an error on non-PIC call to IFUNC in PIC object
> 
>      On i386, IFUNC function must be called via PLT.  Since PLT in PIC
>      object uses EBX register, R_386_PLT32 relocation must be used to
>      call IFUNC function even when IFUNC function is defined locally.
>      Linker should issue an error when R_386_PC32 relocation is used
>      to call IFUNC function.
> 
>      Since PR ld/19784 tests doesn't use PLT relocation to local IFUNC
>      function, they are moved to the x86-64 test directory.
> 
>      bfd/
> 
>              PR ld/14961
>              PR ld/20515
>              * elf32-i386.c (elf_i386_check_relocs): Issue an error when
>              R_386_PC32 relocation is used to call IFUNC function in PIC
>              object.
> 
>
Stefan Liebler Nov. 14, 2017, 12:45 p.m. | #8
On 11/13/2017 06:30 PM, H.J. Lu wrote:
> On Mon, Nov 13, 2017 at 9:11 AM, Stefan Liebler <stli@linux.vnet.ibm.com> wrote:
>> On 11/13/2017 02:58 PM, H.J. Lu wrote:
>>>
>>> On Mon, Nov 13, 2017 at 3:55 AM, Florian Weimer <fweimer@redhat.com>
>>> wrote:
>>>>
>>>> On 11/13/2017 11:16 AM, Stefan Liebler wrote:
>>>>>
>>>>>
>>>>> This patch adds the libc_hidden_proto / libc_hidden_def construct.
>>>>> Then the __GI_* symbols are the default-ifunc-variants which can be
>>>>> called
>>>>> without PLT.
>>>>
>>>>
>>>>
>>>> attribute_hidden and *_hidden_{proto,def} conflict on some architectures.
>>>> You need to remove attribute_hidden as part of this change.
>>>
>>>
>>> That is true.  On i686, a hidden IFUNC function inside libc.so must be
>>> compiled
>>> with -fPIC via PLT since EBX must be loaded with GOT first.   This
>>> isn't an issue
>>> for x86-64 since PLT uses PC-relative addressing.  In this case, we
>>> should remove
>>> hidden attribute, instead of using __GI_* symbols, if we sill want to use
>>> IFUNC
>>> inside libc.so.
>>
>> On s390/s390x, just removing attribute_hidden in wchar.h is fine, too. Then
>> r12 will be setup with the GOT-pointer and IFUNC will be used inside
>> libc.so.
>>
>> What was your original intention? Getting rid of "PLT" or "PLT and IFUNC"
>> for calls inside libc.so?
> 
> The original intention is to remove PLT.  But it doesn't work for targets which
> need to set up a special register for PLT which is required by IFUNC.
But even on x86_64, __wcsnlen is called via a PLT-stub (with 
attribute_hidden). This indirection is removed with the __GI_* symbols.
But then we don't have the advantage of the IFUNC variants.

>>>
>>> Now I have question, is there a way to apply attribute_hidden to a
>>> function
>>> depending on architecture? For example, we remove attribute_hidden
>>> from __wcsnlen, __wcscat, __wcsncpy, __wcpncpy,  __wcschrnul in headers
>>> under include/.   For x86, we mark them hidden in a header file under
>>> sysdeps/x86?
>>
>> I don't know if duplicating the wchar.h file is such a good idea.
>>
> 
> Can we add
> 
> #include <wcharP.h>
> 
> to include/wchar.h and add a dummy sysdeps/generic/wcharP.h?  Then
> I can add sysdeps/x86_64/wcharP.h to hide __wcsnlen, __wcscat, __wcsncpy,
> _wcpncpy,  __wcschrnul
> 
> 
I think we should ask Wilco, Zack, ... if an architecture dependent 
wcharP.h is possible or not. They spent some effort to cleanup the 
string-headers.
H.J. Lu Nov. 14, 2017, 1:08 p.m. | #9
On Tue, Nov 14, 2017 at 4:45 AM, Stefan Liebler <stli@linux.vnet.ibm.com> wrote:
> On 11/13/2017 09:13 PM, H.J. Lu wrote:
>>
>> On Mon, Nov 13, 2017 at 5:58 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>
>>> On Mon, Nov 13, 2017 at 3:55 AM, Florian Weimer <fweimer@redhat.com>
>>> wrote:
>>>>
>>>> On 11/13/2017 11:16 AM, Stefan Liebler wrote:
>>>>>
>>>>>
>>>>> This patch adds the libc_hidden_proto / libc_hidden_def construct.
>>>>> Then the __GI_* symbols are the default-ifunc-variants which can be
>>>>> called
>>>>> without PLT.
>>>>
>>>>
>>>>
>>>> attribute_hidden and *_hidden_{proto,def} conflict on some
>>>> architectures.
>>>> You need to remove attribute_hidden as part of this change.
>>>
>>>
>>> That is true.  On i686, a hidden IFUNC function inside libc.so must be
>>> compiled
>>> with -fPIC via PLT since EBX must be loaded with GOT first.   This
>>> isn't an issue
>>> for x86-64 since PLT uses PC-relative addressing.  In this case, we
>>> should remove
>>> hidden attribute, instead of using __GI_* symbols, if we sill want to use
>>> IFUNC
>>> inside libc.so.
>>>
>>
>> BTW, i386 linker checks invalid PLT usage for IFUNC:
>>
>> [hjl@gnu-6 tmp]$ cat x.s
>> .text
>> .globl bar
>> .type bar, @function
>> bar:
>> jmp foo
>> .size bar, .-bar
>> .hidden foo
>> .type foo, %gnu_indirect_function
>> .globl foo
>> foo:
>> ret
>> .size foo, .-foo
>> [hjl@gnu-6 tmp]$ gcc -m32 -c x.s
>> [hjl@gnu-6 tmp]$ ld -m elf_i386 -shared x.o
>> ld: x.o: unsupported non-PIC call to IFUNC `foo'
>> ld: final link failed: Nonrepresentable section on output
>> [hjl@gnu-6 tmp]$ gcc  -c x.s
>> [hjl@gnu-6 tmp]$ ld  -shared x.o
>> [hjl@gnu-6 tmp]$
>>
>> since i386 uses a different relocation,  R_386_PLT32, for PLT.    We can
>> catch this issue at link-time.  Can you update 390 linker to do something
>> similar?  The relevant linker commit is
>>
>
> Thanks for this hint. I've forwarded this mail to Andreas Krebbel. He will
> have a look into binutils.

BTW, it will only be possible on binutils master branch where I changed
ELF linker to call check_relocs after we have seen all linker inputs.

>
>> commit 74437ea28fb611d4c88077b486fd7c0a8b4c2a25
>> Author: H.J. Lu <hjl.tools@gmail.com>
>> Date:   Mon Aug 29 08:12:59 2016 -0700
>>
>>      i386: Issue an error on non-PIC call to IFUNC in PIC object
>>
>>      On i386, IFUNC function must be called via PLT.  Since PLT in PIC
>>      object uses EBX register, R_386_PLT32 relocation must be used to
>>      call IFUNC function even when IFUNC function is defined locally.
>>      Linker should issue an error when R_386_PC32 relocation is used
>>      to call IFUNC function.
>>
>>      Since PR ld/19784 tests doesn't use PLT relocation to local IFUNC
>>      function, they are moved to the x86-64 test directory.
>>
>>      bfd/
>>
>>              PR ld/14961
>>              PR ld/20515
>>              * elf32-i386.c (elf_i386_check_relocs): Issue an error when
>>              R_386_PC32 relocation is used to call IFUNC function in PIC
>>              object.
>>
>>
>
Florian Weimer Nov. 14, 2017, 1:12 p.m. | #10
On 11/13/2017 02:58 PM, H.J. Lu wrote:

> Now I have question, is there a way to apply attribute_hidden to a function
> depending on architecture? For example, we remove attribute_hidden
> from __wcsnlen, __wcscat, __wcsncpy, __wcpncpy,  __wcschrnul in headers
> under include/.   For x86, we mark them hidden in a header file under
> sysdeps/x86?

We touched this briefly before, when I suggested that we need to 
automate this.  But Joseph thought it wouldn't be possible:

   <https://sourceware.org/ml/libc-alpha/2017-09/msg00113.html>

I don't know if the IFUNC vs hidden symbol issues changes matters 
significantly.  I still don't think it's a good use of our time to 
predict in generic headers what the linker may need in the end, as the 
result of what happens to be emitted by the compiler and assembler.

I'm also not sure if the existing GCC attributes give us sufficient 
control.  I think we need:

   (i)   local call within the current DSO, non-IFUNC implementation
   (ii)  local call within the current DSO, IFUNC implementation
   (iii) non-local call
     (1a) without symbol interposition
     (1b) with symbol interposition (for malloc/free/…)
     and (second choice)
     (2a) through PLT (without --enable-bind-now)
     (2b) without PLT (with --enable-bind-now and recent toolchain)

(ii) looks very similar to (iii)+(1a)+(2b).  But I don't know if that's 
true for all architectures.

Thanks,
Florian
Stefan Liebler Nov. 20, 2017, 3:51 p.m. | #11
On 11/13/2017 06:30 PM, H.J. Lu wrote:
> On Mon, Nov 13, 2017 at 9:11 AM, Stefan Liebler <stli@linux.vnet.ibm.com> wrote:
>> On 11/13/2017 02:58 PM, H.J. Lu wrote:
>>>
>>> On Mon, Nov 13, 2017 at 3:55 AM, Florian Weimer <fweimer@redhat.com>
>>> wrote:
>>>>
>>>> On 11/13/2017 11:16 AM, Stefan Liebler wrote:
>>>>>
>>>>>
>>>>> This patch adds the libc_hidden_proto / libc_hidden_def construct.
>>>>> Then the __GI_* symbols are the default-ifunc-variants which can be
>>>>> called
>>>>> without PLT.
>>>>
>>>>
>>>>
>>>> attribute_hidden and *_hidden_{proto,def} conflict on some architectures.
>>>> You need to remove attribute_hidden as part of this change.
>>>
>>>
>>> That is true.  On i686, a hidden IFUNC function inside libc.so must be
>>> compiled
>>> with -fPIC via PLT since EBX must be loaded with GOT first.   This
>>> isn't an issue
>>> for x86-64 since PLT uses PC-relative addressing.  In this case, we
>>> should remove
>>> hidden attribute, instead of using __GI_* symbols, if we sill want to use
>>> IFUNC
>>> inside libc.so.
For the moment, I propose to remove the attribute_hidden in order to fix 
those internal IFUNC calls.
Is the attached patch okay to commit?

>>
>> On s390/s390x, just removing attribute_hidden in wchar.h is fine, too. Then
>> r12 will be setup with the GOT-pointer and IFUNC will be used inside
>> libc.so.
>>
>> What was your original intention? Getting rid of "PLT" or "PLT and IFUNC"
>> for calls inside libc.so?
> 
> The original intention is to remove PLT.  But it doesn't work for targets which
> need to set up a special register for PLT which is required by IFUNC.
> 
>>>
>>> Now I have question, is there a way to apply attribute_hidden to a
>>> function
>>> depending on architecture? For example, we remove attribute_hidden
>>> from __wcsnlen, __wcscat, __wcsncpy, __wcpncpy,  __wcschrnul in headers
>>> under include/.   For x86, we mark them hidden in a header file under
>>> sysdeps/x86?
>>
>> I don't know if duplicating the wchar.h file is such a good idea.
>>
> 
> Can we add
> 
> #include <wcharP.h>
> 
> to include/wchar.h and add a dummy sysdeps/generic/wcharP.h?  Then
> I can add sysdeps/x86_64/wcharP.h to hide __wcsnlen, __wcscat, __wcsncpy,
> _wcpncpy,  __wcschrnul
> 
> 
@H.J. Lu: Can you propose a separate patch for architecture dependent 
wcharP.h files?

Bye.
Stefan
commit 70c17e8f6a07236aaa37e1aab67703d3bc1e09a2
Author: Stefan Liebler <stli@linux.vnet.ibm.com>
Date:   Mon Nov 20 12:26:24 2017 +0100

    Remove attribute_hidden for wchar ifunc symbols.
    
    On s390 (31bit) various debug/tst-*chk* testcases are failing as the tests
    are ending with a segmentation fault.
    
    One test is e.g. calling wcsnrtombs in debug/tst-chk1.c:1549.
    The function wcsnrtombs itself calls __wcsnlen. This function is called via
    PLT! The PLT-stub itself loads the address from GOT (r12 is assumed to be
    the GOT-pointer). In this case the loaded address is zero and the following
    branch leads to the segmentation fault.
    
    Due to the attribute_hidden in commit 44af8a32c341672b5160fdc2839767e9a837ad26
    "Mark internal wchar functions with attribute_hidden [BZ #18822]"
    for e.g. the __wcsnlen function, r12 is not loaded with the GOT-pointer
    in wcsnrtombs.
    
    On s390x (64bit), this __wcsnlen call is also using the PLT-stub. But it is
    not failing as the GOT-pointer is setup with larl-instruction by the PLT-stub
    itself.
    Note: On s390x/s390, __wcsnlen is an IFUNC symbol.
    
    On x86_64, __wcsnlen is also an IFUNC symbol and is called via PLT, too.
    
    Further IFUNC symbols on s390 which were marked as hidden by the mentioned
    commit are: __wcscat, __wcsncpy, __wcpncpy, __wcschrnul.
    
    This patch removes the attribute_hidden in wchar.h.
    Then the compiler setups e.g. r12 on s390 in order to call __wcsnlen via PLT.
    
    ChangeLog:
    
    	* include/wchar.h (__wcsnlen, __wcscat, __wcsncpy, __wcpncpy,
    	__wcschrnul): Remove attribute_hidden.

diff --git a/include/wchar.h b/include/wchar.h
index 24b2eaa..1db0ac8 100644
--- a/include/wchar.h
+++ b/include/wchar.h
@@ -152,9 +152,8 @@ extern int __wcsncasecmp (const wchar_t *__s1, const wchar_t *__s2,
      __attribute_pure__;
 extern size_t __wcslen (const wchar_t *__s) __attribute_pure__;
 extern size_t __wcsnlen (const wchar_t *__s, size_t __maxlen)
-     attribute_hidden __attribute_pure__;
-extern wchar_t *__wcscat (wchar_t *dest, const wchar_t *src)
-     attribute_hidden;
+     __attribute_pure__;
+extern wchar_t *__wcscat (wchar_t *dest, const wchar_t *src);
 extern wint_t __btowc (int __c) attribute_hidden;
 extern int __mbsinit (const __mbstate_t *__ps);
 extern size_t __mbrtowc (wchar_t *__restrict __pwc,
@@ -182,11 +181,10 @@ extern size_t __wcsnrtombs (char *__restrict __dst,
 			    __mbstate_t *__restrict __ps)
      attribute_hidden;
 extern wchar_t *__wcsncpy (wchar_t *__restrict __dest,
-			   const wchar_t *__restrict __src, size_t __n)
-     attribute_hidden;
+			   const wchar_t *__restrict __src, size_t __n);
 extern wchar_t *__wcpcpy (wchar_t *__dest, const wchar_t *__src);
 extern wchar_t *__wcpncpy (wchar_t *__dest, const wchar_t *__src,
-			   size_t __n) attribute_hidden;
+			   size_t __n);
 extern wchar_t *__wmemcpy (wchar_t *__s1, const wchar_t *s2,
 			   size_t __n) attribute_hidden;
 extern wchar_t *__wmempcpy (wchar_t *__restrict __s1,
@@ -195,7 +193,7 @@ extern wchar_t *__wmempcpy (wchar_t *__restrict __s1,
 extern wchar_t *__wmemmove (wchar_t *__s1, const wchar_t *__s2,
 			    size_t __n) attribute_hidden;
 extern wchar_t *__wcschrnul (const wchar_t *__s, wchar_t __wc)
-     attribute_hidden __attribute_pure__;
+     __attribute_pure__;
 
 extern wchar_t *__wmemset_chk (wchar_t *__s, wchar_t __c, size_t __n,
 			       size_t __ns) __THROW;
H.J. Lu Nov. 20, 2017, 3:59 p.m. | #12
On Mon, Nov 20, 2017 at 7:51 AM, Stefan Liebler <stli@linux.vnet.ibm.com> wrote:
> On 11/13/2017 06:30 PM, H.J. Lu wrote:
>>
>> On Mon, Nov 13, 2017 at 9:11 AM, Stefan Liebler <stli@linux.vnet.ibm.com>
>> wrote:
>>>
>>> On 11/13/2017 02:58 PM, H.J. Lu wrote:
>>>>
>>>>
>>>> On Mon, Nov 13, 2017 at 3:55 AM, Florian Weimer <fweimer@redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 11/13/2017 11:16 AM, Stefan Liebler wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> This patch adds the libc_hidden_proto / libc_hidden_def construct.
>>>>>> Then the __GI_* symbols are the default-ifunc-variants which can be
>>>>>> called
>>>>>> without PLT.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> attribute_hidden and *_hidden_{proto,def} conflict on some
>>>>> architectures.
>>>>> You need to remove attribute_hidden as part of this change.
>>>>
>>>>
>>>>
>>>> That is true.  On i686, a hidden IFUNC function inside libc.so must be
>>>> compiled
>>>> with -fPIC via PLT since EBX must be loaded with GOT first.   This
>>>> isn't an issue
>>>> for x86-64 since PLT uses PC-relative addressing.  In this case, we
>>>> should remove
>>>> hidden attribute, instead of using __GI_* symbols, if we sill want to
>>>> use
>>>> IFUNC
>>>> inside libc.so.
>
> For the moment, I propose to remove the attribute_hidden in order to fix
> those internal IFUNC calls.
> Is the attached patch okay to commit?

LGTM.

>>>
>>> On s390/s390x, just removing attribute_hidden in wchar.h is fine, too.
>>> Then
>>> r12 will be setup with the GOT-pointer and IFUNC will be used inside
>>> libc.so.
>>>
>>> What was your original intention? Getting rid of "PLT" or "PLT and IFUNC"
>>> for calls inside libc.so?
>>
>>
>> The original intention is to remove PLT.  But it doesn't work for targets
>> which
>> need to set up a special register for PLT which is required by IFUNC.
>>
>>>>
>>>> Now I have question, is there a way to apply attribute_hidden to a
>>>> function
>>>> depending on architecture? For example, we remove attribute_hidden
>>>> from __wcsnlen, __wcscat, __wcsncpy, __wcpncpy,  __wcschrnul in headers
>>>> under include/.   For x86, we mark them hidden in a header file under
>>>> sysdeps/x86?
>>>
>>>
>>> I don't know if duplicating the wchar.h file is such a good idea.
>>>
>>
>> Can we add
>>
>> #include <wcharP.h>
>>
>> to include/wchar.h and add a dummy sysdeps/generic/wcharP.h?  Then
>> I can add sysdeps/x86_64/wcharP.h to hide __wcsnlen, __wcscat, __wcsncpy,
>> _wcpncpy,  __wcschrnul
>>
>>
> @H.J. Lu: Can you propose a separate patch for architecture dependent
> wcharP.h files?

Sure.  I will submit one after your patch is checked in.
Stefan Liebler Nov. 21, 2017, 7:48 a.m. | #13
On 11/20/2017 04:59 PM, H.J. Lu wrote:
> On Mon, Nov 20, 2017 at 7:51 AM, Stefan Liebler <stli@linux.vnet.ibm.com> wrote:
>> On 11/13/2017 06:30 PM, H.J. Lu wrote:
>>>
>>> On Mon, Nov 13, 2017 at 9:11 AM, Stefan Liebler <stli@linux.vnet.ibm.com>
>>> wrote:
>>>>
>>>> On 11/13/2017 02:58 PM, H.J. Lu wrote:
>>>>>
>>>>>
>>>>> On Mon, Nov 13, 2017 at 3:55 AM, Florian Weimer <fweimer@redhat.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 11/13/2017 11:16 AM, Stefan Liebler wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> This patch adds the libc_hidden_proto / libc_hidden_def construct.
>>>>>>> Then the __GI_* symbols are the default-ifunc-variants which can be
>>>>>>> called
>>>>>>> without PLT.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> attribute_hidden and *_hidden_{proto,def} conflict on some
>>>>>> architectures.
>>>>>> You need to remove attribute_hidden as part of this change.
>>>>>
>>>>>
>>>>>
>>>>> That is true.  On i686, a hidden IFUNC function inside libc.so must be
>>>>> compiled
>>>>> with -fPIC via PLT since EBX must be loaded with GOT first.   This
>>>>> isn't an issue
>>>>> for x86-64 since PLT uses PC-relative addressing.  In this case, we
>>>>> should remove
>>>>> hidden attribute, instead of using __GI_* symbols, if we sill want to
>>>>> use
>>>>> IFUNC
>>>>> inside libc.so.
>>
>> For the moment, I propose to remove the attribute_hidden in order to fix
>> those internal IFUNC calls.
>> Is the attached patch okay to commit?
> 
> LGTM.
> 
Committed.

>>>>
>>>> On s390/s390x, just removing attribute_hidden in wchar.h is fine, too.
>>>> Then
>>>> r12 will be setup with the GOT-pointer and IFUNC will be used inside
>>>> libc.so.
>>>>
>>>> What was your original intention? Getting rid of "PLT" or "PLT and IFUNC"
>>>> for calls inside libc.so?
>>>
>>>
>>> The original intention is to remove PLT.  But it doesn't work for targets
>>> which
>>> need to set up a special register for PLT which is required by IFUNC.
>>>
>>>>>
>>>>> Now I have question, is there a way to apply attribute_hidden to a
>>>>> function
>>>>> depending on architecture? For example, we remove attribute_hidden
>>>>> from __wcsnlen, __wcscat, __wcsncpy, __wcpncpy,  __wcschrnul in headers
>>>>> under include/.   For x86, we mark them hidden in a header file under
>>>>> sysdeps/x86?
>>>>
>>>>
>>>> I don't know if duplicating the wchar.h file is such a good idea.
>>>>
>>>
>>> Can we add
>>>
>>> #include <wcharP.h>
>>>
>>> to include/wchar.h and add a dummy sysdeps/generic/wcharP.h?  Then
>>> I can add sysdeps/x86_64/wcharP.h to hide __wcsnlen, __wcscat, __wcsncpy,
>>> _wcpncpy,  __wcschrnul
>>>
>>>
>> @H.J. Lu: Can you propose a separate patch for architecture dependent
>> wcharP.h files?
> 
> Sure.  I will submit one after your patch is checked in.
> 
>

Patch

commit f4beed7e3d6e575c289124c634c8eea45638765e
Author: Stefan Liebler <stli@linux.vnet.ibm.com>
Date:   Mon Nov 13 11:05:24 2017 +0100

    Use libc_hidden_proto / _def for hidden wchar ifunc symbols.
    
    On s390 (31bit) various debug/tst-*chk* testcases are failing as the tests
    are ending with a segmentation fault.
    
    One test is e.g. calling wcsnrtombs in debug/tst-chk1.c:1549.
    The function wcsnrtombs itself calls __wcsnlen. This function is called via
    PLT! The PLT-stub itself loads the address from GOT (r12 is assumed to be
    the GOT-pointer). In this case the loaded address is zero and the following
    branch leads to the segmentation fault.
    
    Due to the attribute-hidden in commit 44af8a32c341672b5160fdc2839767e9a837ad26
    "Mark internal wchar functions with attribute_hidden [BZ #18822]"
    for e.g. the __wcsnlen function, r12 is not loaded with the GOT-pointer
    in wcsnrtombs.
    
    On s390x (64bit), this __wcsnlen call is also using the PLT-stub. But it is
    not failing as the GOT-pointer is setup with larl-instruction by the PLT-stub
    itself.
    Note: On s390x/s390, __wcsnlen is an IFUNC symbol.
    
    On x86_64, __wcsnlen is also an IFUNC symbol and is called via PLT, too.
    
    Further IFUNC symbols on s390 which were marked as hidden by the mentioned
    commit are: __wcscat, __wcsncpy, __wcpncpy, __wcschrnul.
    
    This patch adds the libc_hidden_proto / libc_hidden_def construct.
    Then the __GI_* symbols are the default-ifunc-variants which can be called
    without PLT.
    
    ChangeLog:
    
    	* include/wchar.h (__wcsnlen, __wcscat, __wcsncpy, __wcpncpy,
    	__wcschrnul): Add libc_hidden_proto.
    	* wcsmbs/wcsnlen.c (__wcsnlen): Add libc_hidden_def.
    	* wcsmbs/wcscat.c (__wcscat): Likewise.
    	* wcsmbs/wcschrnul.c (__wcschrnul): Likewise.
    	* wcsmbs/wcsncpy.c (__wcsncpy): Likewise.
    	* wcsmbs/wcpncpy.c (__wcpncpy): Likewise.
    	* sysdeps/x86_64/multiarch/wcsnlen-c.c (__wcsnlen): Add libc_hidden_def
    	which aliases to default ifunc-variant.
    	* sysdeps/s390/multiarch/wcsnlen-c.c (__wcsnlen):  Likewise.
    	* sysdeps/s390/multiarch/wcscat-c.c (__wcscat): Likewise.
    	* sysdeps/s390/multiarch/wcschrnul-c.c  (__wcschrnul): Likewise.
    	* sysdeps/s390/multiarch/wcsncpy-c.c (__wcsncpy): Likewise.
    	* sysdeps/s390/multiarch/wcpncpy-c.c (__wcpncpy): Likewise.
    	* sysdeps/s390/multiarch/wcsnlen.c (__wcsnlen): Redirect symbol in
    	header and use s390_vx_libc_ifunc_redirected instead of
    	s390_vx_libc_ifunc.
    	* sysdeps/s390/multiarch/wcscat.c (__wcscat): Likewise.
    	* sysdeps/s390/multiarch/wcschrnul.c (__wcschrnul): Likewise.
    	* sysdeps/s390/multiarch/wcsncpy.c (__wcsncpy): Likewise.
    	* sysdeps/s390/multiarch/wcpncpy.c (__wcpncpy): Likewise.

diff --git a/include/wchar.h b/include/wchar.h
index 24b2eaa..0719df2 100644
--- a/include/wchar.h
+++ b/include/wchar.h
@@ -153,8 +153,10 @@  extern int __wcsncasecmp (const wchar_t *__s1, const wchar_t *__s2,
 extern size_t __wcslen (const wchar_t *__s) __attribute_pure__;
 extern size_t __wcsnlen (const wchar_t *__s, size_t __maxlen)
      attribute_hidden __attribute_pure__;
+libc_hidden_proto (__wcsnlen)
 extern wchar_t *__wcscat (wchar_t *dest, const wchar_t *src)
      attribute_hidden;
+libc_hidden_proto (__wcscat)
 extern wint_t __btowc (int __c) attribute_hidden;
 extern int __mbsinit (const __mbstate_t *__ps);
 extern size_t __mbrtowc (wchar_t *__restrict __pwc,
@@ -184,9 +186,11 @@  extern size_t __wcsnrtombs (char *__restrict __dst,
 extern wchar_t *__wcsncpy (wchar_t *__restrict __dest,
 			   const wchar_t *__restrict __src, size_t __n)
      attribute_hidden;
+libc_hidden_proto (__wcsncpy)
 extern wchar_t *__wcpcpy (wchar_t *__dest, const wchar_t *__src);
 extern wchar_t *__wcpncpy (wchar_t *__dest, const wchar_t *__src,
 			   size_t __n) attribute_hidden;
+libc_hidden_proto (__wcpncpy)
 extern wchar_t *__wmemcpy (wchar_t *__s1, const wchar_t *s2,
 			   size_t __n) attribute_hidden;
 extern wchar_t *__wmempcpy (wchar_t *__restrict __s1,
@@ -196,6 +200,7 @@  extern wchar_t *__wmemmove (wchar_t *__s1, const wchar_t *__s2,
 			    size_t __n) attribute_hidden;
 extern wchar_t *__wcschrnul (const wchar_t *__s, wchar_t __wc)
      attribute_hidden __attribute_pure__;
+libc_hidden_proto (__wcschrnul)
 
 extern wchar_t *__wmemset_chk (wchar_t *__s, wchar_t __c, size_t __n,
 			       size_t __ns) __THROW;
diff --git a/sysdeps/s390/multiarch/wcpncpy-c.c b/sysdeps/s390/multiarch/wcpncpy-c.c
index 7cb834b..b02e40a 100644
--- a/sysdeps/s390/multiarch/wcpncpy-c.c
+++ b/sysdeps/s390/multiarch/wcpncpy-c.c
@@ -21,5 +21,11 @@ 
 
 # include <wchar.h>
 extern __typeof (__wcpncpy) __wcpncpy_c;
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcpncpy_c, __GI___wcpncpy, __wcpncpy_c);
+# endif /* SHARED */
 # include <wcsmbs/wcpncpy.c>
+libc_hidden_def (__wcpncpy)
 #endif
diff --git a/sysdeps/s390/multiarch/wcpncpy.c b/sysdeps/s390/multiarch/wcpncpy.c
index 1a19a99..a902787 100644
--- a/sysdeps/s390/multiarch/wcpncpy.c
+++ b/sysdeps/s390/multiarch/wcpncpy.c
@@ -17,10 +17,12 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #if defined HAVE_S390_VX_ASM_SUPPORT && IS_IN (libc)
+# define __wcpncpy __redirect___wcpncpy
 # include <wchar.h>
+# undef __wcpncpy
 # include <ifunc-resolve.h>
 
-s390_vx_libc_ifunc (__wcpncpy)
+s390_vx_libc_ifunc_redirected (__redirect___wcpncpy, __wcpncpy)
 weak_alias (__wcpncpy, wcpncpy)
 
 #else
diff --git a/sysdeps/s390/multiarch/wcscat-c.c b/sysdeps/s390/multiarch/wcscat-c.c
index ef7c000..3ac6110 100644
--- a/sysdeps/s390/multiarch/wcscat-c.c
+++ b/sysdeps/s390/multiarch/wcscat-c.c
@@ -21,5 +21,11 @@ 
 
 # include <wchar.h>
 extern __typeof (__wcscat) __wcscat_c;
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcscat_c, __GI___wcscat, __wcscat_c);
+# endif /* SHARED */
 # include <wcsmbs/wcscat.c>
+libc_hidden_def (__wcscat)
 #endif
diff --git a/sysdeps/s390/multiarch/wcscat.c b/sysdeps/s390/multiarch/wcscat.c
index 2e7fded..3f1219e 100644
--- a/sysdeps/s390/multiarch/wcscat.c
+++ b/sysdeps/s390/multiarch/wcscat.c
@@ -17,10 +17,12 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #if defined HAVE_S390_VX_ASM_SUPPORT && IS_IN (libc)
+# define __wcscat __redirect___wcscat
 # include <wchar.h>
+# undef __wcscat
 # include <ifunc-resolve.h>
 
-s390_vx_libc_ifunc (__wcscat)
+s390_vx_libc_ifunc_redirected (__redirect___wcscat, __wcscat)
 weak_alias (__wcscat, wcscat)
 
 #else
diff --git a/sysdeps/s390/multiarch/wcschrnul-c.c b/sysdeps/s390/multiarch/wcschrnul-c.c
index 095ea12..209a305 100644
--- a/sysdeps/s390/multiarch/wcschrnul-c.c
+++ b/sysdeps/s390/multiarch/wcschrnul-c.c
@@ -21,5 +21,11 @@ 
 
 # include <wchar.h>
 extern __typeof (__wcschrnul) __wcschrnul_c;
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcschrnul_c, __GI___wcschrnul, __wcschrnul_c);
+# endif /* SHARED */
 # include <wcsmbs/wcschrnul.c>
+libc_hidden_def (__wcschrnul)
 #endif
diff --git a/sysdeps/s390/multiarch/wcschrnul.c b/sysdeps/s390/multiarch/wcschrnul.c
index f01ea9f..c112cfb 100644
--- a/sysdeps/s390/multiarch/wcschrnul.c
+++ b/sysdeps/s390/multiarch/wcschrnul.c
@@ -17,10 +17,12 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #if defined HAVE_S390_VX_ASM_SUPPORT && IS_IN (libc)
+# define __wcschrnul __redirect___wcschrnul
 # include <wchar.h>
+# undef __wcschrnul
 # include <ifunc-resolve.h>
 
-s390_vx_libc_ifunc (__wcschrnul)
+s390_vx_libc_ifunc_redirected (__redirect___wcschrnul, __wcschrnul)
 weak_alias (__wcschrnul, wcschrnul)
 
 #else
diff --git a/sysdeps/s390/multiarch/wcsncpy-c.c b/sysdeps/s390/multiarch/wcsncpy-c.c
index 32ec8ff..d896220 100644
--- a/sysdeps/s390/multiarch/wcsncpy-c.c
+++ b/sysdeps/s390/multiarch/wcsncpy-c.c
@@ -21,5 +21,11 @@ 
 
 # include <wchar.h>
 extern __typeof (__wcsncpy) __wcsncpy_c;
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcsncpy_c, __GI___wcsncpy, __wcsncpy_c);
+# endif /* SHARED */
 # include <wcsmbs/wcsncpy.c>
+libc_hidden_def (__wcsncpy)
 #endif
diff --git a/sysdeps/s390/multiarch/wcsncpy.c b/sysdeps/s390/multiarch/wcsncpy.c
index 6e1e8f0..e08a82f 100644
--- a/sysdeps/s390/multiarch/wcsncpy.c
+++ b/sysdeps/s390/multiarch/wcsncpy.c
@@ -17,10 +17,12 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #if defined HAVE_S390_VX_ASM_SUPPORT && IS_IN (libc)
+# define __wcsncpy __redirect___wcsncpy
 # include <wchar.h>
+# undef __wcsncpy
 # include <ifunc-resolve.h>
 
-s390_vx_libc_ifunc (__wcsncpy)
+s390_vx_libc_ifunc_redirected (__redirect___wcsncpy, __wcsncpy)
 weak_alias (__wcsncpy, wcsncpy)
 
 #else
diff --git a/sysdeps/s390/multiarch/wcsnlen-c.c b/sysdeps/s390/multiarch/wcsnlen-c.c
index e86ca65..c6bc0cc 100644
--- a/sysdeps/s390/multiarch/wcsnlen-c.c
+++ b/sysdeps/s390/multiarch/wcsnlen-c.c
@@ -21,5 +21,11 @@ 
 
 # include <wchar.h>
 extern __typeof (__wcsnlen) __wcsnlen_c;
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcsnlen_c, __GI___wcsnlen, __wcsnlen_c);
+# endif /* SHARED */
 # include <wcsmbs/wcsnlen.c>
+libc_hidden_def (__wcsnlen)
 #endif
diff --git a/sysdeps/s390/multiarch/wcsnlen.c b/sysdeps/s390/multiarch/wcsnlen.c
index 54486b9..df29343 100644
--- a/sysdeps/s390/multiarch/wcsnlen.c
+++ b/sysdeps/s390/multiarch/wcsnlen.c
@@ -17,10 +17,12 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #if defined HAVE_S390_VX_ASM_SUPPORT && IS_IN (libc)
+# define __wcsnlen __redirect___wcsnlen
 # include <wchar.h>
+# undef __wcsnlen
 # include <ifunc-resolve.h>
 
-s390_vx_libc_ifunc (__wcsnlen)
+s390_vx_libc_ifunc_redirected (__redirect___wcsnlen, __wcsnlen)
 weak_alias (__wcsnlen, wcsnlen)
 
 #else
diff --git a/sysdeps/x86_64/multiarch/wcsnlen-c.c b/sysdeps/x86_64/multiarch/wcsnlen-c.c
index e1ec7cf..a2d1187 100644
--- a/sysdeps/x86_64/multiarch/wcsnlen-c.c
+++ b/sysdeps/x86_64/multiarch/wcsnlen-c.c
@@ -4,6 +4,13 @@ 
 # define WCSNLEN __wcsnlen_sse2
 
 extern __typeof (wcsnlen) __wcsnlen_sse2;
+
+# ifdef SHARED
+#  undef libc_hidden_def
+#  define libc_hidden_def(name)					\
+  __hidden_ver1 (__wcsnlen_sse2, __GI___wcsnlen, __wcsnlen_sse2);
+libc_hidden_def (__wcsnlen)
+# endif /* SHARED */
 #endif
 
 #include "wcsmbs/wcsnlen.c"
diff --git a/wcsmbs/wcpncpy.c b/wcsmbs/wcpncpy.c
index 42c17ea..8b49d8b 100644
--- a/wcsmbs/wcpncpy.c
+++ b/wcsmbs/wcpncpy.c
@@ -83,5 +83,6 @@  __wcpncpy (wchar_t *dest, const wchar_t *src, size_t n)
 }
 
 #ifndef WCPNCPY
+libc_hidden_def (__wcpncpy)
 weak_alias (__wcpncpy, wcpncpy)
 #endif
diff --git a/wcsmbs/wcscat.c b/wcsmbs/wcscat.c
index d567b73..1f9187b 100644
--- a/wcsmbs/wcscat.c
+++ b/wcsmbs/wcscat.c
@@ -49,5 +49,6 @@  __wcscat (wchar_t *dest, const wchar_t *src)
   return dest;
 }
 #ifndef WCSCAT
+libc_hidden_def (__wcscat)
 weak_alias (__wcscat, wcscat)
 #endif
diff --git a/wcsmbs/wcschrnul.c b/wcsmbs/wcschrnul.c
index 67ddb1a..fed10b3 100644
--- a/wcsmbs/wcschrnul.c
+++ b/wcsmbs/wcschrnul.c
@@ -34,5 +34,6 @@  __wcschrnul (const wchar_t *wcs, const wchar_t wc)
   return (wchar_t *) wcs;
 }
 #ifndef WCSCHRNUL
+libc_hidden_def (__wcschrnul)
 weak_alias (__wcschrnul, wcschrnul)
 #endif
diff --git a/wcsmbs/wcsncpy.c b/wcsmbs/wcsncpy.c
index 27bfb0d..45175ee 100644
--- a/wcsmbs/wcsncpy.c
+++ b/wcsmbs/wcsncpy.c
@@ -84,5 +84,6 @@  __wcsncpy (wchar_t *dest, const wchar_t *src, size_t n)
   return s;
 }
 #ifndef WCSNCPY
+libc_hidden_def (__wcsncpy)
 weak_alias (__wcsncpy, wcsncpy)
 #endif
diff --git a/wcsmbs/wcsnlen.c b/wcsmbs/wcsnlen.c
index 21bb6db..cb9aff8 100644
--- a/wcsmbs/wcsnlen.c
+++ b/wcsmbs/wcsnlen.c
@@ -46,5 +46,6 @@  __wcsnlen (const wchar_t *s, size_t maxlen)
   return len;
 }
 #ifndef WCSNLEN
+libc_hidden_def (__wcsnlen)
 weak_alias (__wcsnlen, wcsnlen)
 #endif