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

Message ID 20180212195733.23639-3-sergiodj@redhat.com
State New
Headers show
Series
  • [v2,1/2] Create new common/pathstuff.[ch]
Related show

Commit Message

Sergio Durigan Junior Feb. 12, 2018, 7:57 p.m.
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

-- 
2.14.3

Comments

Simon Marchi Feb. 13, 2018, 4:35 a.m. | #1
On 2018-02-12 02:57 PM, Sergio Durigan Junior wrote:
> 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.


This last part is not valid anymore I believe.

> @@ -408,3 +409,34 @@ stringify_argv (const std::vector<char *> &args)

>  

>    return ret;

>  }

> +

> +/* See common/common-utils.h.  */

> +

> +bool

> +is_regular_file (const char *name, int *errno_ptr)

> +{

> +  struct stat st;

> +  const int status = stat (name, &st);

> +

> +  /* Stat should never fail except when the file does not exist.

> +     If stat fails, analyze the source of error and return True


Nit: True -> true

> +     unless the file does not exist, to avoid returning false results

> +     on obscure systems where stat does not work as expected.  */

> +

> +  if (status != 0)

> +    {

> +      if (errno != ENOENT)

> +	return true;

> +      *errno_ptr = ENOENT;

> +      return false;

> +    }

> +

> +  if (S_ISREG (st.st_mode))

> +    return true;

> +

> +  if (S_ISDIR (st.st_mode))

> +    *errno_ptr = EISDIR;

> +  else

> +    *errno_ptr = EINVAL;

> +  return false;

> +}

> diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h

> index 2320318de7..888396637e 100644

> --- a/gdb/common/common-utils.h

> +++ b/gdb/common/common-utils.h

> @@ -146,4 +146,9 @@ in_inclusive_range (T value, T low, T high)

>    return value >= low && value <= high;

>  }

>  

> +/* Return True if the file NAME exists and is a regular file.


Nit: True -> true

> +   If the result is false then *ERRNO_PTR is set to a useful value assuming

> +   we're expecting a regular file.  */

> +extern bool is_regular_file (const char *name, int *errno_ptr);

> +

>  #endif

> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c

> index f931273fa3..24b3e4d4ad 100644

> --- a/gdb/gdbserver/server.c

> +++ b/gdb/gdbserver/server.c

> @@ -39,6 +39,8 @@

>  #include "common-inferior.h"

>  #include "job-control.h"

>  #include "environ.h"

> +#include "filenames.h"

> +#include "pathstuff.h"

>  

>  #include "common/selftest.h"

>  

> @@ -283,6 +285,31 @@ get_environ ()

>    return &our_environ;

>  }

>  

> +/* Verify if PROGRAM_NAME is an absolute path, and perform path

> +   adjustment/expansion if not.  */

> +

> +static void

> +adjust_program_name_path ()

> +{

> +  /* Make sure we're using the absolute path of the inferior when

> +     creating it.  */

> +  if (!IS_ABSOLUTE_PATH (program_name))


I think we should modify the passed path only if really necessary.  If the path
is relative but contains a directory component, we don't need to change it.  I
think it's slightly better, because the argv[0] of the spawned process as well
as the gdbserver output will be true to what the user typed.  I also don't really
like the resulting path in:

$ ./gdbserver/gdbserver --once :1234 ../gdb/test
Process /home/simark/build/binutils-gdb/gdb/../gdb/test created; pid = 12321

So the check would be

  if (!contains_dir_separator (program_name))

where contains_dir_separator would be a new function.

> +    {

> +      int reg_file_errno;

> +

> +      /* Check if the file is in our CWD.  If it is, then we prefix

> +	 its name with CURRENT_DIRECTORY.  Otherwise, we leave the

> +	 name as-is because we'll try searching for it in $PATH.  */

> +      if (is_regular_file (program_name, &reg_file_errno))

> +	{

> +	  char *tmp_program_name = program_name;

> +

> +	  program_name = gdb_abspath (program_name).release ();

> +	  xfree (tmp_program_name);

> +	}

> +    }

> +}

