Message ID | 20240130083432.544403-1-stli@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | S390: Fix building with --disable-mutli-arch [BZ #31196] | expand |
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
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
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.
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
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