[PATCH v2 1/2] Improve gdb_remote_download, remove gdb_download

[PATCH v2 1/2] Improve gdb_remote_download, remove gdb_download

[Simon Marchi]

This patch removes gdb_download in favor of gdb_remote_download, since
they are very close in functionality.  Also, in preparation for the
following patch about shared library handling during tests, it improves
gdb_remote_download so that it uses standard_output_file for any
destination board that is local, not only host.

If the destination board is remote, gdb_remote_download will use the
standard remote_download from DejaGnu, resulting in the file being
transferred on the remote system.

If the destination is local, gdb_remote_download will copy the file to
the standard test directory (found using standard_output_file).  Tcl's
file copy seems to handle gracefully cases where the source file is the
same as the destination, so I don't think it's necessary to check for
that case ourselves, as a previous version of the patch did.

I'd prefer to keep the name gdb_download instead of gdb_remote_download,
since I don't like the fact that gdb_remote_download implies that the
destination is remote, when it's not always the case.  However,
gdb_remote_download is used at many more places than gdb_download, so
it's easier to reuse that.  Also, since it's a wrapper around DejaGnu's
remote_download, it might be better to keep that name.  I don't know.

I ran the testsuite native, with native-gdbserver and with a
remote gdbserver, and didn't see any related failure.

gdb/testsuite/ChangeLog:

	* gdb.base/jit-so.exp: Use gdb_remote_download instead of
	gdb_download.  Use it even if the target is not remote.
	* gdb.base/jit.exp (compile_jit_test): Likewise.
	* lib/gdb.exp (gdb_remote_download): Copy files to the standard
	output directory if the destination board is local, otherwise use
	the standard remote_download from DejaGnu.
	(gdb_download): Remove.
	(gdb_load_shlibs): Use gdb_remote_download instead of
	gdb_download.
	* lib/gdbserver-support.exp (gdbserver_download_current_prog):
	Use gdb_remote_download instead of gdb_download.  Use it even if
	the target is not remote.
	* lib/mi-support.exp (mi_load_shlibs): Use gdb_remote_download
	instead of gdb_download.
---
 gdb/testsuite/gdb.base/jit-so.exp       |  6 +---
 gdb/testsuite/gdb.base/jit.exp          |  6 +---
 gdb/testsuite/lib/gdb.exp               | 52 +++++++++++++++++++--------------
 gdb/testsuite/lib/gdbserver-support.exp |  6 +---
 gdb/testsuite/lib/mi-support.exp        |  2 +-
 5 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/gdb/testsuite/gdb.base/jit-so.exp b/gdb/testsuite/gdb.base/jit-so.exp
index adb21ea..0135473 100644
--- a/gdb/testsuite/gdb.base/jit-so.exp
+++ b/gdb/testsuite/gdb.base/jit-so.exp
@@ -61,11 +61,7 @@ if { [gdb_compile_shlib ${solib_srcfile} ${solib_binfile} {}] != "" } {
     return -1
 }
 
-if {[is_remote target]} {
-    set solib_binfile_target [gdb_download ${solib_binfile}]
-} else {
-    set solib_binfile_target $solib_binfile
-}
+set solib_binfile_target [gdb_remote_download target ${solib_binfile}]
 
 proc one_jit_test {count match_str} {
     with_test_prefix "one_jit_test-$count" {
diff --git a/gdb/testsuite/gdb.base/jit.exp b/gdb/testsuite/gdb.base/jit.exp
index da9449b..17024e4 100644
--- a/gdb/testsuite/gdb.base/jit.exp
+++ b/gdb/testsuite/gdb.base/jit.exp
@@ -57,11 +57,7 @@ proc compile_jit_test {testname binsuffix options} {
 	return -1
     }
 
-    if {[is_remote target]} {
-	set solib_binfile_target [gdb_download ${solib_binfile}]
-    } else {
-	set solib_binfile_target $solib_binfile
-    }
+    set solib_binfile_target [gdb_remote_download target ${solib_binfile}]
 
     return 0
 }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index a77bce4..8b0fd8a 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4183,34 +4183,42 @@ proc gdb_touch_execfile { binfile } {
     }
 }
 