> +

>  static int

>  attach_inferior (int pid)

>  {

> @@ -3016,6 +3043,8 @@ handle_v_run (char *own_buf)

>        program_name = new_program_name;

>      }

>  

> +  adjust_program_name_path ();

> +

>    /* Free the old argv and install the new one.  */

>    free_vector_argv (program_args);

>    program_args = new_argv;

> @@ -3770,6 +3799,8 @@ captured_main (int argc, char *argv[])

>  	program_args.push_back (xstrdup (next_arg[i]));

>        program_args.push_back (NULL);

>  

> +      adjust_program_name_path ();

> +

>        /* Wait till we are at first instruction in program.  */

>        create_inferior (program_name, program_args);

>  

> @@ -4290,6 +4321,8 @@ process_serial_event (void)

>  	  /* Wait till we are at 1st instruction in prog.  */

>  	  if (program_name != NULL)

>  	    {

> +	      adjust_program_name_path ();

> +


I thought I pointed it out in v1, but apparently I didn't.  I don't think we
need this call to adjust_program_name_path here.  We only need it at the
places that set program_name, which is not the case of this code.

Also, to avoid being able to set program_name and forget to call
adjust_program_name_path, I think it would be nice to have a small
class that holds the program path in a private field.  The only way
to change it would be through the setter, which would ensure the
adjustment is applied.

See the patch below for an example containing both of my suggestions.

Finally, I am wondering if maybe the error from getcwd should be fatal.
If you change:

-  current_directory = getcwd (NULL, 0);
+  current_directory = NULL;

to see what would happen if getcwd failed, and launch gdbserver with a
filename-only path, you get a segfault:

$ ./gdbserver/gdbserver --once :1234 test
gdbserver: Success: error finding working directory
[1]    21945 segmentation fault (core dumped)  ./gdbserver/gdbserver --once :1234 test

That's because a NULL current_directory is passed to strlen in gdb_abspath.
I think it's rare enough and critical enough situation that we can just
exit with an error if getcwd fails (I didn't include this in the patch
below because I thought about it after pasting the patch :)).

Thanks,

Simon


From 00a3fc9e340cf6fae1fcb926d39d43594df16643 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>

Date: Mon, 12 Feb 2018 23:02:11 -0500
Subject: [PATCH] Suggestion

---
 gdb/common/pathstuff.c | 12 ++++++++
 gdb/common/pathstuff.h |  4 +++
 gdb/gdbserver/server.c | 79 ++++++++++++++++++++++----------------------------
 3 files changed, 51 insertions(+), 44 deletions(-)

diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
index fc51edffa9..59e2f7a004 100644
--- a/gdb/common/pathstuff.c
+++ b/gdb/common/pathstuff.c
@@ -138,3 +138,15 @@ gdb_abspath (const char *path)
 	     ? "" : SLASH_STRING,
 	     path, (char *) NULL));
 }
+
+bool
+contains_dir_separator (const char *path)
+{
+  for (; *path != '\0'; path++)
+    {
+      if (IS_DIR_SEPARATOR (*path))
+	return true;
+    }
+
+  return false;
+}
diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h
index 909fd786bb..e0a7fd5f50 100644
--- a/gdb/common/pathstuff.h
+++ b/gdb/common/pathstuff.h
@@ -36,4 +36,8 @@ extern gdb::unique_xmalloc_ptr<char>

 extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);

+/* Return whether PATH contains a directory separator character.  */
+
+extern bool contains_dir_separator (const char *path);
+
 #endif /* PATHSTUFF_H */
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 24b3e4d4ad..cbfd2d8c99 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -114,7 +114,32 @@ static int vCont_supported;
    space randomization feature before starting an inferior.  */
 int disable_randomization = 1;

