[PATCH] S390: Fix building with --disable-mutli-arch [BZ #31196]

[PATCH] S390: Fix building with --disable-mutli-arch [BZ #31196]

[Stefan Liebler]

Starting with commits
- 7ea510127e2067efa07865158ac92c330c379950
string: Add libc_hidden_proto for strchrnul
- 22999b2f0fb62eed1af4095d062bd1272d6afeb1
string: Add libc_hidden_proto for memrchr

building glibc on s390x with --disable-multi-arch fails if only
the C-variant of strchrnul / memrchr is used.  This is the case
if gcc uses -march < z13.

The build fails with:
../sysdeps/s390/strchrnul-c.c:28:49: error: ‘__strchrnul_c’ undeclared here (not in a function); did you mean ‘__strchrnul’?
   28 | __hidden_ver1 (__strchrnul_c, __GI___strchrnul, __strchrnul_c);

With --disable-multi-arch, __strchrnul_c is not available as string/strchrnul.c
is just included without defining STRCHRNUL and thus we also don't have to create
the internal hidden symbol.
---
 sysdeps/s390/memrchr-c.c   | 4 +++-
 sysdeps/s390/strchrnul-c.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/sysdeps/s390/memrchr-c.c b/sysdeps/s390/memrchr-c.c
index b4b85725d3..f6ffe85572 100644
--- a/sysdeps/s390/memrchr-c.c
+++ b/sysdeps/s390/memrchr-c.c
@@ -25,7 +25,9 @@
 
 # include <string/memrchr.c>
 
-# if defined SHARED && IS_IN (libc)
+# if HAVE_MEMRCHR_IFUNC
+#  if defined SHARED && IS_IN (libc)
 __hidden_ver1 (__memrchr_c, __GI___memrchr, __memrchr_c);
+#  endif
 # endif
 #endif
diff --git a/sysdeps/s390/strchrnul-c.c b/sysdeps/s390/strchrnul-c.c
index 7f2f1fd540..97fd9ffbf7 100644
--- a/sysdeps/s390/strchrnul-c.c
+++ b/sysdeps/s390/strchrnul-c.c
@@ -24,7 +24,9 @@
 # endif
 
 # include <string/strchrnul.c>
-# if defined SHARED && IS_IN (libc)
+# if HAVE_STRCHRNUL_IFUNC
+#  if defined SHARED && IS_IN (libc)
 __hidden_ver1 (__strchrnul_c, __GI___strchrnul, __strchrnul_c);
+#  endif
 # endif
 #endif
-- 
2.43.0

Re: [PATCH] S390: Fix building with --disable-mutli-arch [BZ #31196]

[Adhemerval Zanella Netto]

On 30/01/24 05:34, Stefan Liebler wrote:
> Starting with commits
> - 7ea510127e2067efa07865158ac92c330c379950
> string: Add libc_hidden_proto for strchrnul
> - 22999b2f0fb62eed1af4095d062bd1272d6afeb1
> string: Add libc_hidden_proto for memrchr
> 
> building glibc on s390x with --disable-multi-arch fails if only
> the C-variant of strchrnul / memrchr is used.  This is the case
> if gcc uses -march < z13.
> 
> The build fails with:
> ../sysdeps/s390/strchrnul-c.c:28:49: error: ‘__strchrnul_c’ undeclared here (not in a function); did you mean ‘__strchrnul’?
>    28 | __hidden_ver1 (__strchrnul_c, __GI___strchrnul, __strchrnul_c);
> 
> With --disable-multi-arch, __strchrnul_c is not available as string/strchrnul.c
> is just included without defining STRCHRNUL and thus we also don't have to create
> the internal hidden symbol.

LGTM, thanks.  In same topic, why s390 is not following the multiarch sysdep
folder organization followed by all others ports, where ifunc implementation
are put only in multiarch folder?

The multiarch mechanism was added to simplify this kind of code, the s390
seems gratuitous over complicated in this regard.

> ---
>  sysdeps/s390/memrchr-c.c   | 4 +++-
>  sysdeps/s390/strchrnul-c.c | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/s390/memrchr-c.c b/sysdeps/s390/memrchr-c.c
> index b4b85725d3..f6ffe85572 100644
> --- a/sysdeps/s390/memrchr-c.c
> +++ b/sysdeps/s390/memrchr-c.c
> @@ -25,7 +25,9 @@
>  
>  # include <string/memrchr.c>
>  
> -# if defined SHARED && IS_IN (libc)
> +# if HAVE_MEMRCHR_IFUNC
> +#  if defined SHARED && IS_IN (libc)
>  __hidden_ver1 (__memrchr_c, __GI___memrchr, __memrchr_c);
> +#  endif
>  # endif
>  #endif
> diff --git a/sysdeps/s390/strchrnul-c.c b/sysdeps/s390/strchrnul-c.c
> index 7f2f1fd540..97fd9ffbf7 100644
> --- a/sysdeps/s390/strchrnul-c.c
> +++ b/sysdeps/s390/strchrnul-c.c
> @@ -24,7 +24,9 @@
>  # endif
>  
>  # include <string/strchrnul.c>
> -# if defined SHARED && IS_IN (libc)
> +# if HAVE_STRCHRNUL_IFUNC
> +#  if defined SHARED && IS_IN (libc)
>  __hidden_ver1 (__strchrnul_c, __GI___strchrnul, __strchrnul_c);
> +#  endif
>  # endif
>  #endif

Re: [PATCH] S390: Fix building with --disable-mutli-arch [BZ #31196]

[Sam James]

Stefan Liebler <stli@linux.ibm.com> writes:

> Starting with commits
> - 7ea510127e2067efa07865158ac92c330c379950
> string: Add libc_hidden_proto for strchrnul
> - 22999b2f0fb62eed1af4095d062bd1272d6afeb1
> string: Add libc_hidden_proto for memrchr

Nit: s/mutli/multi/ in title (it's obvious what it means ofc but
mentioning it because it impacts ability to grep).

Thanks!

>
> building glibc on s390x with --disable-multi-arch fails if only
> the C-variant of strchrnul / memrchr is used.  This is the case
> if gcc uses -march < z13.
>
> The build fails with:
> ../sysdeps/s390/strchrnul-c.c:28:49: error: ‘__strchrnul_c’ undeclared here (not in a function); did you mean ‘__strchrnul’?
>    28 | __hidden_ver1 (__strchrnul_c, __GI___strchrnul, __strchrnul_c);
>
> With --disable-multi-arch, __strchrnul_c is not available as string/strchrnul.c
> is just included without defining STRCHRNUL and thus we also don't have to create
> the internal hidden symbol.
> ---
>  sysdeps/s390/memrchr-c.c   | 4 +++-
>  sysdeps/s390/strchrnul-c.c | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/sysdeps/s390/memrchr-c.c b/sysdeps/s390/memrchr-c.c
> index b4b85725d3..f6ffe85572 100644
> --- a/sysdeps/s390/memrchr-c.c
> +++ b/sysdeps/s390/memrchr-c.c
> @@ -25,7 +25,9 @@
>  
>  # include <string/memrchr.c>
>  
> -# if defined SHARED && IS_IN (libc)
> +# if HAVE_MEMRCHR_IFUNC
> +#  if defined SHARED && IS_IN (libc)
>  __hidden_ver1 (__memrchr_c, __GI___memrchr, __memrchr_c);
> +#  endif
>  # endif
>  #endif
> diff --git a/sysdeps/s390/strchrnul-c.c b/sysdeps/s390/strchrnul-c.c
> index 7f2f1fd540..97fd9ffbf7 100644
> --- a/sysdeps/s390/strchrnul-c.c
> +++ b/sysdeps/s390/strchrnul-c.c
> @@ -24,7 +24,9 @@
>  # endif
>  
>  # include <string/strchrnul.c>
> -# if defined SHARED && IS_IN (libc)
> +# if HAVE_STRCHRNUL_IFUNC
> +#  if defined SHARED && IS_IN (libc)
>  __hidden_ver1 (__strchrnul_c, __GI___strchrnul, __strchrnul_c);
> +#  endif
>  # endif
>  #endif

Re: [PATCH] S390: Fix building with --disable-mutli-arch [BZ #31196]

[Stefan Liebler]

On 30.01.24 19:59, Sam James wrote:
> 
> Stefan Liebler <stli@linux.ibm.com> writes:
> 
>> Starting with commits
>> - 7ea510127e2067efa07865158ac92c330c379950
>> string: Add libc_hidden_proto for strchrnul
>> - 22999b2f0fb62eed1af4095d062bd1272d6afeb1
>> string: Add libc_hidden_proto for memrchr
> 
> Nit: s/mutli/multi/ in title (it's obvious what it means ofc but
> mentioning it because it impacts ability to grep).
> 
> Thanks!
Sorry. Too late. Andreas has already committed it to be within glibc
2.39 release.

Re: [PATCH] S390: Fix building with --disable-mutli-arch [BZ #31196]

[Stefan Liebler]

On 30.01.24 18:21, Adhemerval Zanella Netto wrote:
> 
> 
> On 30/01/24 05:34, Stefan Liebler wrote:
>> Starting with commits
>> - 7ea510127e2067efa07865158ac92c330c379950
>> string: Add libc_hidden_proto for strchrnul
>> - 22999b2f0fb62eed1af4095d062bd1272d6afeb1
>> string: Add libc_hidden_proto for memrchr
>>
>> building glibc on s390x with --disable-multi-arch fails if only
>> the C-variant of strchrnul / memrchr is used.  This is the case
>> if gcc uses -march < z13.
>>
>> The build fails with:
>> ../sysdeps/s390/strchrnul-c.c:28:49: error: ‘__strchrnul_c’ undeclared here (not in a function); did you mean ‘__strchrnul’?
>>    28 | __hidden_ver1 (__strchrnul_c, __GI___strchrnul, __strchrnul_c);
>>
>> With --disable-multi-arch, __strchrnul_c is not available as string/strchrnul.c
>> is just included without defining STRCHRNUL and thus we also don't have to create
>> the internal hidden symbol.
> 
> LGTM, thanks.  In same topic, why s390 is not following the multiarch sysdep
> folder organization followed by all others ports, where ifunc implementation
> are put only in multiarch folder?
> 
> The multiarch mechanism was added to simplify this kind of code, the s390
> seems gratuitous over complicated in this regard.
> 
Thanks for reviewing. Andreas has already committed it to be within
glibc 2.39 release.

Yes, the mechanism on s390 is complicated. It decides at build time
which variants are needed depending on the gcc -march=cpu-level.

If the cpu-level is new enough, it skips generating IFUNC at all and
just uses the variant for the latest cpu-level directly. Also the __GI
symbol points to this variant and it is also used in ld.so.

If IFUNC is used, variants for older than the used cpu-level are
omitted. And the __GI symbols point to the variants fitting best to the
specified cpu-level. Those are also used in ld.so. Newer variants are
available via IFUNC.

E.g. sysdeps/x86_64/strlen.S is also using a mechanism to determine the
default implementation. There it just includes the concrete xyz.S file
in multiarch directory.

On s390 we have plenty of cases where we have an assembler
implementation for e.g. z13-vector-instructions and for older cpu-levels
the common-code implementation in C. Thus we can't just do an include
like done in sysdeps/x86_64/strlen.S. Therefore all the variants are not
in the multiarch-folder and there are the guards with #if directives to
omit or include the variant. As the __GI symbol is pointing to a
concrete variant instead of to the IFUNC symbol, maintaining those is
more complicated.

I've just posted this patch to add more configurations in
build-many-glibcs.py:
[PATCH] build-many-glibcs.py: Add s390 --disable-multi-arch / multi-arch
configurations.
https://sourceware.org/pipermail/libc-alpha/2024-January/154342.html