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

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

On 16-04-07 07:12 AM, Pedro Alves wrote:
> Commit 7817ea46148d (Improve gdb_remote_download, remove gdb_download)
> caused:
> 
>  FAIL: gdb.server/solib-list.exp: non-stop 0: target extended-remote (timeout)
>  FAIL: gdb.server/solib-list.exp: non-stop 0: continue (the program is no longer running)
>  FAIL: gdb.server/solib-list.exp: non-stop 0: p libvar
>  FAIL: gdb.server/solib-list.exp: non-stop 1: target extended-remote (timeout)
>  FAIL: gdb.server/solib-list.exp: non-stop 1: continue (the program is no longer running)
>  FAIL: gdb.server/solib-list.exp: non-stop 1: p libvar
> 
> gdb.log shows:
> 
>  system interpreter is: /lib64/ld-linux-x86-64.so.2
>  ...
>  spawn ../gdbserver/gdbserver --once :2347 /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.server/solib-list/ld-linux-x86-64.so.2 /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.server/solib-list/solib-list
>  Process /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.server/solib-list/ld-linux-x86-64.so.2 created; pid = 18637
>  Cannot exec /home/pedro/brno/pedro/gdb/mygit/build/gdb/testsuite/outputs/gdb.server/solib-list/ld-linux-x86-64.so.2: No such file or directory.
>  ...
> 
> The test copied the interpreter to the outputs directory, however
> ld-linux-x86-64.so.2 is a relative symlink that when copied points
> nowhere:
> 
>  $ ls -l testsuite/outputs/gdb.server/solib-list/
>  total 52
>  -rwxrwxr-x. 1 pedro pedro 13450 Apr  7 10:52 gdb.log
>  -rw-rw-r--. 1 pedro pedro  1512 Apr  7 10:52 gdb.sum
>  lrwxrwxrwx. 1 pedro pedro    10 Apr  7 11:39 ld-linux-x86-64.so.2 -> ld-2.22.so
>  -rwxrwxr-x. 1 pedro pedro  9464 Apr  7 11:39 solib-list
>  -rw-rw-r--. 1 pedro pedro  3472 Apr  7 11:39 solib-list-lib.c.o
>  -rw-rw-r--. 1 pedro pedro  2760 Apr  7 11:39 solib-list.o
>  -rwxrwxr-x. 1 pedro pedro  9232 Apr  7 11:39 solib-list.so

At least I have an excuse for not seeing this one :).  The link on Debian-based
distros seems to be absolute, so it works even when copying the link.

$ ls -l /lib64
lrwxrwxrwx 1 root root 32 Feb 16 14:29 ld-linux-x86-64.so.2 -> /lib/x86_64-linux-gnu/ld-2.19.so
$ ls -l testsuite/outputs/gdb.server/solib-list
lrwxrwxrwx 1 emaisin camorndithub   32 Apr  7 09:58 ld-linux-x86-64.so.2 -> /lib/x86_64-linux-gnu/ld-2.19.so
...

> The copying comes from gdbserver_spawn ->
> gdbserver_download_current_prog -> gdb_remote_download.
> 
> My first attempt at a fix made solib-list.exp resolve the interpreter
> symlink, so that the copying works, and that did make the test pass.
> 
> However, I noticed something else, which can be reduced to this:
> 
>   gdb_load ${binfile}
>   gdbserver_spawn
> 
> gdb_load passes ${binfile} unmodified to the "file" command.  If
> $binfile needs to be run through standard_output_file, that should be
> done before calling gdb_load, and it normally is.
> 
> In solib-list.exp's case, the ${binfile} is the system interpreter,
> outside the outputs directory.  Since gdbserver_download_current_prog
> copies the file to the outputs directory and runs it there, we'd end
> up with a mismatch between what gdbserver starts, and gdb loads.  So
> to address that we'd have to have the test do:
> 
>  +proc gdb_realpath {filename} {
>  +  ...
>  +}
> 
>   set interp_system [section_get ${binfile} .interp]
>  +set interp_system [gdb_realpath $interp_system]
>  +set interp_system [standard_output_file $interp_system]
> 
> But that all seems like pointless copying/work in the first place.  I
> think that instead, when local testing, we should make what gdbserver
> runs be exactly what was passed to gdb_load / gdb_file_cmd, which is
> what we used to do before commit 7817ea46148d (Improve
> gdb_remote_download, remove gdb_download), which did:
> 
> 	 set gdbserver_host_mtime [file mtime $host_exec]
>  -       if [is_remote target] {
>  -           set gdbserver_server_exec [gdb_download $host_exec]
>  -       } else {
>  -           set gdbserver_server_exec $host_exec
>  -       }
>  +       set gdbserver_server_exec [gdb_remote_download target $host_exec]
> 
> gdb/ChangeLog:
> 2016-04-07  Pedro Alves  <palves@redhat.com>
> 
> 	* lib/gdbserver-support.exp (gdbserver_download_current_prog):
> 	Skip download if the target is local.
> ---
>  gdb/testsuite/lib/gdbserver-support.exp | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
> index 67a8333..817331b 100644
> --- a/gdb/testsuite/lib/gdbserver-support.exp
> +++ b/gdb/testsuite/lib/gdbserver-support.exp
> @@ -176,7 +176,16 @@ proc gdbserver_download_current_prog { } {
>      if { $reuse == 0 } {
>  	set gdbserver_host_exec $host_exec
>  	set gdbserver_host_mtime [file mtime $host_exec]
> -	set gdbserver_server_exec [gdb_remote_download target $host_exec]
> +
> +	# If local testing, don't use gdb_remote_download, which would
> +	# copy the file to the testcase's output directory.  Use
> +	# LAST_LOADED_FILE as is, so that gdbserver starts the exact
> +	# same filename that will be handed to gdb's file command.
> +	if [is_remote target] {
> +	    set gdbserver_server_exec [gdb_remote_download target $host_exec]
> +	} else {
> +	    set gdbserver_server_exec $host_exec
> +	}
>      }
>  
>      return $gdbserver_server_exec

I am not sure how I feel about this.  Here is some brain dump, but in the end it's your
call.

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.

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.

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?
Then, we would have to do a remote_upload of the interpreter from the target to the output
directory on the build machine.  We would just have to make sure that DejaGnu's remote_upload
handles symlinks as we want to (i.e. that it doesn't just copy the link), and if it doesn't,
then we write a gdb_remote_upload wrapper.

Simon