-static char *program_name = NULL;
+static struct {
+  void set (gdb::unique_xmalloc_ptr<char> &&path)
+  {
+    m_path = std::move (path);
+
+    /* Make sure we're using the absolute path of the inferior when
+       creating it.  */
+    if (!contains_dir_separator (m_path.get ()))
+      {
+        int reg_file_errno;
+
+        /* Check if the file is in our CWD.  If it is, then we prefix
+	 its name with CURRENT_DIRECTORY.  Otherwise, we leave the
+	 name as-is because we'll try searching for it in $PATH.  */
+        if (is_regular_file (m_path.get (), &reg_file_errno))
+          m_path = gdb_abspath (m_path.get ());
+      }
+  }
+
+  char *get ()
+  { return m_path.get (); }
+
+private:
+  gdb::unique_xmalloc_ptr<char> m_path;
+} program_path;
+
 static std::vector<char *> program_args;
 static std::string wrapper_argv;

@@ -271,10 +296,10 @@ get_exec_wrapper ()
 char *
 get_exec_file (int err)
 {
-  if (err && program_name == NULL)
+  if (err && program_path.get () == NULL)
     error (_("No executable file specified."));

-  return program_name;
+  return program_path.get ();
 }

 /* See server.h.  */
@@ -285,31 +310,6 @@ get_environ ()
   return &our_environ;
 }

