Re: [PATCH v2] S390: Always enable static PIE if build with lld.

[Stefan Liebler <stli@linux.ibm.com>]

On 27.02.24 14:49, Adhemerval Zanella Netto wrote:
> 
> 
> On 26/02/24 11:32, Stefan Liebler wrote:
>> On 23.02.24 15:48, Adhemerval Zanella Netto wrote:
>>>
>>>
>>> On 21/02/24 11:56, Stefan Liebler wrote:
>>>
>>> Sorry, but all the PIE tests on s390x are overcomplicated and brittle.  The version
>>> check is really unecessary and I think we should move away from it, instead check
>>> for either linker support and resulting relocations on generated code.
>>>
>>> The gold supports could be just filtered out by using -Wl,--no-dynamic-linker on
>> Yes, when linking with "gcc -static-pie", gcc automatically adds
>> --no-dynamic-linker and thus "-Wl,--no-dynamic-linker" can be added to
>> the -pie check.
> 
> Indeed, I think all supported gcc version add --no-dynamic-linker for --static-pie.
> 
>>
>>> the -pie check.  Also, the relocation check does not seem correct: at least
>>> with binutils 2.36 ld generates R_390_TLS_DTPMOD/R_390_TLS_DTPOFF for the
>>> 'foo' access.
>> Interesting. I don't see R_390_TLS_DTPMOD/R_390_TLS_DTPOFF with binutils
>> 2.36, but:
>> - conftest1.o
>> 000000000000  000a00000033 R_390_TLS_LE64    0000000000000000 foo + 0
>> - conftest2.o
>> 00000000000a  000a00000031 R_390_TLS_IEENT   0000000000000000 foo + 2
>> - conftest
>> 000000001fe8  000800000038 R_390_TLS_TPOFF   0000000000000000 foo + 0
>>
>> Can you please have a further look how the
>> R_390_TLS_DTPMOD/R_390_TLS_DTPOFF were generated?
> 
> This was from the startup/standard library, the R_390_TLS_TPOFF is indeed
> the correct relocation to check.
> 
>>>
>>> So I think we should use something as the below.  The libc_cv_s390x_staticpie_req_runtime
>>> is correctly set for old binutils and lld version 17, while being true for recent
>>> binutils and lld version 18+.  I also think all the comments of the requirement
>>> are confusing and does not help much (it also references others ABI, like i386
>>> and ia64 which is not straightforward to understand why it matter for s390x),
>>> so I would recommend to just remove them altogether.
>> But, if you are bootstrapping the toolchain, then you don't have
>> crt-files and the linking -pie check fails. Then you don't get static
>> pie support and no rcrt1.o file.
>> This is e.g. the case for the Ubuntu libc6-dev-s390x-cross package.
>>
>> You also see it with build-many-glibcs.py script. Note that there glibc
>> configure is called twice:
>> -
>> logs/compilers/s390x-linux-gnu/026-compilers-s390x-linux-gnu-glibc-s390-linux-gnu-configure-log.txt
>> => called with empty sysroot. The following make install creates
>> crt-files in the sysroot folder. Currently rcrt1.o is created. Without
>> the version check, it would not be created.
>>
>> - logs/glibcs/s390x-linux-gnu/003-glibcs-s390x-linux-gnu-configure-log.txt
> 
> You are right, we need to follow what loongarch and riscv does and use
> -nostartfiles -nostdlib. Something like:
> 
This solves the bootstrapping-case and the test is independent of used
binutils/lld/gold linker. Then the version check is not needed anymore.

As this is your effort, do you want to continue with this patch?

Thanks a lot,
Stefan

> diff --git a/sysdeps/s390/s390-64/configure.ac b/sysdeps/s390/s390-64/configure.ac
> index 4657de0d37..a8849599b8 100644
> --- a/sysdeps/s390/s390-64/configure.ac
> +++ b/sysdeps/s390/s390-64/configure.ac
> @@ -1,34 +1,6 @@
>  GLIBC_PROVIDES dnl See aclocal.m4 in the top level source directory.
>  # Local configure fragment for sysdeps/s390/s390-64.
> 
> -# Bypass result of runtime configure check for known linker versions as
> -# e.g. crt-files or libc.so might not be available in bootstrapping
> -# environments.
> -case $($LD --version) in
> -  "GNU gold"*)
> -    # As of 2023-08-07, gold does not support static PIE due to
> -    # Bug 22221 - add --no-dynamic-linker option
> -    # https://sourceware.org/bugzilla/show_bug.cgi?id=22221
> -    libc_cv_s390x_staticpie_req_version=no
> -    ;;
> -  "LLD"*)
> -    # As of 2023-08-07, there is no lld which supports s390x.
> -    libc_cv_s390x_staticpie_req_version=no
> -    ;;
> -  *)
> -    # The required binutils patches are available with bintuils 2.39
> -    libc_cv_s390x_staticpie_req_version=yes
> -    # Skip AC_CHECK_PROGS and just use the result from main configure.ac.
> -    libc_cv_s390x_staticpie_req_LD=$LD
> -    AC_CHECK_PROG_VER(libc_cv_s390x_staticpie_req_LD, $LD, --version,
> -                     [GNU ld.* \([0-9][0-9]*\.[0-9.]*\)],
> -                     [2.1[0-9][0-9]*|2.39*|2.[4-9][0-9]*|[3-9].*|[1-9][0-9]*],
> -                     libc_cv_s390x_staticpie_req_version=no)
> -    ;;
> -esac
> -AC_MSG_CHECKING([for s390-specific static PIE requirements (version check)])
> -AC_MSG_RESULT($libc_cv_s390x_staticpie_req_version)
> -
>  # Minimal checking for static PIE support in ld.
>  # Compare to ld testcase/bugzilla:
>  # <binutils-source>/ld/testsuite/ld-elf/pr22263-1.rd
> @@ -58,15 +30,14 @@ EOF
>    libc_cv_s390x_staticpie_req_runtime=no
>    if AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS -fPIE -c conftest1.c -o conftest1.o]) \
>       && AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS -fPIE -c conftest2.c -o conftest2.o]) \
> -     && AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS -pie -o conftest conftest1.o conftest2.o]) \
> +     && AC_TRY_COMMAND([${CC-cc} $CFLAGS $CPPFLAGS $LDFLAGS -static-pie -nostartfiles -nostdlib -fPIE -o conftest conftest1.o conftest2.o]) \
>       && AC_TRY_COMMAND([! LC_ALL=C $READELF -Wr conftest | grep R_390_TLS_TPOFF])
>    then
>      libc_cv_s390x_staticpie_req_runtime=yes
>    fi
>    rm -rf conftest.*])
> 
> -if test $libc_cv_s390x_staticpie_req_runtime = yes \
> -   || test $libc_cv_s390x_staticpie_req_version = yes; then
> +if test $libc_cv_s390x_staticpie_req_runtime = yes; then
>     # Static PIE is supported only on 64bit.
>     # Ensure you also have those patches for:
>     # - binutils (ld)