-# Like remote_download but provides a gdb-specific behavior.  If DEST
-# is "host", and the host is not remote, and TOFILE is not specified,
-# then the [file tail] of FROMFILE is passed through
-# standard_output_file to compute the destination.
+# Like remote_download but provides a gdb-specific behavior.
+#
+# If the destination board is remote, the local file FROMFILE is transferred as
+# usual with remote_download to TOFILE on the reemote board.  The destination
+# filename is added to the CLEANFILES global, so it can be cleaned up at the
+# end of the test.
+#
+# If the destination board is local, the destination path TOFILE is passed
+# through standard_output_file, and FROMFILE is copied there.
+#
+# In both cases, if TOFILE is omitted, it defaults to the [file tail] of
+# FROMFILE.
 
 proc gdb_remote_download {dest fromfile {tofile {}}} {
-    if {$dest == "host" && ![is_remote host] && $tofile == ""} {
-	set tofile [standard_output_file [file tail $fromfile]]
+    # If TOFILE is not given, default to the same filename as FROMFILE.
+    if {[string length $tofile] == 0} {
+	set tofile [file tail $fromfile]
     }
 
-    if { $tofile == "" } {
-	return [remote_download $dest $fromfile]
-    } else {
-	return [remote_download $dest $fromfile $tofile]
-    }
-}
+    if {[is_remote $dest]} {
+	# When the DEST is remote, we simply send the file to the target.
+	global cleanfiles
 
-# gdb_download
-#
-# Copy a file to the remote target and return its target filename.
-# Schedule the file to be deleted at the end of this test.
+	set destname [remote_download target $fromfile $tofile]
+	lappend cleanfiles $destname
 
-proc gdb_download { filename } {
-    global cleanfiles
+	return $destname
+    } else {
+	# When the DEST is local, we copy the file to the test directory
+	# (where the executable is), except if that's already where it is.
+	set tofile [standard_output_file $tofile]
 
-    set destname [remote_download target $filename]
-    lappend cleanfiles $destname
-    return $destname
+	file copy -force $fromfile $tofile
+
+	return $tofile
+    }
 }
 
 # gdb_load_shlibs LIB...
@@ -4223,7 +4231,7 @@ proc gdb_load_shlibs { args } {
     }
 
     foreach file $args {
-	gdb_download [shlib_target_file $file]
+	gdb_remote_download target [shlib_target_file $file]
     }
 
     # Even if the target supplies full paths for shared libraries,
diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index 60ac3af..67a8333 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -176,11 +176,7 @@ proc gdbserver_download_current_prog { } {
     if { $reuse == 0 } {
 	set gdbserver_host_exec $host_exec
 	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]
     }
 
     return $gdbserver_server_exec
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index 7f9a3f5..cf3005d 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -1960,7 +1960,7 @@ proc mi_load_shlibs { args } {
     }
 
     foreach file $args {
-	gdb_download [shlib_target_file $file]
+	gdb_remote_download target [shlib_target_file $file]
     }
 
     # Even if the target supplies full paths for shared libraries,
-- 
2.8.0

[PATCH v2 2/2] Make ftrace tests work with remote targets

[Simon Marchi]

When we build a shared library for testing, it is built differently
whether it is meant for the local system or a remote one.  When it is
for the local system, the library is built with no SONAME.  So when the
executable is built, roughly in this way:

  $ gcc testfile.c /path/to/library.so

the executable will contain an absolute reference to the library.  For
example:

  $ readelf -a testsuite/gdb.python/py-shared | grep NEEDED
   0x0000000000000001 (NEEDED)             Shared library: [/home/emaisin/build/binutils-gdb/gdb/testsuite/gdb.python/py-shared-sl.sl]

When testing is done remotely, the absolute path obviously doesn't work.
Therefore, we build the library with an SONAME:

  $ readelf -a testsuite/gdb.python/py-shared-sl.sl | grep SONAME
   0x000000000000000e (SONAME)             Library soname: [py-shared-sl.sl]

which ends up in the executable's NEEDED field:

  $ readelf -a testsuite/gdb.python/py-shared | grep NEEDED
   0x0000000000000001 (NEEDED)             Shared library: [py-shared-sl.sl]

The executable and the library are then uploaded side-by-side on the
remote system.  To allow the dynamic linker to find the shared library,
we have to add the special RPATH value $ORIGIN, which tells it to search
in the executable's directory:

  $ readelf -a testsuite/gdb.python/py-shared | grep ORIGIN
   0x000000000000000f (RPATH)              Library rpath: [$ORIGIN]

The problem with the IPA library is that it doesn't have an SONAME,
making it very difficult to do testing on a remote board.  When a
test executable is linked with it, it contains an absolute reference to
the library path.  Therefore, unless the paths on the target are the
same as on the build system, it won't work.

To make it possible for tests using the IPA library to run test on
remote boards, I suggest adding an SONAME to libinproctrace.so.  I don't
think it should be a big problem for users.  All the libraries installed
on my system have an SONAME, so it should be fine if libinproctrace.so
does too.

As a consequence, native testing does not work anymore, since
executables do not contain the absolute path to the library anymore.  To
keep them working, we can have gdb_load_shlibs copy the library to the
test directory when testing natively.  That's done by modifying
gdb_load_shlibs.  We also have to add RPATH=$ORIGIN to executables, even
when testing natively.

I think it's a good change in general, as it reduces the differences
between testing a native and a remote target.  To further reduce those
differences, we can also always build test shared libraries with an
SONAME.

ftrace.exp and ftrace-lock.exp need to be modified slightly.  The code
checks that the IPA library is loaded using the absolute path on the
build machine.  That obviously doesn't work if the test is done
remotely, as the path will be different.  I changed the tests to only
search for the library basename (e.g. libinproctrace.so).

gdb/gdbserver/ChangeLog:

	* Makefile.in ($(IPA_LIB)): Set SONAME of the IPA lib.

gdb/testsuite/ChangeLog:

	* gdb.trace/ftrace-lock.exp: Check for IPA basename instead of
	absolute.
	* gdb.trace/ftrace.exp: Likewise.
	* lib/gdb.exp (gdb_compile): Set rpath $ORIGIN for non-remote
	targets as well.
	(gdb_compile_shlib): Set SONAME for non-remote targets as well.
	(gdb_load_shlibs): Copy libraries to test directory when testing
	natively.  Only set solib-search-path if testing remotely.
	* lib/mi-support.exp (mi_load_shlibs): Likewise.
---
 gdb/gdbserver/Makefile.in               |  2 +-
 gdb/testsuite/gdb.trace/ftrace-lock.exp |  2 +-
 gdb/testsuite/gdb.trace/ftrace.exp      |  2 +-
 gdb/testsuite/lib/gdb.exp               | 44 +++++++++++++++++----------------
 gdb/testsuite/lib/mi-support.exp        | 16 ++++++------
 5 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 953c07a..8bf3376 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -327,7 +327,7 @@ IPA_LIB=libinproctrace.so
 
 $(IPA_LIB): $(IPA_OBJS) ${ADD_DEPS} ${CDEPS}
 	rm -f $(IPA_LIB)
-	$(CC_LD) -shared -fPIC -Wl,--no-undefined $(INTERNAL_CFLAGS) \
+	$(CC_LD) -shared -fPIC -Wl,--soname=$(IPA_LIB) -Wl,--no-undefined $(INTERNAL_CFLAGS) \
 	$(INTERNAL_LDFLAGS) -o $(IPA_LIB) ${IPA_OBJS} -ldl -pthread
 
 # Put the proper machine-specific files first, so M-. on a machine
diff --git a/gdb/testsuite/gdb.trace/ftrace-lock.exp b/gdb/testsuite/gdb.trace/ftrace-lock.exp
index 0b73086..077a261 100644
--- a/gdb/testsuite/gdb.trace/ftrace-lock.exp
+++ b/gdb/testsuite/gdb.trace/ftrace-lock.exp
@@ -64,7 +64,7 @@ if ![runto_main] {
     return -1
 }
 
-if { [gdb_test "info sharedlibrary" ".*${libipa}.*" "IPA loaded"] != 0 } {
+if { [gdb_test "info sharedlibrary" ".*[file tail ${libipa}].*" "IPA loaded"] != 0 } {
     untested "Could not find IPA lib loaded"
     return 1
 }
diff --git a/gdb/testsuite/gdb.trace/ftrace.exp b/gdb/testsuite/gdb.trace/ftrace.exp
index 4736f0f..23e7d1e 100644
--- a/gdb/testsuite/gdb.trace/ftrace.exp
+++ b/gdb/testsuite/gdb.trace/ftrace.exp
@@ -213,7 +213,7 @@ proc test_ftrace_condition { condexp var list } \
 
 gdb_reinitialize_dir $srcdir/$subdir
 
-if { [gdb_test "info sharedlibrary" ".*${libipa}.*" "IPA loaded"] != 0 } {
+if { [gdb_test "info sharedlibrary" ".*[file tail ${libipa}].*" "IPA loaded"] != 0 } {
     untested "Could not find IPA lib loaded"
     return 1
 }
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 8b0fd8a..264ca7c 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3369,12 +3369,10 @@ proc gdb_compile {source dest type options} {
         }
     }
 
-    # We typically link to shared libraries using an absolute path, and
-    # that's how they are found at runtime.  If we are going to
-    # dynamically load one by basename, we must specify rpath.  If we
-    # are using a remote host, DejaGNU will link to the shared library
-    # using a relative path, so again we must specify an rpath.
-    if { $shlib_load || ($shlib_found && [is_remote target]) } {
+    # Because we link with libraries using their basename, we may need
+    # (depending on the platform) to set a special rpath value, to allow
+    # the executable to find the libraries it depends on.
+    if { $shlib_load || $shlib_found } {
 	if { ([istarget "*-*-mingw*"]
 	      || [istarget *-*-cygwin*]
 	      || [istarget *-*-pe*]) } {
@@ -3585,14 +3583,16 @@ proc gdb_compile_shlib {sources dest options} {
 		set name ${dest}
 	    }
 	    lappend link_options "additional_flags=-Wl,--out-implib,${name}.a"
-	} elseif [is_remote target] {
-	    # By default, we do not set the soname.  This causes the linker
-	    # on ELF systems to create a DT_NEEDED entry in the executable
-	    # refering to the full path name of the library.  This is a
-	    # problem in remote testing if the library is in a different
-	    # directory there.  To fix this, we set a soname of just the
-	    # base filename for the library, and add an appropriate -rpath
-	    # to the main executable (in gdb_compile).
+	} else {
+	    # Set the soname of the library.  This causes the linker on ELF
+	    # systems to create the DT_NEEDED entry in the executable referring
+	    # to the soname of the library, and not its absolute path.  This
+	    # (using the absolute path) would be problem when testing on a
+	    # remote target.
+	    #
+	    # In conjunction with setting the soname, we add the special
+	    # rpath=$ORIGIN value when building the executable, so that it's
+	    # able to find the library in its own directory.
 	    set destbase [file tail $dest]
 	    lappend link_options "additional_flags=-Wl,-soname,$destbase"
 	}
@@ -4226,17 +4226,19 @@ proc gdb_remote_download {dest fromfile {tofile {}}} {
 # Copy the listed libraries to the target.
 
 proc gdb_load_shlibs { args } {
-    if {![is_remote target]} {
-	return
-    }
-
     foreach file $args {
 	gdb_remote_download target [shlib_target_file $file]
     }
 
-    # Even if the target supplies full paths for shared libraries,
-    # they may not be paths for this system.
-    gdb_test "set solib-search-path [file dirname [lindex $args 0]]" "" ""
+    if {[is_remote target]} {
+	# If the target is remote, we need to tell gdb where to find the
+	# libraries.
+	#
+	# We could set this even when not testing remotely, but a user
+	# generally won't set it unless necessary.  In order to make the tests
+	# more like the real-life scenarios, we don't set it for local testing.
+	gdb_test "set solib-search-path [file dirname [lindex $args 0]]" "" ""
+    }
 }
 
 #
diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp
index cf3005d..87ce634 100644
--- a/gdb/testsuite/lib/mi-support.exp
+++ b/gdb/testsuite/lib/mi-support.exp
@@ -1955,17 +1955,19 @@ proc check_mi_and_console_threads {name} {
 
 # Download shared libraries to the target.
 proc mi_load_shlibs { args } {
-    if {![is_remote target]} {
-	return
-    }
-
     foreach file $args {
 	gdb_remote_download target [shlib_target_file $file]
     }
 
-    # Even if the target supplies full paths for shared libraries,
-    # they may not be paths for this system.
-    mi_gdb_test "set solib-search-path [file dirname [lindex $args 0]]" "\^done" ""
+    if {[is_remote target]} {
+	# If the target is remote, we need to tell gdb where to find the
+	# libraries.
+	#
+	# We could set this even when not testing remotely, but a user
+	# generally won't set it unless necessary.  In order to make the tests
+	# more like the real-life scenarios, we don't set it for local testing.
+	mi_gdb_test "set solib-search-path [file dirname [lindex $args 0]]" "\^done" ""
+    }
 }
 
 proc mi_check_thread_states { states test } {
-- 
2.8.0

Re: [PATCH v2 1/2] Improve gdb_remote_download, remove gdb_download

[Pedro Alves]

On 04/04/2016 07:30 PM, Simon Marchi wrote:

>  
> -# Like remote_download but provides a gdb-specific behavior.  If DEST
> -# is "host", and the host is not remote, and TOFILE is not specified,
> -# then the [file tail] of FROMFILE is passed through
> -# standard_output_file to compute the destination.
> +# Like remote_download but provides a gdb-specific behavior.
> +#
> +# If the destination board is remote, the local file FROMFILE is transferred as
> +# usual with remote_download to TOFILE on the reemote board.  The destination

Typo "reemote".

> +# filename is added to the CLEANFILES global, so it can be cleaned up at the
> +# end of the test.
> +#
> +# If the destination board is local, the destination path TOFILE is passed
> +# through standard_output_file, and FROMFILE is copied there.
> +#
> +# In both cases, if TOFILE is omitted, it defaults to the [file tail] of
> +# FROMFILE.
>  
>  proc gdb_remote_download {dest fromfile {tofile {}}} {
> -    if {$dest == "host" && ![is_remote host] && $tofile == ""} {
> -	set tofile [standard_output_file [file tail $fromfile]]
> +    # If TOFILE is not given, default to the same filename as FROMFILE.
> +    if {[string length $tofile] == 0} {
> +	set tofile [file tail $fromfile]
>      }
>  
> -    if { $tofile == "" } {
> -	return [remote_download $dest $fromfile]
> -    } else {
> -	return [remote_download $dest $fromfile $tofile]
> -    }
> -}
> +    if {[is_remote $dest]} {
> +	# When the DEST is remote, we simply send the file to the target.
> +	global cleanfiles

Shouldn't this be, "When the DEST is remote, we simply send the
file to DEST" ?.

>  
> -# gdb_download
> -#
> -# Copy a file to the remote target and return its target filename.
> -# Schedule the file to be deleted at the end of this test.
> +	set destname [remote_download target $fromfile $tofile]

and thus here be "remote_download $dest" ?

> +	lappend cleanfiles $destname

Thanks,
Pedro Alves

Re: [PATCH v2 2/2] Make ftrace tests work with remote targets

[Pedro Alves]

On 04/04/2016 07:30 PM, Simon Marchi wrote:
> When we build a shared library for testing, it is built differently
> whether it is meant for the local system or a remote one.  When it is
> for the local system, the library is built with no SONAME.  So when the
> executable is built, roughly in this way:
> 
>   $ gcc testfile.c /path/to/library.so
> 
> the executable will contain an absolute reference to the library.  For
> example:
> 
>   $ readelf -a testsuite/gdb.python/py-shared | grep NEEDED
>    0x0000000000000001 (NEEDED)             Shared library: [/home/emaisin/build/binutils-gdb/gdb/testsuite/gdb.python/py-shared-sl.sl]
> 
> When testing is done remotely, the absolute path obviously doesn't work.
> Therefore, we build the library with an SONAME:
> 
>   $ readelf -a testsuite/gdb.python/py-shared-sl.sl | grep SONAME
>    0x000000000000000e (SONAME)             Library soname: [py-shared-sl.sl]
> 
> which ends up in the executable's NEEDED field:
> 
>   $ readelf -a testsuite/gdb.python/py-shared | grep NEEDED
>    0x0000000000000001 (NEEDED)             Shared library: [py-shared-sl.sl]
> 
> The executable and the library are then uploaded side-by-side on the
> remote system.  To allow the dynamic linker to find the shared library,
> we have to add the special RPATH value $ORIGIN, which tells it to search
> in the executable's directory:
> 
>   $ readelf -a testsuite/gdb.python/py-shared | grep ORIGIN
>    0x000000000000000f (RPATH)              Library rpath: [$ORIGIN]
> 
> The problem with the IPA library is that it doesn't have an SONAME,
> making it very difficult to do testing on a remote board.  When a
> test executable is linked with it, it contains an absolute reference to
> the library path.  Therefore, unless the paths on the target are the
> same as on the build system, it won't work.
> 
> To make it possible for tests using the IPA library to run test on
> remote boards, I suggest adding an SONAME to libinproctrace.so.  I don't
> think it should be a big problem for users.  All the libraries installed
> on my system have an SONAME, so it should be fine if libinproctrace.so
> does too.
> 
> As a consequence, native testing does not work anymore, since
> executables do not contain the absolute path to the library anymore.  To
> keep them working, we can have gdb_load_shlibs copy the library to the
> test directory when testing natively.  That's done by modifying
> gdb_load_shlibs.  We also have to add RPATH=$ORIGIN to executables, even
> when testing natively.
> 
> I think it's a good change in general, as it reduces the differences
> between testing a native and a remote target.  To further reduce those
> differences, we can also always build test shared libraries with an
> SONAME.
> 
> ftrace.exp and ftrace-lock.exp need to be modified slightly.  The code
> checks that the IPA library is loaded using the absolute path on the
> build machine.  That obviously doesn't work if the test is done
> remotely, as the path will be different.  I changed the tests to only
> search for the library basename (e.g. libinproctrace.so).
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* Makefile.in ($(IPA_LIB)): Set SONAME of the IPA lib.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.trace/ftrace-lock.exp: Check for IPA basename instead of
> 	absolute.
> 	* gdb.trace/ftrace.exp: Likewise.
> 	* lib/gdb.exp (gdb_compile): Set rpath $ORIGIN for non-remote
> 	targets as well.
> 	(gdb_compile_shlib): Set SONAME for non-remote targets as well.
> 	(gdb_load_shlibs): Copy libraries to test directory when testing
> 	natively.  Only set solib-search-path if testing remotely.
> 	* lib/mi-support.exp (mi_load_shlibs): Likewise.

OK.

Thanks,
Pedro Alves

Re: [PATCH v2 1/2] Improve gdb_remote_download, remove gdb_download

[Simon Marchi]

On 16-04-05 01:47 PM, Pedro Alves wrote:
> On 04/04/2016 07:30 PM, Simon Marchi wrote:
> 
>>  
>> -# Like remote_download but provides a gdb-specific behavior.  If DEST
>> -# is "host", and the host is not remote, and TOFILE is not specified,
>> -# then the [file tail] of FROMFILE is passed through
>> -# standard_output_file to compute the destination.
>> +# Like remote_download but provides a gdb-specific behavior.
>> +#
>> +# If the destination board is remote, the local file FROMFILE is transferred as
>> +# usual with remote_download to TOFILE on the reemote board.  The destination
> 
> Typo "reemote".

Fixed.

>> +# filename is added to the CLEANFILES global, so it can be cleaned up at the
>> +# end of the test.
>> +#
>> +# If the destination board is local, the destination path TOFILE is passed
>> +# through standard_output_file, and FROMFILE is copied there.
>> +#
>> +# In both cases, if TOFILE is omitted, it defaults to the [file tail] of
>> +# FROMFILE.
>>  
>>  proc gdb_remote_download {dest fromfile {tofile {}}} {
>> -    if {$dest == "host" && ![is_remote host] && $tofile == ""} {
>> -	set tofile [standard_output_file [file tail $fromfile]]
>> +    # If TOFILE is not given, default to the same filename as FROMFILE.
>> +    if {[string length $tofile] == 0} {
>> +	set tofile [file tail $fromfile]
>>      }
>>  
>> -    if { $tofile == "" } {
>> -	return [remote_download $dest $fromfile]
>> -    } else {
>> -	return [remote_download $dest $fromfile $tofile]
>> -    }
>> -}
>> +    if {[is_remote $dest]} {
>> +	# When the DEST is remote, we simply send the file to the target.
>> +	global cleanfiles
> 
> Shouldn't this be, "When the DEST is remote, we simply send the
> file to DEST" ?.

Right.

>>  
>> -# gdb_download
>> -#
>> -# Copy a file to the remote target and return its target filename.
>> -# Schedule the file to be deleted at the end of this test.
>> +	set destname [remote_download target $fromfile $tofile]
> 
> and thus here be "remote_download $dest" ?

Right.  I tested this procedure with a remote host, but with the target being
the same as the host, so it was working anyway...

Fixed.

Re: [PATCH v2 1/2] Improve gdb_remote_download, remove gdb_download

[Pedro Alves]

On 04/05/2016 06:54 PM, Simon Marchi wrote:
> On 16-04-05 01:47 PM, Pedro Alves wrote:

>> Shouldn't this be, "When the DEST is remote, we simply send the
>> file to DEST" ?.
> 
> Right.
> 
>>>  
>>> -# gdb_download
>>> -#
>>> -# Copy a file to the remote target and return its target filename.
>>> -# Schedule the file to be deleted at the end of this test.
>>> +	set destname [remote_download target $fromfile $tofile]
>>
>> and thus here be "remote_download $dest" ?
> 
> Right.  I tested this procedure with a remote host, but with the target being
> the same as the host, so it was working anyway...
> 
> Fixed.

OK with those fixed then.

Thanks,
Pedro Alves

Re: [PATCH v2 1/2] Improve gdb_remote_download, remove gdb_download

[Simon Marchi]

On 16-04-05 01:56 PM, Pedro Alves wrote:
> On 04/05/2016 06:54 PM, Simon Marchi wrote:
>> On 16-04-05 01:47 PM, Pedro Alves wrote:
> 
>>> Shouldn't this be, "When the DEST is remote, we simply send the
>>> file to DEST" ?.
>>
>> Right.
>>
>>>>  
>>>> -# gdb_download
>>>> -#
>>>> -# Copy a file to the remote target and return its target filename.
>>>> -# Schedule the file to be deleted at the end of this test.
>>>> +	set destname [remote_download target $fromfile $tofile]
>>>
>>> and thus here be "remote_download $dest" ?
>>
>> Right.  I tested this procedure with a remote host, but with the target being
>> the same as the host, so it was working anyway...
>>
>> Fixed.
> 
> OK with those fixed then.
> 
> Thanks,
> Pedro Alves
> 

Pushed, thanks!

Re: [PATCH v2 2/2] Make ftrace tests work with remote targets

[Simon Marchi]

On 16-04-05 01:48 PM, Pedro Alves wrote:
> On 04/04/2016 07:30 PM, Simon Marchi wrote:
>> When we build a shared library for testing, it is built differently
>> whether it is meant for the local system or a remote one.  When it is
>> for the local system, the library is built with no SONAME.  So when the
>> executable is built, roughly in this way:
>>
>>   $ gcc testfile.c /path/to/library.so
>>
>> the executable will contain an absolute reference to the library.  For
>> example:
>>
>>   $ readelf -a testsuite/gdb.python/py-shared | grep NEEDED
>>    0x0000000000000001 (NEEDED)             Shared library: [/home/emaisin/build/binutils-gdb/gdb/testsuite/gdb.python/py-shared-sl.sl]
>>
>> When testing is done remotely, the absolute path obviously doesn't work.
>> Therefore, we build the library with an SONAME:
>>
>>   $ readelf -a testsuite/gdb.python/py-shared-sl.sl | grep SONAME
>>    0x000000000000000e (SONAME)             Library soname: [py-shared-sl.sl]
>>
>> which ends up in the executable's NEEDED field:
>>
>>   $ readelf -a testsuite/gdb.python/py-shared | grep NEEDED
>>    0x0000000000000001 (NEEDED)             Shared library: [py-shared-sl.sl]
>>
>> The executable and the library are then uploaded side-by-side on the
>> remote system.  To allow the dynamic linker to find the shared library,
>> we have to add the special RPATH value $ORIGIN, which tells it to search
>> in the executable's directory:
>>
>>   $ readelf -a testsuite/gdb.python/py-shared | grep ORIGIN
>>    0x000000000000000f (RPATH)              Library rpath: [$ORIGIN]
>>
>> The problem with the IPA library is that it doesn't have an SONAME,
>> making it very difficult to do testing on a remote board.  When a
>> test executable is linked with it, it contains an absolute reference to
>> the library path.  Therefore, unless the paths on the target are the
>> same as on the build system, it won't work.
>>
>> To make it possible for tests using the IPA library to run test on
>> remote boards, I suggest adding an SONAME to libinproctrace.so.  I don't
>> think it should be a big problem for users.  All the libraries installed
>> on my system have an SONAME, so it should be fine if libinproctrace.so
>> does too.
>>
>> As a consequence, native testing does not work anymore, since
>> executables do not contain the absolute path to the library anymore.  To
>> keep them working, we can have gdb_load_shlibs copy the library to the
>> test directory when testing natively.  That's done by modifying
>> gdb_load_shlibs.  We also have to add RPATH=$ORIGIN to executables, even
>> when testing natively.
>>
>> I think it's a good change in general, as it reduces the differences
>> between testing a native and a remote target.  To further reduce those
>> differences, we can also always build test shared libraries with an
>> SONAME.
>>
>> ftrace.exp and ftrace-lock.exp need to be modified slightly.  The code
>> checks that the IPA library is loaded using the absolute path on the
>> build machine.  That obviously doesn't work if the test is done
>> remotely, as the path will be different.  I changed the tests to only
>> search for the library basename (e.g. libinproctrace.so).
>>
>> gdb/gdbserver/ChangeLog:
>>
>> 	* Makefile.in ($(IPA_LIB)): Set SONAME of the IPA lib.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 	* gdb.trace/ftrace-lock.exp: Check for IPA basename instead of
>> 	absolute.
>> 	* gdb.trace/ftrace.exp: Likewise.
>> 	* lib/gdb.exp (gdb_compile): Set rpath $ORIGIN for non-remote
>> 	targets as well.
>> 	(gdb_compile_shlib): Set SONAME for non-remote targets as well.
>> 	(gdb_load_shlibs): Copy libraries to test directory when testing
>> 	natively.  Only set solib-search-path if testing remotely.
>> 	* lib/mi-support.exp (mi_load_shlibs): Likewise.
> 
> OK.
> 
> Thanks,
> Pedro Alves
> 

Pushed as well, thanks!

Re: [PATCH v2 2/2] Make ftrace tests work with remote targets

[Yao Qi]

Simon Marchi <simon.marchi@ericsson.com> writes:

> The executable and the library are then uploaded side-by-side on the
> remote system.  To allow the dynamic linker to find the shared library,
> we have to add the special RPATH value $ORIGIN, which tells it to search
> in the executable's directory:
>
>   $ readelf -a testsuite/gdb.python/py-shared | grep ORIGIN
>    0x000000000000000f (RPATH)              Library rpath: [$ORIGIN]
>
> The problem with the IPA library is that it doesn't have an SONAME,
> making it very difficult to do testing on a remote board.  When a
> test executable is linked with it, it contains an absolute reference to
> the library path.  Therefore, unless the paths on the target are the
> same as on the build system, it won't work.

Yes, that is how fast tracepoint works for me.  My build and target have
the same directory hierarchy.  In fact, target /scratch is mounted to
build /scratch through nfs.  That is quite convenient, and don't need to
copy binaries and libraries from host to target.  However, this commit
breaks my testing.  It also breaks the typical testing with
native-gdbserver.  See https://sourceware.org/ml/gdb-testers/2016-q2/msg00162.html

>
> To make it possible for tests using the IPA library to run test on
> remote boards, I suggest adding an SONAME to libinproctrace.so.  I don't
> think it should be a big problem for users.  All the libraries installed
> on my system have an SONAME, so it should be fine if libinproctrace.so
> does too.

That is fine to add SONAME...

>
> As a consequence, native testing does not work anymore, since
> executables do not contain the absolute path to the library anymore.  To
> keep them working, we can have gdb_load_shlibs copy the library to the
> test directory when testing natively.  That's done by modifying
> gdb_load_shlibs.  We also have to add RPATH=$ORIGIN to executables, even
> when testing natively.

However, setting "RPATH=$ORIGIN" only works for your testing env,
because executable and libraries can be downloaded to the different
places on the target.

-- 
Yao (齐尧)

Re: [PATCH v2 2/2] Make ftrace tests work with remote targets

[Simon Marchi]

On 16-04-11 09:12 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>> The executable and the library are then uploaded side-by-side on the
>> remote system.  To allow the dynamic linker to find the shared library,
>> we have to add the special RPATH value $ORIGIN, which tells it to search
>> in the executable's directory:
>>
>>   $ readelf -a testsuite/gdb.python/py-shared | grep ORIGIN
>>    0x000000000000000f (RPATH)              Library rpath: [$ORIGIN]
>>
>> The problem with the IPA library is that it doesn't have an SONAME,
>> making it very difficult to do testing on a remote board.  When a
>> test executable is linked with it, it contains an absolute reference to
>> the library path.  Therefore, unless the paths on the target are the
>> same as on the build system, it won't work.
> 
> Yes, that is how fast tracepoint works for me.  My build and target have
> the same directory hierarchy.  In fact, target /scratch is mounted to
> build /scratch through nfs.  That is quite convenient, and don't need to
> copy binaries and libraries from host to target.

I agree it's convenient, and I have a similar setup (although with sshfs), but I
don't think it should be a requirement.  For some people it might not be possible
or easy to mount the same paths on both systems.

> However, this commit
> breaks my testing.  It also breaks the typical testing with
> native-gdbserver.  See https://sourceware.org/ml/gdb-testers/2016-q2/msg00162.html

Right, that's because the native-gdbserver board advertises itself as being remote, but
overrides the _download procedure to be a no-op (see gdbserver-base.exp).  So, the
libinproctrace.exp file is not copied to the output directory.

That test passes if you apply:

https://sourceware.org/ml/gdb-patches/2016-04/msg00112.html

>> To make it possible for tests using the IPA library to run test on
>> remote boards, I suggest adding an SONAME to libinproctrace.so.  I don't
>> think it should be a big problem for users.  All the libraries installed
>> on my system have an SONAME, so it should be fine if libinproctrace.so
>> does too.
> 
> That is fine to add SONAME...

Ok.

>>
>> As a consequence, native testing does not work anymore, since
>> executables do not contain the absolute path to the library anymore.  To
>> keep them working, we can have gdb_load_shlibs copy the library to the
>> test directory when testing natively.  That's done by modifying
>> gdb_load_shlibs.  We also have to add RPATH=$ORIGIN to executables, even
>> when testing natively.
> 
> However, setting "RPATH=$ORIGIN" only works for your testing env,
> because executable and libraries can be downloaded to the different
> places on the target.

When running the testsuite?  In which cases does this happen?

All solib tests rely on RPATH=$ORIGIN and the executable being uploaded at
the same location than the libraries, don't they?  Why should it be different
for ftrace tests?

Thanks for the feedback,

Simon