-/* Verify if PROGRAM_NAME is an absolute path, and perform path
-   adjustment/expansion if not.  */
-
-static void
-adjust_program_name_path ()
-{
-  /* Make sure we're using the absolute path of the inferior when
-     creating it.  */
-  if (!IS_ABSOLUTE_PATH (program_name))
-    {
-      int reg_file_errno;
-
-      /* Check if the file is in our CWD.  If it is, then we prefix
-	 its name with CURRENT_DIRECTORY.  Otherwise, we leave the
-	 name as-is because we'll try searching for it in $PATH.  */
-      if (is_regular_file (program_name, &reg_file_errno))
-	{
-	  char *tmp_program_name = program_name;
-
-	  program_name = gdb_abspath (program_name).release ();
-	  xfree (tmp_program_name);
-	}
-    }
-}
-
 static int
 attach_inferior (int pid)
 {
@@ -3030,7 +3030,7 @@ handle_v_run (char *own_buf)
     {
       /* GDB didn't specify a program to run.  Use the program from the
 	 last run with the new argument list.  */
-      if (program_name == NULL)
+      if (program_path.get () == NULL)
 	{
 	  write_enn (own_buf);
 	  free_vector_argv (new_argv);
@@ -3038,18 +3038,13 @@ handle_v_run (char *own_buf)
 	}
     }
   else
-    {
-      xfree (program_name);
-      program_name = new_program_name;
-    }
-
-  adjust_program_name_path ();
+    program_path.set (gdb::unique_xmalloc_ptr<char> (new_program_name));

   /* Free the old argv and install the new one.  */
   free_vector_argv (program_args);
   program_args = new_argv;

-  create_inferior (program_name, program_args);
+  create_inferior (program_path.get (), program_args);

   if (last_status.kind == TARGET_WAITKIND_STOPPED)
     {
@@ -3794,15 +3789,13 @@ captured_main (int argc, char *argv[])
       int i, n;

       n = argc - (next_arg - argv);
-      program_name = xstrdup (next_arg[0]);
+      program_path.set (gdb::unique_xmalloc_ptr<char> (xstrdup (next_arg[0])));
       for (i = 1; i < n; i++)
 	program_args.push_back (xstrdup (next_arg[i]));
       program_args.push_back (NULL);

-      adjust_program_name_path ();
-
       /* Wait till we are at first instruction in program.  */
-      create_inferior (program_name, program_args);
+      create_inferior (program_path.get (), program_args);

       /* We are now (hopefully) stopped at the first instruction of
 	 the target process.  This assumes that the target process was
@@ -4319,11 +4312,9 @@ process_serial_event (void)
 	  fprintf (stderr, "GDBserver restarting\n");

 	  /* Wait till we are at 1st instruction in prog.  */
-	  if (program_name != NULL)
+	  if (program_path.get () != NULL)
 	    {
-	      adjust_program_name_path ();
-
-	      create_inferior (program_name, program_args);
+	      create_inferior (program_path.get (), program_args);

 	      if (last_status.kind == TARGET_WAITKIND_STOPPED)
 		{
-- 
2.16.1
Pedro Alves Feb. 21, 2018, 12:29 p.m. | #2
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
Sergio Durigan Junior Feb. 22, 2018, 6:37 p.m. | #3
On Monday, February 12 2018, Simon Marchi wrote:

> On 2018-02-12 02:57 PM, Sergio Durigan Junior wrote:

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

>

> This last part is not valid anymore I believe.


Yes, I'll remove it.

>> @@ -408,3 +409,34 @@ stringify_argv (const std::vector<char *> &args)

>>  

>>    return ret;

>>  }

>> +

>> +/* See common/common-utils.h.  */

>> +

>> +bool

>> +is_regular_file (const char *name, int *errno_ptr)

>> +{

>> +  struct stat st;

>> +  const int status = stat (name, &st);

>> +

>> +  /* Stat should never fail except when the file does not exist.

>> +     If stat fails, analyze the source of error and return True

>

> Nit: True -> true


Fixed.

>> +     unless the file does not exist, to avoid returning false results

>> +     on obscure systems where stat does not work as expected.  */

>> +

>> +  if (status != 0)

>> +    {

>> +      if (errno != ENOENT)

>> +	return true;

>> +      *errno_ptr = ENOENT;

>> +      return false;

>> +    }

>> +

>> +  if (S_ISREG (st.st_mode))

>> +    return true;

>> +

>> +  if (S_ISDIR (st.st_mode))

>> +    *errno_ptr = EISDIR;

>> +  else

>> +    *errno_ptr = EINVAL;

>> +  return false;

>> +}

>> diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h

>> index 2320318de7..888396637e 100644

>> --- a/gdb/common/common-utils.h

>> +++ b/gdb/common/common-utils.h

>> @@ -146,4 +146,9 @@ in_inclusive_range (T value, T low, T high)

>>    return value >= low && value <= high;

>>  }

>>  

>> +/* Return True if the file NAME exists and is a regular file.

>

> Nit: True -> true


Fixed.

>> +   If the result is false then *ERRNO_PTR is set to a useful value assuming

>> +   we're expecting a regular file.  */

>> +extern bool is_regular_file (const char *name, int *errno_ptr);

>> +

>>  #endif

>> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c

>> index f931273fa3..24b3e4d4ad 100644

>> --- a/gdb/gdbserver/server.c

>> +++ b/gdb/gdbserver/server.c

>> @@ -39,6 +39,8 @@

>>  #include "common-inferior.h"

>>  #include "job-control.h"

>>  #include "environ.h"

>> +#include "filenames.h"

>> +#include "pathstuff.h"

>>  

>>  #include "common/selftest.h"

>>  

>> @@ -283,6 +285,31 @@ get_environ ()

>>    return &our_environ;

>>  }

>>  

>> +/* Verify if PROGRAM_NAME is an absolute path, and perform path

>> +   adjustment/expansion if not.  */

>> +

>> +static void

>> +adjust_program_name_path ()

>> +{

>> +  /* Make sure we're using the absolute path of the inferior when

>> +     creating it.  */

>> +  if (!IS_ABSOLUTE_PATH (program_name))

>

> I think we should modify the passed path only if really necessary.  If the path

> is relative but contains a directory component, we don't need to change it.  I

> think it's slightly better, because the argv[0] of the spawned process as well

> as the gdbserver output will be true to what the user typed.  I also don't really

> like the resulting path in:

>

> $ ./gdbserver/gdbserver --once :1234 ../gdb/test

> Process /home/simark/build/binutils-gdb/gdb/../gdb/test created; pid = 12321

>

> So the check would be

>

>   if (!contains_dir_separator (program_name))

>

> where contains_dir_separator would be a new function.


OK.

>> +    {

>> +      int reg_file_errno;

>> +

>> +      /* Check if the file is in our CWD.  If it is, then we prefix

>> +	 its name with CURRENT_DIRECTORY.  Otherwise, we leave the

>> +	 name as-is because we'll try searching for it in $PATH.  */

>> +      if (is_regular_file (program_name, &reg_file_errno))

>> +	{

>> +	  char *tmp_program_name = program_name;

>> +

>> +	  program_name = gdb_abspath (program_name).release ();

>> +	  xfree (tmp_program_name);

>> +	}

>> +    }

>> +}

>> +

>>  static int

>>  attach_inferior (int pid)

>>  {

>> @@ -3016,6 +3043,8 @@ handle_v_run (char *own_buf)

>>        program_name = new_program_name;

>>      }

>>  

>> +  adjust_program_name_path ();

>> +

>>    /* Free the old argv and install the new one.  */

>>    free_vector_argv (program_args);

>>    program_args = new_argv;

>> @@ -3770,6 +3799,8 @@ captured_main (int argc, char *argv[])

>>  	program_args.push_back (xstrdup (next_arg[i]));

>>        program_args.push_back (NULL);

>>  

>> +      adjust_program_name_path ();

>> +

>>        /* Wait till we are at first instruction in program.  */

>>        create_inferior (program_name, program_args);

>>  

>> @@ -4290,6 +4321,8 @@ process_serial_event (void)

>>  	  /* Wait till we are at 1st instruction in prog.  */

>>  	  if (program_name != NULL)

>>  	    {

>> +	      adjust_program_name_path ();

>> +

>

> I thought I pointed it out in v1, but apparently I didn't.  I don't think we

> need this call to adjust_program_name_path here.  We only need it at the

> places that set program_name, which is not the case of this code.

>

> Also, to avoid being able to set program_name and forget to call

> adjust_program_name_path, I think it would be nice to have a small

> class that holds the program path in a private field.  The only way

> to change it would be through the setter, which would ensure the

> adjustment is applied.

>

> See the patch below for an example containing both of my suggestions.


I incorporated your patch, thanks.

> Finally, I am wondering if maybe the error from getcwd should be fatal.

> If you change:

>

> -  current_directory = getcwd (NULL, 0);

> +  current_directory = NULL;

>

> to see what would happen if getcwd failed, and launch gdbserver with a

> filename-only path, you get a segfault:

>

> $ ./gdbserver/gdbserver --once :1234 test

> gdbserver: Success: error finding working directory

> [1]    21945 segmentation fault (core dumped)  ./gdbserver/gdbserver --once :1234 test

>

> That's because a NULL current_directory is passed to strlen in gdb_abspath.

> I think it's rare enough and critical enough situation that we can just

> exit with an error if getcwd fails (I didn't include this in the patch

> below because I thought about it after pasting the patch :)).


Maybe the error should be fatal.  I decided to mimick what GDB already
does (call perror_warning_with_name, which issues a warning, and
continue).  I will modify this code to call error instead on gdbserver.

> Thanks,

>

> Simon

>

>

> From 00a3fc9e340cf6fae1fcb926d39d43594df16643 Mon Sep 17 00:00:00 2001

> From: Simon Marchi <simon.marchi@polymtl.ca>

> Date: Mon, 12 Feb 2018 23:02:11 -0500

> Subject: [PATCH] Suggestion

>

> ---

>  gdb/common/pathstuff.c | 12 ++++++++

>  gdb/common/pathstuff.h |  4 +++

>  gdb/gdbserver/server.c | 79 ++++++++++++++++++++++----------------------------

>  3 files changed, 51 insertions(+), 44 deletions(-)

>

> diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c

> index fc51edffa9..59e2f7a004 100644

> --- a/gdb/common/pathstuff.c

> +++ b/gdb/common/pathstuff.c

> @@ -138,3 +138,15 @@ gdb_abspath (const char *path)

>  	     ? "" : SLASH_STRING,

>  	     path, (char *) NULL));

