[v2,2/2] Make gdbserver work with filename-only binaries

  Changes from v1:

- Moved "is_regular_file" from "source.c" to "common/common-utils.c".

- Made "adjust_program_name_path" use "is_regular_file" in order to
  check if there is a file named PROGRAM_NAME in CWD, and prefix it
  with CURRENT_DIRECTORY if it exists.  Otherwise, don't prefix it and
  let gdbserver try to find the binary in $PATH.


Simon mentioned on IRC that, after the startup-with-shell feature has
been implemented on gdbserver, it is not possible to specify a
filename-only binary, like:

  $ gdbserver :1234 a.out
  /bin/bash: line 0: exec: a.out: not found
  During startup program exited with code 127.
  Exiting

This happens on systems where the current directory "." is not listed
in the PATH environment variable.  Although include "." in the PATH
variable is a possible workaround, this can be considered a regression
because before startup-with-shell it was possible to use only the
filename (due to reason that gdbserver used "exec*" directly).

The idea of the patch is to perform a call to "gdb_abspath" and adjust
the PROGRAM_NAME variable before the call to "create_inferior".  This
adjustment will consist of tilde-expansion or prefixing PROGRAM_NAME
using the CURRENT_DIRECTORY (a variable that was specific to GDB, but
has been put into common/common-defs.h and now is set/used by
gdbserver as well), thus transforming PROGRAM_NAME in an absolute
path.

This mimicks the behaviour seen on GDB (look at "openp" and
"attach_inferior", for example).  Now, we'll always execute the binary
using its full path on gdbserver.

I am also submitting a testcase which exercises the scenario described
above.  Because the test requires copying (and deleting) files
locally, I decided to restrict its execution to non-remote
targets/hosts.  I've also had to do a minor adjustment on
gdb.server/non-existing-program.exp's regexp in order to match the
correct error message.

Built and regtested on BuildBot, without regressions.

gdb/gdbserver/ChangeLog:
2018-02-12  Sergio Durigan Junior  <sergiodj@redhat.com>

	* common/common-utils.c: Include "sys/stat.h".
	(is_regular_file): Move here from "source.c"; change return
	type to "bool".
	* common/common-utils.h (is_regular_file): New prototype.
	* source.c: Don't include "sys/stat.h".
	(is_regular_file): Move to "common/common-utils.c".

gdb/gdbserver/ChangeLog:
2018-02-12  Sergio Durigan Junior  <sergiodj@redhat.com>

	* server.c: Include "filenames.h" and "pathstuff.h".
	(adjust_program_name_path): New function.
	(attach_inferior): Call "adjust_program_name_path" before
	"create_inferior".
	(captured_main): Likewise.
	(process_serial_event): Likewise.

gdb/testsuite/ChangeLog:
2018-02-12  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.server/abspath.exp: New file.
---
 gdb/common/common-utils.c            | 32 +++++++++++++++++++++
 gdb/common/common-utils.h            |  5 ++++
 gdb/gdbserver/server.c               | 33 ++++++++++++++++++++++
 gdb/source.c                         | 34 -----------------------
 gdb/testsuite/gdb.server/abspath.exp | 54 ++++++++++++++++++++++++++++++++++++
 5 files changed, 124 insertions(+), 34 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/abspath.exp
  

Hi Sergio,

A few quick comments below.  

> @@ -3539,6 +3564,13 @@ captured_main (int argc, char *argv[])
>    const char *selftest_filter = NULL;
>  #endif
>  
> +  current_directory = getcwd (NULL, 0);
> +  if (current_directory == NULL)
> +    {
> +      warning (_("%s: error finding working directory"),
> +	       safe_strerror (errno));

I think it's much more usual to put the strerror string at the
end of the warning rather than at the beginning.

I.e., something like:

   warning (_("Could not find working directory: %s"),
       safe_strerror (errno));

See

  $ (cd gdb; git grep -3 "warning (" *.c | grep strerr -C 3)

for example.

From the ongoing discussion, it sounded like this hunk may change
in the next iteration, but I thought I'd still comment as it
may help with future patches.


> +    }


> +# We only test things locally, and on native-gdbserver
> +if { [is_remote target] || [is_remote host] || ![use_gdb_stub] } {
> +    return 0
> +}


I don't see why to restrict this to "only on native-gdbserver".  The test
is calling gdbserver_start etc. manually, so it should work when testing
with any local board, I think?  I.e., when testing with native or
extended-remote too.  For the latter, tests will usually call "disconnect".

> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
> +    return -1
> +}
> +
> +set target_exec [gdbserver_download_current_prog]
> +set target_execname [file tail $target_exec]
> +# We temporarily copy the file to our current directory
> +file copy -force $target_exec [pwd]
> +set res [gdbserver_start "" $target_execname]

Please remind me -- is the current directory here usually
the testcase's output dir?  I.e., is it guaranteed that
the current directory here is not going to be the same
directory of another testcase running in parallel at
the same time?

> +
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
> +
> +if { [runto_main] } {
> +    pass "load filename without absolute path"
> +} else {
> +    fail "load filename without absolute path"
> +}

