Revert "Correctly determine libc.so 'OUTPUT_FORMAT' when cross-compiling."

  This reverts commit 361d6454c034a920f2c96517c277990d390b9652.

This trips up riscv gnu toolchain builds [1]

riscv ld segfaults when linking libgcc because libc.so linker script
contains `OUTPUT_FORMAT(elf32-little)` vs. `OUTPUT_FORMAT(elf32-littleriscv)`.

This patch causes builds to lookup riscv32* prefixed objdump and failing
to find it falls back to host objdump which is the root of the issue.
The host objdump in turn generates `OUTPUT_FORMAT(elf32-little)`

riscv glibc multilib builds lack riscv32 prefix binaries. They have a
single set of "riscv64" prefixed binaries supporting both 32 and 64-bit
abis: ilp32/ilp32d/lp64/lp64d using -march/-mabi.

FWIW I'm not sure how this patch fixed a real problem to begin with.
At least for the riscv toolchain builds the non-prefixed objdump was
still from the cross-tools and behaved exactly same as the prefixed one.
At least it was not picking up the host version which would fail -
ironically what this patch ended up triggering for riscv.

| $ find TC_INSTALL -name *objdump -exec md5sum {} \;
| 5fd0b967d4977a4f8bc3a7b7a9318b1d  ./bin/riscv64-unknown-linux-gnu-objdump
| 5fd0b967d4977a4f8bc3a7b7a9318b1d  ./riscv64-unknown-linux-gnu/bin/objdump
|
| $ ./bin/riscv64-unknown-linux-gnu-objdump -f ./xx.lds.so  | grep "file format"
| ./xx.lds.so:     file format elf32-littleriscv
|
| $ ./riscv64-unknown-linux-gnu/bin/objdump -f ./xx.lds.so  | grep "file format"
| ./xx.lds.so:     file format elf32-littleriscv

[1] https://github.com/riscv-collab/riscv-gnu-toolchain/issues/1161

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 aclocal.m4   |  2 ++
 configure    | 96 ++--------------------------------------------------
 configure.ac |  1 -
 3 files changed, 5 insertions(+), 94 deletions(-)
  

Hi,

Vineet Gupta <vineetg@rivosinc.com> skribis:

> This reverts commit 361d6454c034a920f2c96517c277990d390b9652.
>
> This trips up riscv gnu toolchain builds [1]
>
> riscv ld segfaults when linking libgcc because libc.so linker script
> contains `OUTPUT_FORMAT(elf32-little)` vs. `OUTPUT_FORMAT(elf32-littleriscv)`.
>
> This patch causes builds to lookup riscv32* prefixed objdump and failing
> to find it falls back to host objdump which is the root of the issue.
> The host objdump in turn generates `OUTPUT_FORMAT(elf32-little)`
>
> riscv glibc multilib builds lack riscv32 prefix binaries. They have a
> single set of "riscv64" prefixed binaries supporting both 32 and 64-bit
> abis: ilp32/ilp32d/lp64/lp64d using -march/-mabi.
>
> FWIW I'm not sure how this patch fixed a real problem to begin with.

The rationale was described in the context of cross-compilation to
aarch64-linux-gnu:

  https://sourceware.org/pipermail/libc-alpha/2021-July/128333.html

We observed a similar issue with objcopy when cross-compiling to
powerpc64le-linux-gnu:

  https://issues.guix.gnu.org/49417

Maybe we should see, in your case, why “riscv32-linux-gnu-objdump -f”
reports “elf32-little” instead of “elf32-littleriscv”?

HTH,
Ludo’.
  

Hi Ludovic,

On 11/26/22 06:57, Ludovic Courtès wrote:
> Hi,
>
> Vineet Gupta <vineetg@rivosinc.com> skribis:
>
>> This reverts commit 361d6454c034a920f2c96517c277990d390b9652.
>>
>> This trips up riscv gnu toolchain builds [1]
>>
>> riscv ld segfaults when linking libgcc because libc.so linker script
>> contains `OUTPUT_FORMAT(elf32-little)` vs. `OUTPUT_FORMAT(elf32-littleriscv)`.
>>
>> This patch causes builds to lookup riscv32* prefixed objdump and failing
>> to find it falls back to host objdump which is the root of the issue.
>> The host objdump in turn generates `OUTPUT_FORMAT(elf32-little)`
>>
>> riscv glibc multilib builds lack riscv32 prefix binaries. They have a
>> single set of "riscv64" prefixed binaries supporting both 32 and 64-bit
>> abis: ilp32/ilp32d/lp64/lp64d using -march/-mabi.
>>
>> FWIW I'm not sure how this patch fixed a real problem to begin with.
> The rationale was described in the context of cross-compilation to
> aarch64-linux-gnu:
>
>    https://sourceware.org/pipermail/libc-alpha/2021-July/128333.html

