Re: [PATCH] Fix gdb.server/solib-list.exp regression

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 16-04-07 07:07 PM, Pedro Alves wrote:
> On 04/07/2016 09:32 PM, Simon Marchi wrote:
> 
>> I am not sure how I feel about this.  Here is some brain dump, but in the end it's your
>> call.
> 
> I appreciate the comments and ideas very much.  Keep them coming.
> 
>>
>> I wouldn't see it as a problem that gdb and gdbserver are not started with the same
>> filename, as long as the files are identical.  After all, when they run on different
>> systems, they are not loading the same file.
> 
> It's not a big issue, but when running locally, they will normally be the
> same file (I mean, when actually debugging things, not the testsuite).  I guess
> if we tried hard, we could come up with some test that might want to
> exercise something around argv[0], perhaps.
> 
>>
>> So another solution could be to make gdb_remote_download get the realpath of its source
>> path, so that we don't copy a symlink in the output directory (it doesn't sound like
>> something we would ever want to do).  Maybe it's more generic to handle it at this level?
>> This way, if there is another instance of a symlink being copied, we won't have to add
>> such a workaround.
>>
>> Also, it follows the idea (which I like) of having all the artifacts involved in a test
>> case in the test's output directory.  Similarly, we wouldn't need to copy Python .py files
>> over to the output directory, but I find it nice that everything that is used gets copied
>> at the same place.
> 
> I think we need to draw a line somewhere.  We don't copy test program system dependencies
> like libc.so.6, or the kernel image to the output directory either, since they're
> environment / system components, not something we build ourselves.  I think the 
> interpreter is in the same boat.  Agree?

Right, I thought that in that case, the program interpreter was a test subject, but it's
not.  I think I finally understand what the test is really about.  We launch the binary
in this indirect way to prevent gdbserver from reading the solib debug info directly from
the binary.  So we don't actually care about the interpreter.

>>
>> Another idea, I see that the test doesn't support remote testing right now:
>>
>>   # This test case (currently) does not support remote targets, since it
>>   # assumes the ELF interpreter can be found on the host system
>>   if [is_remote target] then {
>>       return
>>   }
>>
>> Maybe the more "definitive" solution would be to make it work with a real remote target?
> 
> I agree that it'd be nice to make this work with real remote targets.
> 
>> Then, we would have to do a remote_upload of the interpreter from the target to the output
>> directory on the build machine.  
> 
> Hmm, actually, there's no need to upload the interpreter to the host.  gdb does not
> use it at all.  It's only gdbserver that is started with the interpreter as program
> to debug.  GDB loads $binfile.  Even if we needed to upload the interpreter, I'd
> say that there'd be no need to download it to the target again; it's already there.
> 
> It is sounding to me like the test should just not use gdb_load (and gdb_file_cmd as
> consequence) at all, and instead pass the interpreter path to gdbserver as
> an argument.  I tried it (patch below), and it works.  I imagine this makes the
> test work when testing with remote gdbserver boards, if we add a
> "gdb_remote_download target $binfile" call (though I haven't tried it).
> 
> I'm liking this approach even better.  WDYT?

This approach looks fine, and should work with a remote target (in theory).

I tried to add a

  set remote_binfile [gdb_remote_download target $binfile]

and change the gdbserver_spawn to

  set res [gdbserver_spawn "${interp_system} ${remote_binfile}"]

but it didn't work out of the box.  I'll come back to it soon.  But in the mean time,
I think you can push your patch, which makes it work for you locally.

> From 2551e07a66266cca7c7bc9c0c8e65ff35de663b5 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 7 Apr 2016 23:36:50 +0100
> Subject: [PATCH] solib-list
> 
> ---
>  gdb/testsuite/gdb.server/solib-list.exp | 9 ++++++---
>  gdb/testsuite/lib/gdbserver-support.exp | 4 ++++
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.server/solib-list.exp b/gdb/testsuite/gdb.server/solib-list.exp
> index fcd6d25..bbbdde6 100644
> --- a/gdb/testsuite/gdb.server/solib-list.exp
> +++ b/gdb/testsuite/gdb.server/solib-list.exp
> @@ -57,7 +57,6 @@ foreach nonstop { 0 1 } { with_test_prefix "non-stop $nonstop" {
>      gdb_exit
>      gdb_start
>      gdb_reinitialize_dir $srcdir/$subdir
> -    gdb_load ${interp_system}
>      gdb_load_shlibs ${binfile}
>      gdb_load_shlibs ${binlibfile}
>  
> @@ -72,8 +71,12 @@ foreach nonstop { 0 1 } { with_test_prefix "non-stop $nonstop" {
>      # But GDB having symbols from the main executable it would try to use
>      # displaced-stepping buffer at unmapped that time address _start.
>      gdb_test "set displaced-stepping off"
> -	
> -    set res [gdbserver_spawn ${binfile}]
> +
> +    # Note we pass ${interp_system}, the program gdbserver spawns, as
> +    # argument here, instead of using gdb_load, because we don't want
> +    # to download the interpreter to the target (it's already there)
> +    # or the the test output directory.
> +    set res [gdbserver_spawn "${interp_system} ${binfile}"]
>      set gdbserver_protocol [lindex $res 0]
>      set gdbserver_gdbport [lindex $res 1]
>  
> diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
> index 67a8333..951afe5 100644
> --- a/gdb/testsuite/lib/gdbserver-support.exp
> +++ b/gdb/testsuite/lib/gdbserver-support.exp
> @@ -155,6 +155,10 @@ proc gdbserver_download_current_prog { } {
>      global gdbserver_server_exec
>      global last_loaded_file
>  
> +    if { ![info exists last_loaded_file] } {
> +	return ""
> +    }
> +
>      set host_exec $last_loaded_file
>  
>      # If we already downloaded a file to the target, see if we can reuse it.
>