[PATCH] gdbserver-base.exp: Copy file to standard output directory in ${board}_download

[PATCH] gdbserver-base.exp: Copy file to standard output directory in ${board}_download

[Simon Marchi]

gdbserver-base.exp is used as the base for both native-gdbserver.exp and
native-extended-gdbserver.exp.  (Despite its name, it should really be
considered as a "local-gdbserver-base", as it's not really appropriate to
implement a remote gdbserver board.)

Currently, the _download procedure is implemented as a no-op (it returns
the source file path).  Because of the SONAME change, The fast
tracepoint tests now require the executable and the IPA
(libinproctrace.so) to be located in the same directory (see [1]).  When
using the native-gdbserver board, because _download returns the original
file path, the executable does not end up in the same directory as the
library, and it fails to execute.

In more general terms, with the recent changes, the testsuite now
assumes that when it does

  ${board}_download <source path 1> <destination path 1>
  ${board}_download <source path 2> <destination path 2>

where the destination paths are relative (generally just the file name),
both files will end up in the same base directory.  That assumption does
not hold for the current implementation in gdbserver-base.exp.

The proper fix would be to make native-gdbserver non-remote, so that
gdb_remote_download would not call DejaGnu's remote_download (see [2]).
We could then get rid of ${board}_download in gdbserver-base.exp.
However, that will likely take some time to complete.  In the mean time,
in order to make the fast tracepoint tests pass, we can simply copy the
file to the standard output directory.  Basically, it just mimics what
gdb_remote_download would do if the board wasn't flagged as remote.

Note that I missed these failures originally because I had a
libinproctrace.so in /usr/local/lib.  So, even though libinproctrace.so
wasn't copied to the test output directory, it did find the one in
/usr/local/lib.  It would be nice to find a way to protect against this,
as it could easily happen again...

Regtested with unix, native-gdbserver and native-extended-gdbserver, and
didn't see anything notable, except the ftrace tests now passing for
native-gdbserver.