Yes I understood that clearly before posting this patch.

> We observed a similar issue with objcopy when cross-compiling to
> powerpc64le-linux-gnu:
>
>    https://issues.guix.gnu.org/49417

Right but they both are for the guix build env. Can you show that 
problem exists in standard glibc build env and that this patch fixes 
some issue there. FWIW at above page the full log link is broken.

Anyhow if you read the details in my revert changelog, I mentioned that 
for the cross toolchain, there are 2 objdump binaries. One with the 
prefix and one w/o.

| $ find TC_INSTALL -name *objdump -exec md5sum {} \;
| 5fd0b967d4977a4f8bc3a7b7a9318b1d  ./bin/riscv64-unknown-linux-gnu-objdump
| 5fd0b967d4977a4f8bc3a7b7a9318b1d  ./riscv64-unknown-linux-gnu/bin/objdump

W/o the patch, sure glibc build picks up the non-prefixed one.

| /scratch/vineetg/gnu/TC_INSTALL/lib/gcc/riscv64-unknown-linux-gnu/12.2.0/../../../../riscv64-unknown-linux-gnu/bin/objdump -f /scratch/vineetg/gnu/toolchain-src/build-glibc-linux-rv32imac-ilp32/format.lds.so | sed -n 's/.*file format \(.*\)/OUTPUT_FORMAT(\1)/;T;p' > /scratch/vineetg/gnu/toolchain-src/build-glibc-linux-rv32imac-ilp32/format.lds

And the non-prefixed objdump is functionally exactly same as prefixed one (as evident from md5sum above). Obviously they both produce the desired/correct "file format" - again from my changelog posted.

|
| $ ./bin/riscv64-unknown-linux-gnu-objdump -f ./xx.lds.so  | grep "file format"
| ./xx.lds.so:     file format elf32-littleriscv
|
| $ ./riscv64-unknown-linux-gnu/bin/objdump -f ./xx.lds.so  | grep "file format"


The issue is this patch makes an assumption that a triplet prefixed 
cross binary exists, which may not always happen in multilib setups.


> Maybe we should see, in your case, why “riscv32-linux-gnu-objdump -f”
> reports “elf32-little” instead of “elf32-littleriscv”?

Again if you read my changelog carefully I mention that in my setup 
riscv32-* binaries don't exist, as explained above.
So lets not add that assumption in tooling.

I'm not aware of how guix build env is setup. Can you check if PATH is 
not clobbered there - if the tooling uses -print-prog-name=objdump it 
should pick the correct cross objdump, not host binary.

-Vineet
  

Hi,

Vineet Gupta <vineetg@rivosinc.com> skribis:

> On 11/26/22 06:57, Ludovic Courtès wrote:
>> Hi,
>>
>> Vineet Gupta <vineetg@rivosinc.com> skribis:

[...]

>>> riscv glibc multilib builds lack riscv32 prefix binaries. They have a
>>> single set of "riscv64" prefixed binaries supporting both 32 and 64-bit
>>> abis: ilp32/ilp32d/lp64/lp64d using -march/-mabi.

[...]

> The issue is this patch makes an assumption that a triplet prefixed
> cross binary exists, which may not always happen in multilib setups.

Sorry, I had overlooked that.  I trust your judgment.

It does seem that we’ll have either one issue or the other though.
Downstream we can adjust either way, so that’s okay.

Thanks,
Ludo’.
  

On 11/28/22 07:37, Ludovic Courtès wrote:
> It does seem that we’ll have either one issue or the other though.

Not necessarily.

> Downstream we can adjust either way, so that’s okay.

If $CC -print-prog-name=objdump is used, I don't see why wrong objdump 
would be picked up in your setup.

-Vineet
  

On Tue, 22 Nov 2022 18:59:32 PST (-0800), Vineet Gupta wrote:
> This reverts commit 361d6454c034a920f2c96517c277990d390b9652.
>
> This trips up riscv gnu toolchain builds [1]
>
> riscv ld segfaults when linking libgcc because libc.so linker script
> contains `OUTPUT_FORMAT(elf32-little)` vs. `OUTPUT_FORMAT(elf32-littleriscv)`.

Sounds like a linker bug?

