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:55, Simon Marchi wrote:
> 
> 
> On 11/6/22 05:54, Tom de Vries via Gdb-patches wrote:
>> 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.
>>
>> Ivan, could you check if the patch fixes the gdb.base/print-file-var.exp test-case for you?
> 
> I just gave it a quick look and I'm not familiar with the entire
> problem.  But it sounds to me like it would be clearer to just make two
> procs for the two steps, instead of one proc with flags to control its
> behavior.  And perhaps a third proc that does both steps, for
> convenience (which would be called gdb_load_shlib for backwards
> compatibility, I guess).

Thanks for the review.

Two-proc approach proposed here ( 
https://sourceware.org/pipermail/gdb-patches/2022-November/193522.html ).

Thanks,
- Tom