>  }

> +

> +bool

> +contains_dir_separator (const char *path)

> +{

> +  for (; *path != '\0'; path++)

> +    {

> +      if (IS_DIR_SEPARATOR (*path))

> +	return true;

> +    }

> +

> +  return false;

> +}

> diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h

> index 909fd786bb..e0a7fd5f50 100644

> --- a/gdb/common/pathstuff.h

> +++ b/gdb/common/pathstuff.h

> @@ -36,4 +36,8 @@ extern gdb::unique_xmalloc_ptr<char>

>

>  extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);

>

> +/* Return whether PATH contains a directory separator character.  */

> +

> +extern bool contains_dir_separator (const char *path);

> +

>  #endif /* PATHSTUFF_H */

> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c

> index 24b3e4d4ad..cbfd2d8c99 100644

> --- a/gdb/gdbserver/server.c

> +++ b/gdb/gdbserver/server.c

> @@ -114,7 +114,32 @@ static int vCont_supported;

>     space randomization feature before starting an inferior.  */

>  int disable_randomization = 1;

>

> -static char *program_name = NULL;

> +static struct {

> +  void set (gdb::unique_xmalloc_ptr<char> &&path)

> +  {

> +    m_path = std::move (path);

> +

> +    /* Make sure we're using the absolute path of the inferior when

> +       creating it.  */

> +    if (!contains_dir_separator (m_path.get ()))

> +      {

> +        int reg_file_errno;

> +

> +        /* Check if the file is in our CWD.  If it is, then we prefix

> +	 its name with CURRENT_DIRECTORY.  Otherwise, we leave the

> +	 name as-is because we'll try searching for it in $PATH.  */

> +        if (is_regular_file (m_path.get (), &reg_file_errno))

> +          m_path = gdb_abspath (m_path.get ());

> +      }

> +  }

> +

> +  char *get ()

> +  { return m_path.get (); }

> +

> +private:

> +  gdb::unique_xmalloc_ptr<char> m_path;

> +} program_path;