> This patch causes builds to lookup riscv32* prefixed objdump and failing
> to find it falls back to host objdump which is the root of the issue.
> The host objdump in turn generates `OUTPUT_FORMAT(elf32-little)`
>
> riscv glibc multilib builds lack riscv32 prefix binaries. They have a
> single set of "riscv64" prefixed binaries supporting both 32 and 64-bit
> abis: ilp32/ilp32d/lp64/lp64d using -march/-mabi.

Though we _could_ have riscv32-* and riscv64-* binaries (and also 
riscv*- too), it just doesn't make much of a difference as the riscv64-* 
binaries can target everything.  If the answer here is just "fix 
riscv-gnu-toolchain" that seems reasonable to me, but also happy to 
avoid touching that so

Acked-by: Palmer Dabbelt <palmer@rivosinc.com>

Though some sort of global reviewer should take a look here, I 
definately don't understand the glibc build process well enough to 
review this one.

Thanks!

> FWIW I'm not sure how this patch fixed a real problem to begin with.
> At least for the riscv toolchain builds the non-prefixed objdump was
> still from the cross-tools and behaved exactly same as the prefixed one.
> At least it was not picking up the host version which would fail -
> ironically what this patch ended up triggering for riscv.
>
> | $ find TC_INSTALL -name *objdump -exec md5sum {} \;
> | 5fd0b967d4977a4f8bc3a7b7a9318b1d  ./bin/riscv64-unknown-linux-gnu-objdump
> | 5fd0b967d4977a4f8bc3a7b7a9318b1d  ./riscv64-unknown-linux-gnu/bin/objdump
> |
> | $ ./bin/riscv64-unknown-linux-gnu-objdump -f ./xx.lds.so  | grep "file format"
> | ./xx.lds.so:     file format elf32-littleriscv
> |
> | $ ./riscv64-unknown-linux-gnu/bin/objdump -f ./xx.lds.so  | grep "file format"
> | ./xx.lds.so:     file format elf32-littleriscv
>
> [1] https://github.com/riscv-collab/riscv-gnu-toolchain/issues/1161
>
> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> ---
>  aclocal.m4   |  2 ++
>  configure    | 96 ++--------------------------------------------------
>  configure.ac |  1 -
>  3 files changed, 5 insertions(+), 94 deletions(-)
>
> diff --git a/aclocal.m4 b/aclocal.m4
> index 531b11cb6988..3b4df4a37197 100644
> --- a/aclocal.m4
> +++ b/aclocal.m4
> @@ -122,6 +122,8 @@ AS=`$CC -print-prog-name=as`
>  LD=`$CC -print-prog-name=$LDNAME`
>  AR=`$CC -print-prog-name=ar`
>  AC_SUBST(AR)
> +OBJDUMP=`$CC -print-prog-name=objdump`
> +AC_SUBST(OBJDUMP)
>  OBJCOPY=`$CC -print-prog-name=objcopy`
>  AC_SUBST(OBJCOPY)
>  GPROF=`$CC -print-prog-name=gprof`
> diff --git a/configure b/configure
> index cb0996d59e21..fbaa20e040b5 100755
> --- a/configure
> +++ b/configure
> @@ -649,6 +649,7 @@ LD
>  AS
>  GPROF
>  OBJCOPY
> +OBJDUMP
>  AR
>  LN_S
>  INSTALL_DATA
> @@ -684,7 +685,6 @@ sysheaders
>  ac_ct_CXX
>  CXXFLAGS
>  CXX
> -OBJDUMP
>  READELF
>  CPP
>  cross_compiling
> @@ -2963,98 +2963,6 @@ else
>    READELF="$ac_cv_prog_READELF"
>  fi
>
> -if test -n "$ac_tool_prefix"; then
> -  # Extract the first word of "${ac_tool_prefix}objdump", so it can be a program name with args.
> -set dummy ${ac_tool_prefix}objdump; ac_word=$2
> -{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
> -$as_echo_n "checking for $ac_word... " >&6; }
> -if ${ac_cv_prog_OBJDUMP+:} false; then :
> -  $as_echo_n "(cached) " >&6
> -else
> -  if test -n "$OBJDUMP"; then
> -  ac_cv_prog_OBJDUMP="$OBJDUMP" # Let the user override the test.
> -else
> -as_save_IFS=$IFS; IFS=$PATH_SEPARATOR
> -for as_dir in $PATH
> -do
> -  IFS=$as_save_IFS
> -  test -z "$as_dir" && as_dir=.
> -    for ac_exec_ext in '' $ac_executable_extensions; do
> -  if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then
> -    ac_cv_prog_OBJDUMP="${ac_tool_prefix}objdump"
> -    $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5
> -    break 2
> -  fi
> -done
> -  done
> -IFS=$as_save_IFS
> -
> -fi
> -fi
> -OBJDUMP=$ac_cv_prog_OBJDUMP
> -if test -n "$OBJDUMP"; then
> -  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $OBJDUMP" >&5
> -$as_echo "$OBJDUMP" >&6; }
> -else
> -  { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
> -$as_echo "no" >&6; }
> -fi
> -
> -
> -fi
> -if test -z "$ac_cv_prog_OBJDUMP"; then
> -  ac_ct_OBJDUMP=$OBJDUMP
> -  # Extract the first word of "objdump", so it can be a program name with args.
> -set dummy objdump; ac_word=$2
> -{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5
> -$as_echo_n "checking for $ac_word... " >&6; }
> -if ${ac_cv_prog_ac_ct_OBJDUMP+:} false; then :
> -  $as_echo_n "(cached) " >&6
> -else
> -  if test -n "$ac_ct_OBJDUMP"; then
> -  ac_cv_prog_ac_ct_OBJDUMP="$ac_ct_OBJDUMP" # Let the user override the test.
> -else
> -as_save_IFS=$IFS; IFS=$PATH_SEPARATOR
> -for as_dir in $PATH
> -do
> -  IFS=$as_save_IFS
> -  test -z "$as_dir" && as_dir=.
> -    for ac_exec_ext in '' $ac_executable_extensions; do
> -  if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then
> -    ac_cv_prog_ac_ct_OBJDUMP="objdump"
> -    $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5
> -    break 2
> -  fi
> -done
> -  done
> -IFS=$as_save_IFS
> -
> -fi
> -fi
> -ac_ct_OBJDUMP=$ac_cv_prog_ac_ct_OBJDUMP
> -if test -n "$ac_ct_OBJDUMP"; then
> -  { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_ct_OBJDUMP" >&5
> -$as_echo "$ac_ct_OBJDUMP" >&6; }
> -else
> -  { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
> -$as_echo "no" >&6; }
> -fi
> -
> -  if test "x$ac_ct_OBJDUMP" = x; then
> -    OBJDUMP="false"
> -  else
> -    case $cross_compiling:$ac_tool_warned in
> -yes:)
> -{ $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: using cross tools not prefixed with host triplet" >&5
> -$as_echo "$as_me: WARNING: using cross tools not prefixed with host triplet" >&2;}
> -ac_tool_warned=yes ;;
> -esac
> -    OBJDUMP=$ac_ct_OBJDUMP
> -  fi
> -else
> -  OBJDUMP="$ac_cv_prog_OBJDUMP"
> -fi
> -
>
>  # We need the C++ compiler only for testing.
>  ac_ext=cpp
> @@ -4672,6 +4580,8 @@ AS=`$CC -print-prog-name=as`
>  LD=`$CC -print-prog-name=$LDNAME`
>  AR=`$CC -print-prog-name=ar`
>
> +OBJDUMP=`$CC -print-prog-name=objdump`
> +
>  OBJCOPY=`$CC -print-prog-name=objcopy`
>
>  GPROF=`$CC -print-prog-name=gprof`
> diff --git a/configure.ac b/configure.ac
> index 20b5e8043fad..0ca4bf56ef76 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -52,7 +52,6 @@ fi
>  AC_SUBST(cross_compiling)
>  AC_PROG_CPP
>  AC_CHECK_TOOL(READELF, readelf, false)
> -AC_CHECK_TOOL(OBJDUMP, objdump, false)
>
>  # We need the C++ compiler only for testing.
>  AC_PROG_CXX
  

