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

[Tom de Vries <tdevries@suse.de>]

On 11/7/22 14:32, Andrew Burgess wrote:
> 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.
> 

Thanks for confirming.

>>
>> 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.

Ack, and thanks for the review.

Thanks,
- Tom