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

[Pedro Alves <palves at redhat dot com>]

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?

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

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