On 28/11/22 20:25, Palmer Dabbelt wrote:
> On Tue, 22 Nov 2022 18:59:32 PST (-0800), Vineet Gupta wrote:
>> This reverts commit 361d6454c034a920f2c96517c277990d390b9652.
>>
>> This trips up riscv gnu toolchain builds [1]
>>
>> riscv ld segfaults when linking libgcc because libc.so linker script
>> contains `OUTPUT_FORMAT(elf32-little)` vs. `OUTPUT_FORMAT(elf32-littleriscv)`.
> 
> Sounds like a linker bug?
> 
>> This patch causes builds to lookup riscv32* prefixed objdump and failing
>> to find it falls back to host objdump which is the root of the issue.
>> The host objdump in turn generates `OUTPUT_FORMAT(elf32-little)`
>>
>> riscv glibc multilib builds lack riscv32 prefix binaries. They have a
>> single set of "riscv64" prefixed binaries supporting both 32 and 64-bit
>> abis: ilp32/ilp32d/lp64/lp64d using -march/-mabi.
> 
> Though we _could_ have riscv32-* and riscv64-* binaries (and also riscv*- too), it just doesn't make much of a difference as the riscv64-* binaries can target everything.  If the answer here is just "fix riscv-gnu-toolchain" that seems reasonable to me, but also happy to avoid touching that so
> 
> Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
> 
> Though some sort of global reviewer should take a look here, I definately don't understand the glibc build process well enough to review this one.
> 