> +

>  static std::vector<char *> program_args;

>  static std::string wrapper_argv;

>

> @@ -271,10 +296,10 @@ get_exec_wrapper ()

>  char *

>  get_exec_file (int err)

>  {

> -  if (err && program_name == NULL)

> +  if (err && program_path.get () == NULL)

>      error (_("No executable file specified."));

>

> -  return program_name;

> +  return program_path.get ();

>  }

>

>  /* See server.h.  */

> @@ -285,31 +310,6 @@ get_environ ()

>    return &our_environ;

>  }

>

> -/* Verify if PROGRAM_NAME is an absolute path, and perform path

> -   adjustment/expansion if not.  */

> -

> -static void

> -adjust_program_name_path ()

> -{

> -  /* Make sure we're using the absolute path of the inferior when

> -     creating it.  */

> -  if (!IS_ABSOLUTE_PATH (program_name))

> -    {

> -      int reg_file_errno;

> -

> -      /* Check if the file is in our CWD.  If it is, then we prefix

> -	 its name with CURRENT_DIRECTORY.  Otherwise, we leave the

> -	 name as-is because we'll try searching for it in $PATH.  */

> -      if (is_regular_file (program_name, &reg_file_errno))

> -	{

> -	  char *tmp_program_name = program_name;

> -

> -	  program_name = gdb_abspath (program_name).release ();

> -	  xfree (tmp_program_name);

> -	}

> -    }

> -}

> -

>  static int

>  attach_inferior (int pid)