runto_main here looks a bit odd to me.  You're manually connecting
with gdb_target_cmd, bypassing whatever the current board file
may want to do, but then you're using a routine that call back
into the board file to do random things to prepare for running.

I think you should set a breakpoint at main and continue to
it without using runto_main.  Note how no other test in gdb.server/
uses runto_main.

Thanks,
Pedro Alves
  

On Wednesday, February 21 2018, Pedro Alves wrote:

> Hi Sergio,
>
> A few quick comments below.  

Thanks, Pedro, and sorry for the delay in replying.

>> @@ -3539,6 +3564,13 @@ captured_main (int argc, char *argv[])
>>    const char *selftest_filter = NULL;
>>  #endif
>>  
>> +  current_directory = getcwd (NULL, 0);
>> +  if (current_directory == NULL)
>> +    {
>> +      warning (_("%s: error finding working directory"),
>> +	       safe_strerror (errno));
>
> I think it's much more usual to put the strerror string at the
> end of the warning rather than at the beginning.
>
> I.e., something like:
>
>    warning (_("Could not find working directory: %s"),
>        safe_strerror (errno));
>
> See
>
>   $ (cd gdb; git grep -3 "warning (" *.c | grep strerr -C 3)
>
> for example.
>
> From the ongoing discussion, it sounded like this hunk may change
> in the next iteration, but I thought I'd still comment as it
> may help with future patches.

The only change is s/warning/error/, but I can also change the message.

>> +# We only test things locally, and on native-gdbserver
>> +if { [is_remote target] || [is_remote host] || ![use_gdb_stub] } {
>> +    return 0
>> +}
>
>
> I don't see why to restrict this to "only on native-gdbserver".  The test
> is calling gdbserver_start etc. manually, so it should work when testing
> with any local board, I think?  I.e., when testing with native or
> extended-remote too.  For the latter, tests will usually call "disconnect".

As far as I have tested (and remember), extended-remote doesn't actually
start gdbserver by using the binary.  Instead, it starts gdbserver
without a binary and relies on 'set remote exec-file'.

.... (fiddles with testcase) ....

Right, I managed to remove this restriction and now the tests runs and
passes on other target boards as well.

>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
>> +    return -1
>> +}
>> +
>> +set target_exec [gdbserver_download_current_prog]
>> +set target_execname [file tail $target_exec]
>> +# We temporarily copy the file to our current directory
>> +file copy -force $target_exec [pwd]
>> +set res [gdbserver_start "" $target_execname]
>
> Please remind me -- is the current directory here usually
> the testcase's output dir?  I.e., is it guaranteed that
> the current directory here is not going to be the same
> directory of another testcase running in parallel at
> the same time?

No, [pwd] is actually the gdb/testsuite/ directory, from where the
Makefile runs.  Which means that other tests running in parallel at the
same time will have the same value for [pwd].  I copied the file to
[pwd] because that's how I solved the problem of having the binary at
the same directory as the one I'm starting gdbserver from.

Another solution that I thought was to cd into the the dirname of
the downloaded $target_exec, execute gdbserver from there, and the cd
back to the original directory.  WDYT?

>> +
>> +set gdbserver_protocol [lindex $res 0]
>> +set gdbserver_gdbport [lindex $res 1]
>> +gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
>> +
>> +if { [runto_main] } {
>> +    pass "load filename without absolute path"
>> +} else {
>> +    fail "load filename without absolute path"
>> +}
>
> runto_main here looks a bit odd to me.  You're manually connecting
> with gdb_target_cmd, bypassing whatever the current board file
> may want to do, but then you're using a routine that call back
> into the board file to do random things to prepare for running.
>
> I think you should set a breakpoint at main and continue to
> it without using runto_main.  Note how no other test in gdb.server/
> uses runto_main.

Ah, OK.  I'm usually confused about our testsuite when it comes to
remote vs. remote-extended, so thanks for the advice.

Thanks,
  

On Monday, February 26 2018, I wrote:

> On Wednesday, February 21 2018, Pedro Alves wrote:
>>> +
>>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
>>> +    return -1
>>> +}
>>> +
>>> +set target_exec [gdbserver_download_current_prog]
>>> +set target_execname [file tail $target_exec]
>>> +# We temporarily copy the file to our current directory
>>> +file copy -force $target_exec [pwd]
>>> +set res [gdbserver_start "" $target_execname]
>>
>> Please remind me -- is the current directory here usually
>> the testcase's output dir?  I.e., is it guaranteed that
>> the current directory here is not going to be the same
>> directory of another testcase running in parallel at
>> the same time?
>
> No, [pwd] is actually the gdb/testsuite/ directory, from where the
> Makefile runs.  Which means that other tests running in parallel at the
> same time will have the same value for [pwd].  I copied the file to
> [pwd] because that's how I solved the problem of having the binary at
> the same directory as the one I'm starting gdbserver from.
>
> Another solution that I thought was to cd into the the dirname of
> the downloaded $target_exec, execute gdbserver from there, and the cd
> back to the original directory.  WDYT?

I decided to go ahead and implement this idea.  v3 is now out.  Please
let me know your thoughts.

Thanks,