I think the question is whether we need to set OBJDUMP and/or can override
or if we should derive the tools solely from CC.  It make sense for later,
since using a different binutils than the one CC is meant to use not always
guarantee that compiler will emits all supports instructions or ABI.

The build-many-glibc.py sets the OBJDUMP, so I think it would be good to
also remove its usage (since with this patch OBJDUMP can not be override
anymore).  In fact the only way to override the binutils with
LIBC_PROG_BINUTILS is to use --with-binutils (which has some issues [1] 
though).

The --with-binutils is also a developer options, so I wonder if we should 
simplify the build process by removing it along with LIBC_PROG_BINUTILS, and 
replace with a new macro that checks whether the tools is set, otherwise uses 
the CC with -print-prog-name for default case.  

It would continue to provide a way to override binutils tools (for development 
and testing) and simplify the build process (one less configure option).  GUIX
can then override OBJDUMP if it need (and it was not clear not me why 
CC -print-prog-name does not work on this environment).

[1] https://patchwork.sourceware.org/project/glibc/patch/20221115193159.173838-2-adhemerval.zanella@linaro.org/
  

On 11/30/22 04:39, Adhemerval Zanella Netto wrote:
>
> On 28/11/22 20:25, Palmer Dabbelt wrote:
>> On Tue, 22 Nov 2022 18:59:32 PST (-0800), Vineet Gupta wrote:
>>> This reverts commit 361d6454c034a920f2c96517c277990d390b9652.
>>>
>>> This trips up riscv gnu toolchain builds [1]
>>>
>>> riscv ld segfaults when linking libgcc because libc.so linker script
>>> contains `OUTPUT_FORMAT(elf32-little)` vs. `OUTPUT_FORMAT(elf32-littleriscv)`.
>> Sounds like a linker bug?
>>
>>> This patch causes builds to lookup riscv32* prefixed objdump and failing
>>> to find it falls back to host objdump which is the root of the issue.
>>> The host objdump in turn generates `OUTPUT_FORMAT(elf32-little)`
>>>
>>> riscv glibc multilib builds lack riscv32 prefix binaries. They have a
>>> single set of "riscv64" prefixed binaries supporting both 32 and 64-bit
>>> abis: ilp32/ilp32d/lp64/lp64d using -march/-mabi.
>> Though we _could_ have riscv32-* and riscv64-* binaries (and also riscv*- too), it just doesn't make much of a difference as the riscv64-* binaries can target everything.  If the answer here is just "fix riscv-gnu-toolchain" that seems reasonable to me, but also happy to avoid touching that so
>>
>> Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> Though some sort of global reviewer should take a look here, I definately don't understand the glibc build process well enough to review this one.
>>
>
> I think the question is whether we need to set OBJDUMP and/or can override
> or if we should derive the tools solely from CC.  It make sense for later,
> since using a different binutils than the one CC is meant to use not always
> guarantee that compiler will emits all supports instructions or ABI.

Certainly the ability to override OBJDUMP (actually all of binutils) 
seems useful. However the issue with the original patch was it assuming 
that triplet prefixed binary exists and even worse it silently falling 
back to host binary if it fails to find one. If we really want to 
support the original patch, it should add a fallback of using $CC 
-print-prog-name if the triplet binary search fails.

But as it stands, riscv multilib toolchain builds are currently hosed 
because of this.

-Vineet

> The build-many-glibc.py sets the OBJDUMP, so I think it would be good to
> also remove its usage (since with this patch OBJDUMP can not be override
> anymore).  In fact the only way to override the binutils with
> LIBC_PROG_BINUTILS is to use --with-binutils (which has some issues [1]
> though).
>
> The --with-binutils is also a developer options, so I wonder if we should
> simplify the build process by removing it along with LIBC_PROG_BINUTILS, and
> replace with a new macro that checks whether the tools is set, otherwise uses
> the CC with -print-prog-name for default case.
>
> It would continue to provide a way to override binutils tools (for development
> and testing) and simplify the build process (one less configure option).  GUIX
> can then override OBJDUMP if it need (and it was not clear not me why
> CC -print-prog-name does not work on this environment).
>
> [1] https://patchwork.sourceware.org/project/glibc/patch/20221115193159.173838-2-adhemerval.zanella@linaro.org/