>  {

> @@ -3030,7 +3030,7 @@ handle_v_run (char *own_buf)

>      {

>        /* GDB didn't specify a program to run.  Use the program from the

>  	 last run with the new argument list.  */

> -      if (program_name == NULL)

> +      if (program_path.get () == NULL)

>  	{

>  	  write_enn (own_buf);

>  	  free_vector_argv (new_argv);

> @@ -3038,18 +3038,13 @@ handle_v_run (char *own_buf)

>  	}

>      }

>    else

> -    {

> -      xfree (program_name);

> -      program_name = new_program_name;

> -    }

> -

> -  adjust_program_name_path ();

> +    program_path.set (gdb::unique_xmalloc_ptr<char> (new_program_name));

>

>    /* Free the old argv and install the new one.  */

>    free_vector_argv (program_args);

>    program_args = new_argv;

>

> -  create_inferior (program_name, program_args);

> +  create_inferior (program_path.get (), program_args);

>

>    if (last_status.kind == TARGET_WAITKIND_STOPPED)

>      {

> @@ -3794,15 +3789,13 @@ captured_main (int argc, char *argv[])

>        int i, n;

>

>        n = argc - (next_arg - argv);

> -      program_name = xstrdup (next_arg[0]);

> +      program_path.set (gdb::unique_xmalloc_ptr<char> (xstrdup (next_arg[0])));

>        for (i = 1; i < n; i++)

>  	program_args.push_back (xstrdup (next_arg[i]));

>        program_args.push_back (NULL);

>

> -      adjust_program_name_path ();

> -

>        /* Wait till we are at first instruction in program.  */

> -      create_inferior (program_name, program_args);

> +      create_inferior (program_path.get (), program_args);

>

>        /* We are now (hopefully) stopped at the first instruction of

>  	 the target process.  This assumes that the target process was

> @@ -4319,11 +4312,9 @@ process_serial_event (void)

>  	  fprintf (stderr, "GDBserver restarting\n");

>

>  	  /* Wait till we are at 1st instruction in prog.  */

> -	  if (program_name != NULL)

> +	  if (program_path.get () != NULL)

>  	    {

> -	      adjust_program_name_path ();

> -

> -	      create_inferior (program_name, program_args);

> +	      create_inferior (program_path.get (), program_args);

>

>  	      if (last_status.kind == TARGET_WAITKIND_STOPPED)

>  		{

> -- 

> 2.16.1


-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Sergio Durigan Junior Feb. 27, 2018, 12:20 a.m. | #4
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,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/
Sergio Durigan Junior Feb. 28, 2018, 3:32 a.m. | #5
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,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Patch

diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index ae2dd9db2b..aa403d8088 100644
--- a/gdb/common/common-utils.c
+++ b/gdb/common/common-utils.c
@@ -20,6 +20,7 @@ 
 #include "common-defs.h"
 #include "common-utils.h"
 #include "host-defs.h"
+#include <sys/stat.h>
 #include <ctype.h>
 
 /* The xmalloc() (libiberty.h) family of memory management routines.
@@ -408,3 +409,34 @@  stringify_argv (const std::vector<char *> &args)
 
   return ret;
 }
+
+/* See common/common-utils.h.  */
+
+bool
+is_regular_file (const char *name, int *errno_ptr)
+{
+  struct stat st;
+  const int status = stat (name, &st);
+
+  /* Stat should never fail except when the file does not exist.
+     If stat fails, analyze the source of error and return True
+     unless the file does not exist, to avoid returning false results
+     on obscure systems where stat does not work as expected.  */
+
+  if (status != 0)
+    {
+      if (errno != ENOENT)
+	return true;
+      *errno_ptr = ENOENT;
+      return false;
+    }
+
+  if (S_ISREG (st.st_mode))
+    return true;
+
+  if (S_ISDIR (st.st_mode))
+    *errno_ptr = EISDIR;
+  else
+    *errno_ptr = EINVAL;
+  return false;
+}
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 2320318de7..888396637e 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -146,4 +146,9 @@  in_inclusive_range (T value, T low, T high)
   return value >= low && value <= high;
 }
 
+/* Return True if the file NAME exists and is a regular file.
+   If the result is false then *ERRNO_PTR is set to a useful value assuming
+   we're expecting a regular file.  */
+extern bool is_regular_file (const char *name, int *errno_ptr);
+
 #endif
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index f931273fa3..24b3e4d4ad 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -39,6 +39,8 @@ 
 #include "common-inferior.h"
 #include "job-control.h"
 #include "environ.h"
+#include "filenames.h"
+#include "pathstuff.h"
 
 #include "common/selftest.h"
 
@@ -283,6 +285,31 @@  get_environ ()
   return &our_environ;
 }
 
+/* Verify if PROGRAM_NAME is an absolute path, and perform path
+   adjustment/expansion if not.  */
+
+static void
+adjust_program_name_path ()
+{
+  /* Make sure we're using the absolute path of the inferior when
+     creating it.  */
+  if (!IS_ABSOLUTE_PATH (program_name))
+    {
+      int reg_file_errno;
+
+      /* Check if the file is in our CWD.  If it is, then we prefix
+	 its name with CURRENT_DIRECTORY.  Otherwise, we leave the
+	 name as-is because we'll try searching for it in $PATH.  */
+      if (is_regular_file (program_name, &reg_file_errno))
+	{
+	  char *tmp_program_name = program_name;
+
+	  program_name = gdb_abspath (program_name).release ();
+	  xfree (tmp_program_name);
+	}
+    }
+}
+
 static int
 attach_inferior (int pid)
 {
@@ -3016,6 +3043,8 @@  handle_v_run (char *own_buf)
       program_name = new_program_name;
     }
 
+  adjust_program_name_path ();
+
   /* Free the old argv and install the new one.  */
   free_vector_argv (program_args);
   program_args = new_argv;
@@ -3770,6 +3799,8 @@  captured_main (int argc, char *argv[])
 	program_args.push_back (xstrdup (next_arg[i]));
       program_args.push_back (NULL);
 
+      adjust_program_name_path ();
+
       /* Wait till we are at first instruction in program.  */
       create_inferior (program_name, program_args);
 
@@ -4290,6 +4321,8 @@  process_serial_event (void)
 	  /* Wait till we are at 1st instruction in prog.  */
 	  if (program_name != NULL)
 	    {
+	      adjust_program_name_path ();
+
 	      create_inferior (program_name, program_args);
 
 	      if (last_status.kind == TARGET_WAITKIND_STOPPED)
diff --git a/gdb/source.c b/gdb/source.c
index 77f5e8d4d4..bfa9fd6e4c 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -29,7 +29,6 @@ 
 #include "filestuff.h"
 
 #include <sys/types.h>
-#include <sys/stat.h>
 #include <fcntl.h>
 #include "gdbcore.h"
 #include "gdb_regex.h"
@@ -670,39 +669,6 @@  info_source_command (const char *ignore, int from_tty)
 }
 
 
-/* Return True if the file NAME exists and is a regular file.
-   If the result is false then *ERRNO_PTR is set to a useful value assuming
-   we're expecting a regular file.  */
-
-static int
-is_regular_file (const char *name, int *errno_ptr)
-{
-  struct stat st;
-  const int status = stat (name, &st);
-
-  /* Stat should never fail except when the file does not exist.
-     If stat fails, analyze the source of error and return True
-     unless the file does not exist, to avoid returning false results
-     on obscure systems where stat does not work as expected.  */
-
-  if (status != 0)
-    {
-      if (errno != ENOENT)
-	return 1;
-      *errno_ptr = ENOENT;
-      return 0;
-    }
-
-  if (S_ISREG (st.st_mode))
-    return 1;
-
-  if (S_ISDIR (st.st_mode))
-    *errno_ptr = EISDIR;
-  else
-    *errno_ptr = EINVAL;
-  return 0;
-}
-
 /* Open a file named STRING, searching path PATH (dir names sep by some char)
    using mode MODE in the calls to open.  You cannot use this function to
    create files (O_CREAT).
diff --git a/gdb/testsuite/gdb.server/abspath.exp b/gdb/testsuite/gdb.server/abspath.exp
new file mode 100644
index 0000000000..fbde5ee537
--- /dev/null
+++ b/gdb/testsuite/gdb.server/abspath.exp
@@ -0,0 +1,54 @@ 
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test that gdbserver performs path expansion/adjustment when we
+# provide just a filename (without any path specifications) to it.
+
+load_lib gdbserver-support.exp
+
+standard_testfile normal.c
+
+if { [skip_gdbserver_tests] } {
+    return 0
+}
+
+# We only test things locally, and on native-gdbserver
+if { [is_remote target] || [is_remote host] || ![use_gdb_stub] } {
+    return 0
+}
+
+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]
+
+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"
+}
+
+file delete -force "[pwd]/$target_execname"