Re: [PATCH v2] aarch64: Add fallback if ARM_CC_FOR_TARGET not set (was: Re: [PATCH 5/5] gdb: native target invalid architecture detection)

[Luis Machado <luis.machado@arm.com>]

On 6/8/22 12:20, Pedro Alves wrote:
> On 2022-06-08 11:12, Pedro Alves wrote:
>> On 2022-06-08 08:54, Luis Machado wrote:
> 
>> A 64-bit program can always exec a 32-bit program.  Likewise the other way around.  That should be
>> debuggable with a single gdb.
>>
>> As you say, it's supported.  So, better test it routinely.  Not testing it doesn't gain us anything,
>> other than leading to bitrot, as has happened.
>>
>>>>
>>>> Maybe it would be possible to come up with a way to default ARM_CC_FOR_TARGET to
>>>> something that works for most people when testing aarch64, somehow.
>>> The problem is that a compiler is not enough to make things run fine. If you have an aarch64
>>> Ubuntu, for example, you'd need to add armhf as an architecture option and install both the
>>> compiler and the libraries (libc6:armhf mostly).
>>
>> That is not an issue specific to aarch64.  On x86-64, you need to install extra runtime
>> packages to test -m32 too, just the compiler isn't enough, and those packages aren't installed
>> by default.
>>
>>>
>>> Another problem is that not every processor/kernel supports running a 32-bit process in a
>>> 64-bit environment. This makes things more complicated.
>>>
>>
>> Why "more complicated"?  If you're worried about FAILs in the testsuite, then we can e.g. make it run
>> the 32-bit program once outside gdb to check that running 32-bits works.  If that fails, then
>> we don't run the test.  Seems simple, not complicated.
> 
> Here's a new version of the patch, doing that.  This version also moves the ARM_CC_FOR_TARGET fallback
> discovery to lib/gdb.exp so it can be used by gdb.multi/multi-arch-exec.exp as well.  I've now tested
> this on gcc80 on the GCC compile farm (an aarch64 ubuntu 18.04 machine), and confirmed that it works.  As in,
> we pick the right compiler (turns out it is arm-linux-gnueabihf-gcc, the third fallback compiler, so the try-to-run
> code is exercised).  Of course, the testcases actually FAIL, due to 32-bit support bitrot in gdb itself.
> 
>  From 689671e40f88f1bf29af2433c7b6c7662c8f38c6 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Tue, 7 Jun 2022 20:11:32 +0100
> Subject: [PATCH] aarch64: Add fallback if ARM_CC_FOR_TARGET not set
> 
> On Aarch64, you can set ARM_CC_FOR_TARGET to point to the 32-bit
> compiler to use when testing gdb.multi/multi-arch.exp and
> gdb.multi/multi-arch-exec.exp.  If you don't set it, then those
> testcases don't run.
> 
> I guess that approximately nobody remembers to set ARM_CC_FOR_TARGET.
> 
> This commit adds a fallback.  If ARM_CC_FOR_TARGET is not set, and
> testing for Linux, try arm-linux-gnueabi-gcc,
> arm-none-linux-gnueabi-gcc, arm-linux-gnueabihf-gcc as 32-bit
> compilers, making sure that the produced executable runs on the target
> machine before claiming that the compiler produces useful executables.
> 
> Change-Id: Iefe5865d5fc84b4032eaff7f4c5c61582bf75c39
> ---
>   gdb/testsuite/gdb.multi/multi-arch-exec.exp |  4 +-
>   gdb/testsuite/gdb.multi/multi-arch.exp      |  4 +-
>   gdb/testsuite/lib/gdb.exp                   | 57 +++++++++++++++++++++
>   3 files changed, 61 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.multi/multi-arch-exec.exp b/gdb/testsuite/gdb.multi/multi-arch-exec.exp
> index a1496fb5571..a8e81e29ee9 100644
> --- a/gdb/testsuite/gdb.multi/multi-arch-exec.exp
> +++ b/gdb/testsuite/gdb.multi/multi-arch-exec.exp
> @@ -62,8 +62,8 @@ proc append_arch2_options {options_var} {
>       upvar 1 $options_var options
>   
>       if { [istarget "aarch64*-*-*"] } {
> -	if {[info exists ARM_CC_FOR_TARGET]} {
> -	    lappend options "compiler=${ARM_CC_FOR_TARGET}"
> +	if {[arm_cc_for_target] != ""} {
> +	    lappend options "compiler=[arm_cc_for_target]"
>   	    return 1
>   	} else {
>   	    unsupported "ARM compiler is not known"
> diff --git a/gdb/testsuite/gdb.multi/multi-arch.exp b/gdb/testsuite/gdb.multi/multi-arch.exp
> index e5c2d9cc161..b564e6caf25 100644
> --- a/gdb/testsuite/gdb.multi/multi-arch.exp
> +++ b/gdb/testsuite/gdb.multi/multi-arch.exp
> @@ -60,8 +60,8 @@ if { [build_executable "failed to prepare" ${exec1} "${srcfile1}" \
>   set options [list debug]
>   
>   if [istarget "aarch64*-*-*"] {
> -    if {[info exists ARM_CC_FOR_TARGET]} {
> -	lappend options "compiler=${ARM_CC_FOR_TARGET}"
> +    if {[arm_cc_for_target] != ""} {
> +	lappend options "compiler=[arm_cc_for_target]"
>       } else {
>   	unsupported "ARM compiler is not known"
>   	return -1
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 402450152ac..ac01a31655f 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -8666,5 +8666,62 @@ proc get_set_option_choices {set_cmd} {
>       return $values
>   }
>   
> +# Return the compiler that can generate 32-bit ARM executables.  Used
> +# when testing biarch support on aarch64.  If ARM_CC_FOR_TARGET is
> +# set, use that.  If not, try a few common compiler names, making sure
> +# that the executable they produce can run.
> +
> +gdb_caching_proc arm_cc_for_target {
> +    if {[info exists ARM_CC_FOR_TARGET]} {
> +	# If the user specified the compiler explicitly, then don't
> +	# check whether the resulting binary runs outside GDB.  Assume
> +	# that it does, and if it turns out it doesn't, then the user
> +	# should get loud FAILs, instead of UNSUPPORTED.
> +	return $ARM_CC_FOR_TARGET
> +    }
> +
> +    # Fallback to a few common compiler names.  Also confirm the
> +    # produced binary actually runs on the system before declaring
> +    # we've found the right compiler.
> +
> +    if [istarget "*-linux*-*"] {
> +	set compilers {
> +	    arm-linux-gnueabi-gcc
> +	    arm-none-linux-gnueabi-gcc
> +	    arm-linux-gnueabihf-gcc
> +	}
> +    } else {
> +	set compilers {}
> +    }
> +
> +    foreach compiler $compilers {
> +	if {![is_remote host] && [which $compiler] == 0} {
> +	    # Avoid "default_target_compile: Can't find
> +	    # $compiler." warning issued from gdb_compile.
> +	    continue
> +	}
> +
> +	set src { int main() { return 0; } }
> +	if {[gdb_simple_compile aarch64-32bit \
> +		 $src \
> +		 executable [list compiler=$compiler]]} {
> +
> +	    set result [remote_exec target $obj]
> +	    set status [lindex $result 0]
> +	    set output [lindex $result 1]
> +
> +	    file delete $obj
> +
> +	    if { $output != "" || $status != 0} {
> +		continue
> +	    }
> +
> +	    return $compiler
> +	}
> +    }
> +
> +    return ""
> +}
> +
>   # Always load compatibility stuff.
>   load_lib future.exp
> 
> base-commit: bcdbf606bc054bd1fa9a1c8978f2d3a57e42adf4

That looks OK to me. Thanks for writing the patch.

I'll send a patch later documenting the ARM_CC_FOR_TARGET option and maybe another
one for listing/installing Ubuntu packages required by the testsuite for broad coverage.