Re: [PATCH 2/7] [gdb/testsuite] fix test gdb.base/print-file-var.exp for remote execution

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

> On 10/25/22 18:29, Ivan Tetyushkin wrote:
>> ---
>>   gdb/testsuite/gdb.base/print-file-var.exp | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gdb/testsuite/gdb.base/print-file-var.exp b/gdb/testsuite/gdb.base/print-file-var.exp
>> index 9abe87d7758..73137630fed 100644
>> --- a/gdb/testsuite/gdb.base/print-file-var.exp
>> +++ b/gdb/testsuite/gdb.base/print-file-var.exp
>> @@ -42,6 +42,8 @@ proc test {hidden dlopen version_id_main lang} {
>>       set libobj1 [standard_output_file ${lib1}$suffix.so]
>>       set libobj2 [standard_output_file ${lib2}$suffix.so]
>>   
>> +    set runtimelibobj2 [get_runtime_file $libobj2]
>> +
>>       set lib_opts { debug $lang }
>>       lappend lib_opts "additional_flags=-DHIDDEN=$hidden"
>>   
>> @@ -60,7 +62,7 @@ proc test {hidden dlopen version_id_main lang} {
>>       set link_opts [list debug shlib=${libobj1}]
>>   
>>       if {$dlopen} {
>> -	lappend main_opts "additional_flags=-DSHLIB_NAME=\"$libobj2\""
>> +	lappend main_opts "additional_flags=-DSHLIB_NAME=\"$runtimelibobj2\""
>>   	lappend link_opts "shlib_load"
>>       } else {
>>   	lappend link_opts "shlib=${libobj2}"
>
> I get this test-case passing by avoiding to use an absolute file name:
> ...
> diff --git a/gdb/testsuite/gdb.base/print-file-var.exp 
> b/gdb/testsuite/gdb.base/print-file-
> var.exp
> index 9abe87d7758..338840cb05e 100644
> --- a/gdb/testsuite/gdb.base/print-file-var.exp
> +++ b/gdb/testsuite/gdb.base/print-file-var.exp
> @@ -60,7 +60,7 @@ proc test {hidden dlopen version_id_main lang} {
>       set link_opts [list debug shlib=${libobj1}]
>
>       if {$dlopen} {
> -       lappend main_opts "additional_flags=-DSHLIB_NAME=\"$libobj2\""
> +       lappend main_opts "additional_flags=-DSHLIB_NAME=\"[file tail 
> $libobj2]\""
>          lappend link_opts "shlib_load"
>       } else {
>          lappend link_opts "shlib=${libobj2}"
> ...
> which means that the file is found relative to $ORIGIN, as is the case 
> for $dlopen == 0.
>
> I'm not entirely happy with this fix, because what we'd rather want is 
> to use the name as returned by remote_download, but that's done in 
> gdb_load_shlib later on (which also handles shlib_target_file, so 
> actually, the name as returned by this proc is the one we really want), 
> so it's not available yet at the time we do the compilation.
>
> I've thought about allowing gdb_load_shlib to be called without gdb 
> instance, and for cases where there's no instance, registering the 
> solib-search-path setting to be done at gdb start.  This will allow us 
> to move the gdb_load_shlib calls to before the compilation, such that we 
> can use the result to set -DSHLIB_NAME.  But I think it's a solution 
> bound to cause confusion because things happen under the hood.
>
> Alternatively, we can try to not pass in the name into compilation 
> (working around only some of the problems related to remote host 
> testing, see https://sourceware.org/bugzilla/show_bug.cgi?id=16947), but 
> that also has its limitations: we need to be able to patch target memory 
> or the ability to use argv.
>
> Perhaps splitting up the functionality in gdb_load_shlib makes the most 
> sense.  By default, it'll do the same as before, but by calling it 
> twice, once with -only-download, and once with -no-download can we get 
> the solib name before starting gdb.
>
> I've given this a try in patch attached below.

I also prefer the approach that Tom has suggested here.  I gave the
patch a go and it fixed the problems in print_file-var.exp for me using
a local/remote type setup.

>
> Ivan, could you check if the patch fixes the gdb.base/print-file-var.exp 
> test-case for you?
>
> Thanks,
> - Tom
> From b072645433f83d62a4b7cf857d3d50f0dd7e0c75 Mon Sep 17 00:00:00 2001
> From: Tom de Vries <tdevries@suse.de>
> Date: Sun, 6 Nov 2022 11:26:23 +0100
> Subject: [PATCH 3/3] [gdb/testsuite] Fix gdb.base/print-file-var.exp for
>  remote target
>
> ---
>  gdb/testsuite/gdb.base/print-file-var.exp |  6 ++-
>  gdb/testsuite/lib/gdb.exp                 | 48 +++++++++++++++++------
>  2 files changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/print-file-var.exp b/gdb/testsuite/gdb.base/print-file-var.exp
> index 9abe87d7758..abdbbbfdce4 100644
> --- a/gdb/testsuite/gdb.base/print-file-var.exp
> +++ b/gdb/testsuite/gdb.base/print-file-var.exp
> @@ -59,8 +59,10 @@ proc test {hidden dlopen version_id_main lang} {
>      set main_opts [list debug $lang]
>      set link_opts [list debug shlib=${libobj1}]
>  
> +    set target_libobj2 [gdb_load_shlib $libobj2 -only-download]
> +
>      if {$dlopen} {
> -	lappend main_opts "additional_flags=-DSHLIB_NAME=\"$libobj2\""
> +	lappend main_opts "additional_flags=-DSHLIB_NAME=\"$target_libobj2\""
>  	lappend link_opts "shlib_load"
>      } else {
>  	lappend link_opts "shlib=${libobj2}"
> @@ -79,7 +81,7 @@ proc test {hidden dlopen version_id_main lang} {
>  
>      clean_restart $executable
>      gdb_load_shlib $libobj1
> -    gdb_load_shlib $libobj2
> +    gdb_load_shlib $libobj2 -no-download
>  
>      if ![runto_main] {
>  	return -1
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index e2cda30b95a..5328afb27e8 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -5873,24 +5873,46 @@ proc gdb_remote_download {dest fromfile {tofile {}}} {
>  #
>  # Copy the listed library to the target.
>  
> -proc gdb_load_shlib { file } {
> +proc gdb_load_shlib { file args } {
>      global gdb_spawn_id

The comments on this function will need updating for a final version of
this patch.

Otherwise, looks good.

Thanks,
Andrew

>  
> -    if ![info exists gdb_spawn_id] {
> -	perror "gdb_load_shlib: GDB is not running"
> +    set download 1
> +    set solib-search-path 1
> +    parse_args {
> +	{no-download}
> +	{only-download}
> +    }
> +    if { ${no-download} && ${only-download} } {
> +	perror \
> +	    "gdb_load_shlib: Cannot use both -no-download and -only-download"
> +    }
> +    if { ${no-download} } {
> +	set download 0
> +    }
> +    if { ${only-download} } {
> +	set solib-search-path 0
>      }
>  
> -    set dest [gdb_remote_download target [shlib_target_file $file]]
> +    set dest ""
> +    if { $download } {
> +	set dest [gdb_remote_download target [shlib_target_file $file]]
> +    }
>  
> -    if {[is_remote target]} {
> -	# If the target is remote, we need to tell gdb where to find the
> -	# libraries.
> -	#
> -	# We could set this even when not testing remotely, but a user
> -	# generally won't set it unless necessary.  In order to make the tests
> -	# more like the real-life scenarios, we don't set it for local testing.
> -	gdb_test "set solib-search-path [file dirname $file]" "" \
> -	    "set solib-search-path for [file tail $file]"
> +    if { ${solib-search-path} } {
> +	if ![info exists gdb_spawn_id] {
> +	    perror "gdb_load_shlib: GDB is not running"
> +	}
> +
> +	if {[is_remote target]} {
> +	    # If the target is remote, we need to tell gdb where to find the
> +	    # libraries.
> +	    #
> +	    # We could set this even when not testing remotely, but a user
> +	    # generally won't set it unless necessary.  In order to make the tests
> +	    # more like the real-life scenarios, we don't set it for local testing.
> +	    gdb_test "set solib-search-path [file dirname $file]" "" \
> +		"set solib-search-path for [file tail $file]"
> +	}
>      }
>  
>      return $dest
> -- 
> 2.35.3