[1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=6e774b13c3b81ac2599812adf058796948ce7e95
[2] https://sourceware.org/ml/gdb-patches/2016-04/msg00112.html

gdb/testsuite/ChangeLog:

	* boards/gdbserver-base.exp (${board}_download): Copy source file to
	standard output directory.
---
 gdb/testsuite/boards/gdbserver-base.exp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/testsuite/boards/gdbserver-base.exp b/gdb/testsuite/boards/gdbserver-base.exp
index b686204..399713f 100644
--- a/gdb/testsuite/boards/gdbserver-base.exp
+++ b/gdb/testsuite/boards/gdbserver-base.exp
@@ -41,6 +41,7 @@ proc ${board}_file { dest op args } {
 }
 
 proc ${board}_download { board host dest } {
+    file copy -force $host [standard_output_file $dest]
     return $host
 }
 
-- 
2.8.1

Re: [PATCH] gdbserver-base.exp: Copy file to standard output directory in ${board}_download

[Pedro Alves]

On 04/12/2016 06:48 PM, Simon Marchi wrote:

>  proc ${board}_download { board host dest } {
> +    file copy -force $host [standard_output_file $dest]
>      return $host
>  }

I think this should be:

   set dest [standard_output_file $dest]
   file copy -force $host $dest
   return $dest

Don't we need to consider the case of $dest being an absolute path?

Thanks,
Pedro Alves

Re: [PATCH] gdbserver-base.exp: Copy file to standard output directory in ${board}_download

[Simon Marchi]

On 16-04-12 02:08 PM, Pedro Alves wrote:
> On 04/12/2016 06:48 PM, Simon Marchi wrote:
> 
>>  proc ${board}_download { board host dest } {
>> +    file copy -force $host [standard_output_file $dest]
>>      return $host
>>  }
> 
> I think this should be:
> 
>    set dest [standard_output_file $dest]
>    file copy -force $host $dest
>    return $dest

Doh, you're right.

> Don't we need to consider the case of $dest being an absolute path?

I thought it wouldn't be better not, since we don't even want for tests to
be able to write outside the standard output directory.  I guess that if a
test tries to download to /foo/bar, it will end up as
outputs/gdb.blah/thetest/foo/bar.  Then I would say the test would be at fault.

Note that we don't do it either in gdb_remote_download.  I guess we should
do it at both places or at none.


Updated patch:

From 2101a781ec30a46c797e56d6fcc6df8e4dcb7611 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Tue, 12 Apr 2016 11:31:58 -0400
Subject: [PATCH] gdbserver-base.exp: Copy file to standard output directory in
 ${board}_download

gdbserver-base.exp is used as the base for both native-gdbserver.exp and
native-extended-gdbserver.exp.  (Despite its name, it should really be
considered as a "local-gdbserver-base", as it's not really appropriate to
implement a remote gdbserver board.)

Currently, the _download procedure is implemented as a no-op (it returns
the source file path).  Because of the SONAME change, The fast
tracepoint tests now require the executable and the IPA
(libinproctrace.so) to be located in the same directory (see [1]).  When
using the native-gdbserver board, because _download returns the original
file path, the executable does not end up in the same directory as the
library, and it fails to execute.

In more general terms, with the recent changes, the testsuite now
assumes that when it does

  ${board}_download <source path 1> <destination path 1>
  ${board}_download <source path 2> <destination path 2>

where the destination paths are relative (generally just the file name),
both files will end up in the same base directory.  That assumption does
not hold for the current implementation in gdbserver-base.exp.

The proper fix would be to make native-gdbserver non-remote, so that
gdb_remote_download would not call DejaGnu's remote_download (see [2]).
We could then get rid of ${board}_download in gdbserver-base.exp.
However, that will likely take some time to complete.  In the mean time,
in order to make the fast tracepoint tests pass, we can simply copy the
file to the standard output directory.  Basically, it just mimics what
gdb_remote_download would do if the board wasn't flagged as remote.

Note that I missed these failures originally because I had a
libinproctrace.so in /usr/local/lib.  So, even though libinproctrace.so
wasn't copied to the test output directory, it did find the one in
/usr/local/lib.  It would be nice to find a way to protect against this,
as it could easily happen again...

Regtested with unix, native-gdbserver and native-extended-gdbserver, and
didn't see anything notable, except the ftrace tests now passing for
native-gdbserver.

[1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=6e774b13c3b81ac2599812adf058796948ce7e95
[2] https://sourceware.org/ml/gdb-patches/2016-04/msg00112.html

gdb/testsuite/ChangeLog:

	* boards/gdbserver-base.exp (${board}_download): Copy source file to
	standard output directory.
---
 gdb/testsuite/boards/gdbserver-base.exp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/boards/gdbserver-base.exp b/gdb/testsuite/boards/gdbserver-base.exp
index b686204..3579d25 100644
--- a/gdb/testsuite/boards/gdbserver-base.exp
+++ b/gdb/testsuite/boards/gdbserver-base.exp
@@ -41,7 +41,9 @@ proc ${board}_file { dest op args } {
 }

 proc ${board}_download { board host dest } {
-    return $host
+    set dest [standard_output_file $dest]
+    file copy -force $host $dest
+    return $dest
 }

 proc ${board}_upload {dest srcfile args} {
-- 
2.8.1

Re: [PATCH] gdbserver-base.exp: Copy file to standard output directory in ${board}_download

[Pedro Alves]

On 04/12/2016 07:36 PM, Simon Marchi wrote:
> On 16-04-12 02:08 PM, Pedro Alves wrote:
>> On 04/12/2016 06:48 PM, Simon Marchi wrote:
>>
>>>  proc ${board}_download { board host dest } {
>>> +    file copy -force $host [standard_output_file $dest]
>>>      return $host
>>>  }
>>
>> I think this should be:
>>
>>    set dest [standard_output_file $dest]
>>    file copy -force $host $dest
>>    return $dest
> 
> Doh, you're right.
> 
>> Don't we need to consider the case of $dest being an absolute path?
> 
> I thought it wouldn't be better not, since we don't even want for tests to
> be able to write outside the standard output directory.  I guess that if a
> test tries to download to /foo/bar, it will end up as
> outputs/gdb.blah/thetest/foo/bar.  Then I would say the test would be at fault.

OK, but I think we should put a comment to the effect in place.  After all,
we'll be pointing other people that run into this in their boards to this 
commit.  :-)

Otherwise LGTM.  (Let's wait a bit to give Yao a chance to comment though.)

Thanks,
Pedro Alves

Re: [PATCH] gdbserver-base.exp: Copy file to standard output directory in ${board}_download

[Simon Marchi]

On 16-04-12 02:45 PM, Pedro Alves wrote:
> On 04/12/2016 07:36 PM, Simon Marchi wrote:
>> On 16-04-12 02:08 PM, Pedro Alves wrote:
>>> On 04/12/2016 06:48 PM, Simon Marchi wrote:
>>>
>>>>  proc ${board}_download { board host dest } {
>>>> +    file copy -force $host [standard_output_file $dest]
>>>>      return $host
>>>>  }
>>>
>>> I think this should be:
>>>
>>>    set dest [standard_output_file $dest]
>>>    file copy -force $host $dest
>>>    return $dest
>>
>> Doh, you're right.
>>
>>> Don't we need to consider the case of $dest being an absolute path?
>>
>> I thought it wouldn't be better not, since we don't even want for tests to
>> be able to write outside the standard output directory.  I guess that if a
>> test tries to download to /foo/bar, it will end up as
>> outputs/gdb.blah/thetest/foo/bar.  Then I would say the test would be at fault.
> 
> OK, but I think we should put a comment to the effect in place.  After all,
> we'll be pointing other people that run into this in their boards to this 
> commit.  :-)
> 
> Otherwise LGTM.  (Let's wait a bit to give Yao a chance to comment though.)

Good idea.  And if that's ok, I'd put the same comment in gdb_remote_download.

What do you think about this?


From 4460bbb1367d910e48f4887fe2eb5372245200cc Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Tue, 12 Apr 2016 11:31:58 -0400
Subject: [PATCH] gdbserver-base.exp: Copy file to standard output directory in
 ${board}_download

gdbserver-base.exp is used as the base for both native-gdbserver.exp and
native-extended-gdbserver.exp.  (Despite its name, it should really be
considered as a "local-gdbserver-base", as it's not really appropriate to
implement a remote gdbserver board.)

Currently, the _download procedure is implemented as a no-op (it returns
the source file path).  Because of the SONAME change, The fast
tracepoint tests now require the executable and the IPA
(libinproctrace.so) to be located in the same directory (see [1]).  When
using the native-gdbserver board, because _download returns the original
file path, the executable does not end up in the same directory as the
library, and it fails to execute.

In more general terms, with the recent changes, the testsuite now
assumes that when it does

  ${board}_download <source path 1> <destination path 1>
  ${board}_download <source path 2> <destination path 2>

where the destination paths are relative (generally just the file name),
both files will end up in the same base directory.  That assumption does
not hold for the current implementation in gdbserver-base.exp.

The proper fix would be to make native-gdbserver non-remote, so that
gdb_remote_download would not call DejaGnu's remote_download (see [2]).
We could then get rid of ${board}_download in gdbserver-base.exp.
However, that will likely take some time to complete.  In the mean time,
in order to make the fast tracepoint tests pass, we can simply copy the
file to the standard output directory.  Basically, it just mimics what
gdb_remote_download would do if the board wasn't flagged as remote.

Note that I missed these failures originally because I had a
libinproctrace.so in /usr/local/lib.  So, even though libinproctrace.so
wasn't copied to the test output directory, it did find the one in
/usr/local/lib.  It would be nice to find a way to protect against this,
as it could easily happen again...

Regtested with unix, native-gdbserver and native-extended-gdbserver, and
didn't see anything notable, except the ftrace tests now passing for
native-gdbserver.

[1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=6e774b13c3b81ac2599812adf058796948ce7e95
[2] https://sourceware.org/ml/gdb-patches/2016-04/msg00112.html

gdb/testsuite/ChangeLog:

	* boards/gdbserver-base.exp (${board}_download): Copy source file to
	standard output directory.
---
 gdb/testsuite/boards/gdbserver-base.exp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/boards/gdbserver-base.exp b/gdb/testsuite/boards/gdbserver-base.exp
index b686204..02a9244 100644
--- a/gdb/testsuite/boards/gdbserver-base.exp
+++ b/gdb/testsuite/boards/gdbserver-base.exp
@@ -41,7 +41,14 @@ proc ${board}_file { dest op args } {
 }

 proc ${board}_download { board host dest } {
-    return $host
+    # We pass DEST in standard_output_file, regardless of whether it is absolute
+    # or relative, because we don't want the tests to be able to write outside
+    # their standard output directory.
+    set dest [standard_output_file $dest]
+
+    file copy -force $host $dest
+
+    return $dest
 }

 proc ${board}_upload {dest srcfile args} {
-- 
2.8.1

Re: [PATCH] gdbserver-base.exp: Copy file to standard output directory in ${board}_download

[Pedro Alves]

On 04/12/2016 08:44 PM, Simon Marchi wrote:
> On 16-04-12 02:45 PM, Pedro Alves wrote:
>> On 04/12/2016 07:36 PM, Simon Marchi wrote:
>>> On 16-04-12 02:08 PM, Pedro Alves wrote:
>>>> Don't we need to consider the case of $dest being an absolute path?
>>>
>>> I thought it wouldn't be better not, since we don't even want for tests to
>>> be able to write outside the standard output directory.  I guess that if a
>>> test tries to download to /foo/bar, it will end up as
>>> outputs/gdb.blah/thetest/foo/bar.  Then I would say the test would be at fault.
>>
>> OK, but I think we should put a comment to the effect in place.  After all,
>> we'll be pointing other people that run into this in their boards to this 
>> commit.  :-)
>>
>> Otherwise LGTM.  (Let's wait a bit to give Yao a chance to comment though.)
> 
> Good idea.  And if that's ok, I'd put the same comment in gdb_remote_download.
> 
> What do you think about this?

Fine with me.

Thanks,
Pedro Alves

Re: [PATCH] gdbserver-base.exp: Copy file to standard output directory in ${board}_download

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Otherwise LGTM.  (Let's wait a bit to give Yao a chance to comment though.)

Patch is fine by me as well.

-- 
Yao (齐尧)

Re: [PATCH] gdbserver-base.exp: Copy file to standard output directory in ${board}_download

[Simon Marchi]

On 16-04-13 07:33 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> Otherwise LGTM.  (Let's wait a bit to give Yao a chance to comment though.)
> 
> Patch is fine by me as well.
> 

Thanks, pushed.