[PATCH 1/2] Create new common/pathstuff.[ch]

[PATCH 1/2] Create new common/pathstuff.[ch]

[Sergio Durigan Junior]

This commit moves the path manipulation routines found on utils.c to a
new common/pathstuff.c, and updates the Makefile.in's accordingly.
The routines moved are "gdb_realpath", "gdb_realpath_keepfile" and
"gdb_abspath".

This will be needed because gdbserver will have to call "gdb_abspath"
on my next patch, which implements a way to expand the path of the
inferior provided by the user in order to allow specifying just the
binary name when starting gdbserver, like:

  $ gdbserver :1234 a.out

With the recent addition of the startup-with-shell feature on
gdbserver, this scenario doesn't work anymore if the user doesn't have
the current directory listed in the PATH variable.

I had to do a minor adjustment on "gdb_abspath" because we don't have
access to "tilde_expand" on gdbserver, so now the function is using
"gdb_tilde_expand" instead.  Otherwise, the code is the same.

Regression tested on the BuildBot, without regressions.

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

	* Makefile.in (SFILES): Add "common/pathstuff.c".
	(HFILES_NO_SRCDIR): Add "common/pathstuff.h".
	(COMMON_OBS): Add "pathstuff.o".
	* common/pathstuff.c: New file.
	* common/pathstuff.h: New file.
	* utils.c (gdb_realpath): Move to "common/pathstuff.c".
	(gdb_realpath_keepfile): Likewise.
	(gdb_abspath): Likewise.
	* utils.h: Include "common/pathstuff.h".
	(gdb_realpath): Move to "common/pathstuff.h".
	(gdb_realpath_keepfile): Likewise.
	(gdb_abspath): Likewise.

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

	* Makefile.in (SFILES): Add "$(srcdir)/common/pathstuff.c".
	(OBJS): Add "pathstuff.o".
---
 gdb/Makefile.in           |   3 +
 gdb/common/pathstuff.c    | 144 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/common/pathstuff.h    |  39 +++++++++++++
 gdb/gdbserver/Makefile.in |   2 +
 gdb/utils.c               | 119 --------------------------------------
 gdb/utils.h               |   7 +--
 6 files changed, 189 insertions(+), 125 deletions(-)
 create mode 100644 gdb/common/pathstuff.c
 create mode 100644 gdb/common/pathstuff.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 957654c9bd..64c9df3eaf 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1136,6 +1136,7 @@ SFILES = \
 	common/gdb_tilde_expand.c \
 	common/gdb_vecs.c \
 	common/new-op.c \
+	common/pathstuff.c \
 	common/print-utils.c \
 	common/ptid.c \
 	common/rsp-low.c \
@@ -1427,6 +1428,7 @@ HFILES_NO_SRCDIR = \
 	common/gdb_wait.h \
 	common/common-inferior.h \
 	common/host-defs.h \
+	common/pathstuff.h \
 	common/print-utils.h \
 	common/ptid.h \
 	common/queue.h \
@@ -1552,6 +1554,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	gdb_vecs.o \
 	mi/mi-common.o \
 	new-op.o \
+	pathstuff.o \
 	print-utils.o \
 	ptid.o \
 	rsp-low.o \
diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
new file mode 100644
index 0000000000..42af3fa65a
--- /dev/null
+++ b/gdb/common/pathstuff.c
@@ -0,0 +1,144 @@
+/* Path manipulation routines for GDB and gdbserver.
+
+   Copyright (C) 1986-2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#include "common-defs.h"
+#include "pathstuff.h"
+#include "host-defs.h"
+#include "filenames.h"
+#include "gdb_tilde_expand.h"
+
+gdb::unique_xmalloc_ptr<char>
+gdb_realpath (const char *filename)
+{
+/* On most hosts, we rely on canonicalize_file_name to compute
+   the FILENAME's realpath.
+
+   But the situation is slightly more complex on Windows, due to some
+   versions of GCC which were reported to generate paths where
+   backlashes (the directory separator) were doubled.  For instance:
+      c:\\some\\double\\slashes\\dir
+   ... instead of ...
+      c:\some\double\slashes\dir
+   Those double-slashes were getting in the way when comparing paths,
+   for instance when trying to insert a breakpoint as follow:
+      (gdb) b c:/some/double/slashes/dir/foo.c:4
+      No source file named c:/some/double/slashes/dir/foo.c:4.
+      (gdb) b c:\some\double\slashes\dir\foo.c:4
+      No source file named c:\some\double\slashes\dir\foo.c:4.
+   To prevent this from happening, we need this function to always
+   strip those extra backslashes.  While canonicalize_file_name does
+   perform this simplification, it only works when the path is valid.
+   Since the simplification would be useful even if the path is not
+   valid (one can always set a breakpoint on a file, even if the file
+   does not exist locally), we rely instead on GetFullPathName to
+   perform the canonicalization.  */
+
+#if defined (_WIN32)
+  {
+    char buf[MAX_PATH];
+    DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
+
+    /* The file system is case-insensitive but case-preserving.
+       So it is important we do not lowercase the path.  Otherwise,
+       we might not be able to display the original casing in a given
+       path.  */
+    if (len > 0 && len < MAX_PATH)
+      return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
+  }
+#else
+  {
+    char *rp = canonicalize_file_name (filename);
+
+    if (rp != NULL)
+      return gdb::unique_xmalloc_ptr<char> (rp);
+  }
+#endif
+
+  /* This system is a lost cause, just dup the buffer.  */
+  return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
+}
+
+/* See common/pathstuff.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_realpath_keepfile (const char *filename)
+{
+  const char *base_name = lbasename (filename);
+  char *dir_name;
+  char *result;
+
+  /* Extract the basename of filename, and return immediately 
+     a copy of filename if it does not contain any directory prefix.  */
+  if (base_name == filename)
+    return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
+
+  dir_name = (char *) alloca ((size_t) (base_name - filename + 2));
+  /* Allocate enough space to store the dir_name + plus one extra
+     character sometimes needed under Windows (see below), and
+     then the closing \000 character.  */
+  strncpy (dir_name, filename, base_name - filename);
+  dir_name[base_name - filename] = '\000';
+
+#ifdef HAVE_DOS_BASED_FILE_SYSTEM
+  /* We need to be careful when filename is of the form 'd:foo', which
+     is equivalent of d:./foo, which is totally different from d:/foo.  */
+  if (strlen (dir_name) == 2 && isalpha (dir_name[0]) && dir_name[1] == ':')
+    {
+      dir_name[2] = '.';
+      dir_name[3] = '\000';
+    }
+#endif
+
+  /* Canonicalize the directory prefix, and build the resulting
+     filename.  If the dirname realpath already contains an ending
+     directory separator, avoid doubling it.  */
+  gdb::unique_xmalloc_ptr<char> path_storage = gdb_realpath (dir_name);
+  const char *real_path = path_storage.get ();
+  if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
+    result = concat (real_path, base_name, (char *) NULL);
+  else
+    result = concat (real_path, SLASH_STRING, base_name, (char *) NULL);
+
+  return gdb::unique_xmalloc_ptr<char> (result);
+}
+
+/* See common/pathstuff.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_abspath (const char *path)
+{
+  gdb_assert (path != NULL && path[0] != '\0');
+
+  if (path[0] == '~')
+    {
+      std::string new_path = gdb_tilde_expand (path);
+
+      return gdb::unique_xmalloc_ptr<char> (xstrdup (new_path.c_str ()));
+    }
+
+  if (IS_ABSOLUTE_PATH (path))
+    return gdb::unique_xmalloc_ptr<char> (xstrdup (path));
+
+  /* Beware the // my son, the Emacs barfs, the botch that catch...  */
+  return gdb::unique_xmalloc_ptr<char>
+    (concat (current_directory,
+	     IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
+	     ? "" : SLASH_STRING,
+	     path, (char *) NULL));
+}
diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h
new file mode 100644
index 0000000000..909fd786bb
--- /dev/null
+++ b/gdb/common/pathstuff.h
@@ -0,0 +1,39 @@
+/* Path manipulation routines for GDB and gdbserver.
+
+   Copyright (C) 1986-2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#ifndef PATHSTUFF_H
+#define PATHSTUFF_H
+
+/* Path utilities.  */
+
+extern gdb::unique_xmalloc_ptr<char> gdb_realpath (const char *filename);
+
+/* Return a copy of FILENAME, with its directory prefix canonicalized
+   by gdb_realpath.  */
+
+extern gdb::unique_xmalloc_ptr<char>
+  gdb_realpath_keepfile (const char *filename);
+
+/* Return PATH in absolute form, performing tilde-expansion if necessary.
+   PATH cannot be NULL or the empty string.
+   This does not resolve symlinks however, use gdb_realpath for that.  */
+
+extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);
+
+#endif /* PATHSTUFF_H */
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 3ce086d70f..4ac068c6dd 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -209,6 +209,7 @@ SFILES = \
 	$(srcdir)/common/gdb_tilde_expand.c \
 	$(srcdir)/common/gdb_vecs.c \
 	$(srcdir)/common/new-op.c \
+	$(srcdir)/common/pathstuff.c \
 	$(srcdir)/common/print-utils.c \
 	$(srcdir)/common/ptid.c \
 	$(srcdir)/common/rsp-low.c \
@@ -256,6 +257,7 @@ OBS = \
 	mem-break.o \
 	new-op.o \
 	notif.o \
+	pathstuff.o \
 	print-utils.o \
 	ptid.o \
 	regcache.o \
diff --git a/gdb/utils.c b/gdb/utils.c
index c531748fe4..0acfad60cf 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -2838,57 +2838,6 @@ string_to_core_addr (const char *my_string)
   return addr;
 }
 
-gdb::unique_xmalloc_ptr<char>
-gdb_realpath (const char *filename)
-{
-/* On most hosts, we rely on canonicalize_file_name to compute
-   the FILENAME's realpath.
-
-   But the situation is slightly more complex on Windows, due to some
-   versions of GCC which were reported to generate paths where
-   backlashes (the directory separator) were doubled.  For instance:
-      c:\\some\\double\\slashes\\dir
-   ... instead of ...
-      c:\some\double\slashes\dir
-   Those double-slashes were getting in the way when comparing paths,
-   for instance when trying to insert a breakpoint as follow:
-      (gdb) b c:/some/double/slashes/dir/foo.c:4
-      No source file named c:/some/double/slashes/dir/foo.c:4.
-      (gdb) b c:\some\double\slashes\dir\foo.c:4
-      No source file named c:\some\double\slashes\dir\foo.c:4.
-   To prevent this from happening, we need this function to always
-   strip those extra backslashes.  While canonicalize_file_name does
-   perform this simplification, it only works when the path is valid.
-   Since the simplification would be useful even if the path is not
-   valid (one can always set a breakpoint on a file, even if the file
-   does not exist locally), we rely instead on GetFullPathName to
-   perform the canonicalization.  */
-
-#if defined (_WIN32)
-  {
-    char buf[MAX_PATH];
-    DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
-
-    /* The file system is case-insensitive but case-preserving.
-       So it is important we do not lowercase the path.  Otherwise,
-       we might not be able to display the original casing in a given
-       path.  */
-    if (len > 0 && len < MAX_PATH)
-      return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
-  }
-#else
-  {
-    char *rp = canonicalize_file_name (filename);
-
-    if (rp != NULL)
-      return gdb::unique_xmalloc_ptr<char> (rp);
-  }
-#endif
-
-  /* This system is a lost cause, just dup the buffer.  */
-  return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
-}
-
 #if GDB_SELF_TEST
 
 static void
@@ -2925,74 +2874,6 @@ gdb_realpath_tests ()
 
 #endif /* GDB_SELF_TEST */
 
-/* Return a copy of FILENAME, with its directory prefix canonicalized
-   by gdb_realpath.  */
-
-gdb::unique_xmalloc_ptr<char>
-gdb_realpath_keepfile (const char *filename)
-{
-  const char *base_name = lbasename (filename);
-  char *dir_name;
-  char *result;
-
-  /* Extract the basename of filename, and return immediately 
-     a copy of filename if it does not contain any directory prefix.  */
-  if (base_name == filename)
-    return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
-
-  dir_name = (char *) alloca ((size_t) (base_name - filename + 2));
-  /* Allocate enough space to store the dir_name + plus one extra
-     character sometimes needed under Windows (see below), and
-     then the closing \000 character.  */
-  strncpy (dir_name, filename, base_name - filename);
-  dir_name[base_name - filename] = '\000';
-
-#ifdef HAVE_DOS_BASED_FILE_SYSTEM
-  /* We need to be careful when filename is of the form 'd:foo', which
-     is equivalent of d:./foo, which is totally different from d:/foo.  */
-  if (strlen (dir_name) == 2 && isalpha (dir_name[0]) && dir_name[1] == ':')
-    {
-      dir_name[2] = '.';
-      dir_name[3] = '\000';
-    }
-#endif
-
-  /* Canonicalize the directory prefix, and build the resulting
-     filename.  If the dirname realpath already contains an ending
-     directory separator, avoid doubling it.  */
-  gdb::unique_xmalloc_ptr<char> path_storage = gdb_realpath (dir_name);
-  const char *real_path = path_storage.get ();
-  if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
-    result = concat (real_path, base_name, (char *) NULL);
-  else
-    result = concat (real_path, SLASH_STRING, base_name, (char *) NULL);
-
-  return gdb::unique_xmalloc_ptr<char> (result);
-}
-
-/* Return PATH in absolute form, performing tilde-expansion if necessary.
-   PATH cannot be NULL or the empty string.
-   This does not resolve symlinks however, use gdb_realpath for that.  */
-
-gdb::unique_xmalloc_ptr<char>
-gdb_abspath (const char *path)
-{
-  gdb_assert (path != NULL && path[0] != '\0');
-
-  if (path[0] == '~')
-    return gdb::unique_xmalloc_ptr<char> (tilde_expand (path));
-
-  if (IS_ABSOLUTE_PATH (path))
-    return gdb::unique_xmalloc_ptr<char> (xstrdup (path));
-
-  /* Beware the // my son, the Emacs barfs, the botch that catch...  */
-  return gdb::unique_xmalloc_ptr<char>
-    (concat (current_directory,
-	     IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
-	     ? "" : SLASH_STRING,
-	     path, (char *) NULL));
-}
-
 ULONGEST
 align_up (ULONGEST v, int n)
 {
diff --git a/gdb/utils.h b/gdb/utils.h
index b234762929..4f25be0968 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -23,6 +23,7 @@
 
 #include "exceptions.h"
 #include "common/scoped_restore.h"
+#include "common/pathstuff.h"
 #include <chrono>
 
 extern void initialize_utils (void);
@@ -295,12 +296,6 @@ extern struct cleanup *make_bpstat_clear_actions_cleanup (void);
 \f
 /* Path utilities.  */
 
-extern gdb::unique_xmalloc_ptr<char> gdb_realpath (const char *);
-
-extern gdb::unique_xmalloc_ptr<char> gdb_realpath_keepfile (const char *);
-
-extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *);
-
 extern int gdb_filename_fnmatch (const char *pattern, const char *string,
 				 int flags);
 
-- 
2.14.3

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

[Sergio Durigan Junior]

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/ChangeLog:
2018-02-09  Sergio Durigan Junior  <sergiodj@redhat.com>

	* common/common-def.h (current_directory): Move here...
	* defs.h (current_directory): ...from here.

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

	* server.c: Include "filenames.h" and "pathstuff.h".
	(current_directory): New global variable.
	(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-09  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.server/abspath.exp: New file.
	* gdb.server/non-existing-program.exp: Adjust regex for the
	"startup-with-shell enabled" case.
---
 gdb/common/common-defs.h                          |  3 ++
 gdb/defs.h                                        |  4 --
 gdb/gdbserver/server.c                            | 36 +++++++++++++++
 gdb/testsuite/gdb.server/abspath.exp              | 54 +++++++++++++++++++++++
 gdb/testsuite/gdb.server/non-existing-program.exp |  2 +-
 5 files changed, 94 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/abspath.exp

diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index acbc32ca69..881a4eaaff 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -91,4 +91,7 @@
 /* Pull in gdb::unique_xmalloc_ptr.  */
 #include "common/gdb_unique_ptr.h"
 
+/* String containing the current directory (what getwd would return).  */
+extern char *current_directory;
+
 #endif /* COMMON_DEFS_H */
diff --git a/gdb/defs.h b/gdb/defs.h
index 4fb2129b30..61be475858 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -423,10 +423,6 @@ enum info_proc_what
     IP_ALL
   };
 
-/* * String containing the current directory (what getwd would return).  */
-
-extern char *current_directory;
-
 /* * Default radixes for input and output.  Only some values supported.  */
 extern unsigned input_radix;
 extern unsigned output_radix;
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index cb02b58507..2d87886daf 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"
 
@@ -56,6 +58,10 @@
       break;					\
     }
 
+/* String containing the current directory (what getwd would return).  */
+
+char *current_directory;
+
 /* The environment to pass to the inferior when creating it.  */
 
 static gdb_environ our_environ;
@@ -279,6 +285,23 @@ 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))
+    {
+      char *tmp_program_name = program_name;
+
+      program_name = gdb_abspath (program_name).release ();
+      xfree (tmp_program_name);
+    }
+}
+
 static int
 attach_inferior (int pid)
 {
@@ -3012,6 +3035,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;
@@ -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));
+    }
+
   while (*next_arg != NULL && **next_arg == '-')
     {
       if (strcmp (*next_arg, "--version") == 0)
@@ -3759,6 +3791,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);
 
@@ -4279,6 +4313,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/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"
diff --git a/gdb/testsuite/gdb.server/non-existing-program.exp b/gdb/testsuite/gdb.server/non-existing-program.exp
index a8e057d590..af5b412992 100644
--- a/gdb/testsuite/gdb.server/non-existing-program.exp
+++ b/gdb/testsuite/gdb.server/non-existing-program.exp
@@ -48,7 +48,7 @@ expect {
     }
     # Likewise, but with startup-with-shell enabled, which is the
     # default behaviour.
-    -re "stdin/stdout redirected.*exec: non-existing-program: not found\r\nDuring startup program exited with code 127\.\r\nExiting\r\n$" {
+    -re "stdin/stdout redirected.*: .*non-existing-program: No such file or directory\r\nDuring startup program exited with code 127\.\r\nExiting\r\n$" {
 	set saw_exiting 1
 	exp_continue
     }
-- 
2.14.3

[PATCH 0/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

These two patches fix the issue pointed by Simon on IRC, that
gdbserver doesn't recognize filename-only binaries anymore:

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

The first one is a preparation patch (and can go in independently),
which moves path-manipulation functions from utils.c to a new
common/pathstuff.c, making them available for gdbserver as well.

The second patch is the fix itself.

More details in each message.

Re: [PATCH 1/2] Create new common/pathstuff.[ch]

[Simon Marchi]

On 2018-02-09 08:42 PM, Sergio Durigan Junior wrote:
> This commit moves the path manipulation routines found on utils.c to a
> new common/pathstuff.c, and updates the Makefile.in's accordingly.
> The routines moved are "gdb_realpath", "gdb_realpath_keepfile" and
> "gdb_abspath".
> 
> This will be needed because gdbserver will have to call "gdb_abspath"
> on my next patch, which implements a way to expand the path of the
> inferior provided by the user in order to allow specifying just the
> binary name when starting gdbserver, like:
> 
>   $ gdbserver :1234 a.out
> 
> With the recent addition of the startup-with-shell feature on
> gdbserver, this scenario doesn't work anymore if the user doesn't have
> the current directory listed in the PATH variable.
> 
> I had to do a minor adjustment on "gdb_abspath" because we don't have
> access to "tilde_expand" on gdbserver, so now the function is using
> "gdb_tilde_expand" instead.  Otherwise, the code is the same.
> 
> Regression tested on the BuildBot, without regressions.

Hi Sergio,

Thanks for looking into this!

This commit does not build:

/home/simark/src/binutils-gdb/gdb/common/pathstuff.c: In function ‘gdb::unique_xmalloc_ptr<char> gdb_abspath(const char*)’:
/home/simark/src/binutils-gdb/gdb/common/pathstuff.c:140:14: error: ‘current_directory’ was not declared in this scope
     (concat (current_directory,
              ^~~~~~~~~~~~~~~~~
/home/simark/src/binutils-gdb/gdb/common/pathstuff.c:140:14: note: suggested alternative: ‘read_direction’
     (concat (current_directory,
              ^~~~~~~~~~~~~~~~~
              read_direction


I guess you need to move the declaration to common-defs.h in this commit instead of
the next one.  I also got this whitespace error from git am:

.git/rebase-apply/patch:131: trailing whitespace.
  /* Extract the basename of filename, and return immediately

I think it's in code you copied, but if you can remove the extra space while at it
it would be nice.

> +/* See common/pathstuff.h.  */
> +
> +gdb::unique_xmalloc_ptr<char>
> +gdb_abspath (const char *path)
> +{
> +  gdb_assert (path != NULL && path[0] != '\0');
> +
> +  if (path[0] == '~')
> +    {
> +      std::string new_path = gdb_tilde_expand (path);
> +
> +      return gdb::unique_xmalloc_ptr<char> (xstrdup (new_path.c_str ()));

We should try to avoid unnecessary copies, when possible.  Here, we could either make
another version of gdb_tilde_expand (it would have to be another name) that returns
a gdb::unique_xmalloc_ptr<char> or make gdb_abspath return an std::string.  I think the
former would be better for now because some callers require a gdb::unique_xmalloc_ptr<char>,
and would have to do a copy themselves.  So using a gdb::unique_xmalloc_ptr across the
whole chain would give the least amount of copies.

> diff --git a/gdb/utils.h b/gdb/utils.h
> index b234762929..4f25be0968 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -23,6 +23,7 @@
>  
>  #include "exceptions.h"
>  #include "common/scoped_restore.h"
> +#include "common/pathstuff.h"

I don't think utils.h should be including common/pathstuff.h, because it is not using it.
I understand why you did this (this ensures that every current user of these functions
will automatically see the new declaration), but in the long term I think it's better if
the users include "pathstuff.h".  The files that use these functions are:

$ grep -e gdb_realpath_keepfile -e gdb_realpath -e gdb_abspath *.c */*.c -l | sort
auto-load.c
common/pathstuff.c
compile/compile.c
dwarf2read.c
exec.c
guile/scm-safe-call.c
linux-thread-db.c
main.c
nto-tdep.c
objfiles.c
source.c
symtab.c
utils.c

They are all files included in a --enable-targets=all build, so it should be easy to
find where #include "common/pathstuff.h" is missing.

Thanks,

Simon

Re: [PATCH 2/2] Make gdbserver work with filename-only binaries

[Simon Marchi]

On 2018-02-09 08:42 PM, Sergio Durigan Junior wrote:
> 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.

Hi Sergio,

The behavior is still different than for GDB (and previous gdbservers), in
the case where you specify a filename-only binary that is found in PATH.  For
example, try "gdb ls" and/or "gdbserver ls".  The expected behavior is to
search for a file with this name in the current directory, and if there isn't
one, to search in the PATH.  This is what openp does when OPF_TRY_CWD_FIRST
is passed.

Bringing openp to gdbserver may not be easy nor desirable, since it supports
some concepts that don't exist in gdbserver (like $-variables).  Also, we would
not really want to open the file in this case, only see if it exists.

I didn't think this through completely, but maybe we could do something simpler,
if the program_name doesn't contain a directory separator and the file exists in
the current working directory, we add "./" in front of it when passing it to the
shell?  I think all three use cases would work:

- gdbserver :1234 foo (foo in current directory)
- gdbserver :1234 foo (foo in PATH)
- gdbserver :1234 ./foo

Simon

Re: [PATCH 1/2] Create new common/pathstuff.[ch]

[Sergio Durigan Junior]

On Sunday, February 11 2018, Simon Marchi wrote:

> On 2018-02-09 08:42 PM, Sergio Durigan Junior wrote:
>> This commit moves the path manipulation routines found on utils.c to a
>> new common/pathstuff.c, and updates the Makefile.in's accordingly.
>> The routines moved are "gdb_realpath", "gdb_realpath_keepfile" and
>> "gdb_abspath".
>> 
>> This will be needed because gdbserver will have to call "gdb_abspath"
>> on my next patch, which implements a way to expand the path of the
>> inferior provided by the user in order to allow specifying just the
>> binary name when starting gdbserver, like:
>> 
>>   $ gdbserver :1234 a.out
>> 
>> With the recent addition of the startup-with-shell feature on
>> gdbserver, this scenario doesn't work anymore if the user doesn't have
>> the current directory listed in the PATH variable.
>> 
>> I had to do a minor adjustment on "gdb_abspath" because we don't have
>> access to "tilde_expand" on gdbserver, so now the function is using
>> "gdb_tilde_expand" instead.  Otherwise, the code is the same.
>> 
>> Regression tested on the BuildBot, without regressions.
>
> Hi Sergio,

Hey, Simon,

> Thanks for looking into this!

My pleasure.

> This commit does not build:
>
> /home/simark/src/binutils-gdb/gdb/common/pathstuff.c: In function ‘gdb::unique_xmalloc_ptr<char> gdb_abspath(const char*)’:
> /home/simark/src/binutils-gdb/gdb/common/pathstuff.c:140:14: error: ‘current_directory’ was not declared in this scope
>      (concat (current_directory,
>               ^~~~~~~~~~~~~~~~~
> /home/simark/src/binutils-gdb/gdb/common/pathstuff.c:140:14: note: suggested alternative: ‘read_direction’
>      (concat (current_directory,
>               ^~~~~~~~~~~~~~~~~
>               read_direction

Ah.  Sorry about that; that's the problem of testing on BuildBot with
everything-in-one-patch.  I'll include the declaration of
current_directory in common-defs.h and also in gdbserver/server.c.

> I guess you need to move the declaration to common-defs.h in this commit instead of
> the next one.  I also got this whitespace error from git am:
>
> .git/rebase-apply/patch:131: trailing whitespace.
>   /* Extract the basename of filename, and return immediately
>
> I think it's in code you copied, but if you can remove the extra space while at it
> it would be nice.

Sure.

>> +/* See common/pathstuff.h.  */
>> +
>> +gdb::unique_xmalloc_ptr<char>
>> +gdb_abspath (const char *path)
>> +{
>> +  gdb_assert (path != NULL && path[0] != '\0');
>> +
>> +  if (path[0] == '~')
>> +    {
>> +      std::string new_path = gdb_tilde_expand (path);
>> +
>> +      return gdb::unique_xmalloc_ptr<char> (xstrdup (new_path.c_str ()));
>
> We should try to avoid unnecessary copies, when possible.  Here, we could either make
> another version of gdb_tilde_expand (it would have to be another name) that returns
> a gdb::unique_xmalloc_ptr<char> or make gdb_abspath return an std::string.  I think the
> former would be better for now because some callers require a gdb::unique_xmalloc_ptr<char>,
> and would have to do a copy themselves.  So using a gdb::unique_xmalloc_ptr across the
> whole chain would give the least amount of copies.

OK.

>> diff --git a/gdb/utils.h b/gdb/utils.h
>> index b234762929..4f25be0968 100644
>> --- a/gdb/utils.h
>> +++ b/gdb/utils.h
>> @@ -23,6 +23,7 @@
>>  
>>  #include "exceptions.h"
>>  #include "common/scoped_restore.h"
>> +#include "common/pathstuff.h"
>
> I don't think utils.h should be including common/pathstuff.h, because it is not using it.
> I understand why you did this (this ensures that every current user of these functions
> will automatically see the new declaration), but in the long term I think it's better if
> the users include "pathstuff.h".  The files that use these functions are:
>
> $ grep -e gdb_realpath_keepfile -e gdb_realpath -e gdb_abspath *.c */*.c -l | sort
> auto-load.c
> common/pathstuff.c
> compile/compile.c
> dwarf2read.c
> exec.c
> guile/scm-safe-call.c
> linux-thread-db.c
> main.c
> nto-tdep.c
> objfiles.c
> source.c
> symtab.c
> utils.c
>
> They are all files included in a --enable-targets=all build, so it should be easy to
> find where #include "common/pathstuff.h" is missing.

Alright, no problem.

I'll send a v2 soon.

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/

Re: [PATCH 2/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

On Sunday, February 11 2018, Simon Marchi wrote:

> On 2018-02-09 08:42 PM, Sergio Durigan Junior wrote:
>> 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.
>
> Hi Sergio,

Hey, Simon,

> The behavior is still different than for GDB (and previous gdbservers), in
> the case where you specify a filename-only binary that is found in PATH.  For
> example, try "gdb ls" and/or "gdbserver ls".  The expected behavior is to
> search for a file with this name in the current directory, and if there isn't
> one, to search in the PATH.  This is what openp does when OPF_TRY_CWD_FIRST
> is passed.

Ah, I guess I didn't consider this (obvious) scenario for gdbserver.  I
was thinking that gdbserver (before the startup-with-shell feature)
would not work with binaries in PATH...

> Bringing openp to gdbserver may not be easy nor desirable, since it supports
> some concepts that don't exist in gdbserver (like $-variables).  Also, we would
> not really want to open the file in this case, only see if it exists.

Yeah, it crossed my mind to move openp to common, but it's not a trivial
task as you pointed.

> I didn't think this through completely, but maybe we could do something simpler,
> if the program_name doesn't contain a directory separator and the file exists in
> the current working directory, we add "./" in front of it when passing it to the
> shell?  I think all three use cases would work:
>
> - gdbserver :1234 foo (foo in current directory)
> - gdbserver :1234 foo (foo in PATH)
> - gdbserver :1234 ./foo

So, what do you think of checking if the file exists in the CWD (and is
executable), and prefixing it with current_directory, as I'm doing with
this patch?  I personally prefer to be more verbose, so using the full
path is better IMHO than just adding "./".

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 v2 2/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

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

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)
 }
 \f
 
-/* 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"
-- 
2.14.3

[PATCH v2 1/2] Create new common/pathstuff.[ch]

[Sergio Durigan Junior]

Changes from v1:

- Moved all of the "current_directory" modifications to this patch.

- Eliminated spurious whitespace from comment.

- Added new "gdb_tilde_expand_up" function, which returns a
  "gdb::unique_xmalloc_ptr<char>".  Updated "gdb_abspath" to use it.

- Removed #include of "common/pathstuff.h" from "utils.h".  Instead,
  include "common/pathstuff.h" only in those files that need it.



This commit moves the path manipulation routines found on utils.c to a
new common/pathstuff.c, and updates the Makefile.in's accordingly.
The routines moved are "gdb_realpath", "gdb_realpath_keepfile" and
"gdb_abspath".

This will be needed because gdbserver will have to call "gdb_abspath"
on my next patch, which implements a way to expand the path of the
inferior provided by the user in order to allow specifying just the
binary name when starting gdbserver, like:

  $ gdbserver :1234 a.out

With the recent addition of the startup-with-shell feature on
gdbserver, this scenario doesn't work anymore if the user doesn't have
the current directory listed in the PATH variable.

I had to do a minor adjustment on "gdb_abspath" because we don't have
access to "tilde_expand" on gdbserver, so now the function is using
"gdb_tilde_expand" instead.  Otherwise, the code is the same.

Regression tested on the BuildBot, without regressions.

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

	* Makefile.in (SFILES): Add "common/pathstuff.c".
	(HFILES_NO_SRCDIR): Add "common/pathstuff.h".
	(COMMON_OBS): Add "pathstuff.o".
	* auto-load.c: Include "common/pathstuff.h".
	* common/common-def.h (current_directory): Move here.
	* common/gdb_tilde_expand.c (gdb_tilde_expand_up): New
	function.
	* common/gdb_tilde_expand.h (gdb_tilde_expand_up): New
	prototype.
	* common/pathstuff.c: New file.
	* common/pathstuff.h: New file.
	* compile/compile.c: Include "common/pathstuff.h".
	* defs.h (current_directory): Move to "common/common-defs.h".
	* dwarf2read.c: Include "common/pathstuff.h".
	* exec.c: Likewise.
	* guile/scm-safe-call.c: Likewise.
	* linux-thread-db.c: Likewise.
	* main.c: Likewise.
	* nto-tdep.c: Likewise.
	* objfiles.c: Likewise.
	* source.c: Likewise.
	* symtab.c: Likewise.
	* utils.c: Include "common/pathstuff.h".
	(gdb_realpath): Move to "common/pathstuff.c".
	(gdb_realpath_keepfile): Likewise.
	(gdb_abspath): Likewise.
	* utils.h (gdb_realpath): Move to "common/pathstuff.h".
	(gdb_realpath_keepfile): Likewise.
	(gdb_abspath): Likewise.

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

	* server.c (current_directory): New global variable.
	(captured_main): Initialize "current_directory".
	* Makefile.in (SFILES): Add "$(srcdir)/common/pathstuff.c".
	(OBJS): Add "pathstuff.o".
---
 gdb/Makefile.in               |   3 +
 gdb/auto-load.c               |   1 +
 gdb/common/common-defs.h      |   3 +
 gdb/common/gdb_tilde_expand.c |  13 ++++
 gdb/common/gdb_tilde_expand.h |   4 ++
 gdb/common/pathstuff.c        | 140 ++++++++++++++++++++++++++++++++++++++++++
 gdb/common/pathstuff.h        |  39 ++++++++++++
 gdb/compile/compile.c         |   1 +
 gdb/defs.h                    |   4 --
 gdb/dwarf2read.c              |   1 +
 gdb/exec.c                    |   1 +
 gdb/gdbserver/Makefile.in     |   2 +
 gdb/gdbserver/server.c        |  11 ++++
 gdb/guile/scm-safe-call.c     |   1 +
 gdb/linux-thread-db.c         |   1 +
 gdb/main.c                    |   1 +
 gdb/nto-tdep.c                |   1 +
 gdb/objfiles.c                |   1 +
 gdb/source.c                  |   1 +
 gdb/symtab.c                  |   1 +
 gdb/utils.c                   | 120 +-----------------------------------
 gdb/utils.h                   |   6 --
 22 files changed, 227 insertions(+), 129 deletions(-)
 create mode 100644 gdb/common/pathstuff.c
 create mode 100644 gdb/common/pathstuff.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 957654c9bd..64c9df3eaf 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1136,6 +1136,7 @@ SFILES = \
 	common/gdb_tilde_expand.c \
 	common/gdb_vecs.c \
 	common/new-op.c \
+	common/pathstuff.c \
 	common/print-utils.c \
 	common/ptid.c \
 	common/rsp-low.c \
@@ -1427,6 +1428,7 @@ HFILES_NO_SRCDIR = \
 	common/gdb_wait.h \
 	common/common-inferior.h \
 	common/host-defs.h \
+	common/pathstuff.h \
 	common/print-utils.h \
 	common/ptid.h \
 	common/queue.h \
@@ -1552,6 +1554,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	gdb_vecs.o \
 	mi/mi-common.o \
 	new-op.o \
+	pathstuff.o \
 	print-utils.o \
 	ptid.o \
 	rsp-low.o \
diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index b79341faf6..9dd8754e1a 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -40,6 +40,7 @@
 #include "extension.h"
 #include "gdb/section-scripts.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 /* The section to look in for auto-loaded scripts (in file formats that
    support sections).
diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index acbc32ca69..881a4eaaff 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -91,4 +91,7 @@
 /* Pull in gdb::unique_xmalloc_ptr.  */
 #include "common/gdb_unique_ptr.h"
 
+/* String containing the current directory (what getwd would return).  */
+extern char *current_directory;
+
 #endif /* COMMON_DEFS_H */
diff --git a/gdb/common/gdb_tilde_expand.c b/gdb/common/gdb_tilde_expand.c
index b4f371464d..fcb97961ac 100644
--- a/gdb/common/gdb_tilde_expand.c
+++ b/gdb/common/gdb_tilde_expand.c
@@ -80,3 +80,16 @@ gdb_tilde_expand (const char *dir)
 
   return expanded_dir;
 }
+
+/* See common/gdb_tilde_expand.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_tilde_expand_up (const char *dir)
+{
+  gdb_glob glob (dir, GLOB_TILDE_CHECK, NULL);
+
+  gdb_assert (glob.pathc () > 0);
+  /* "glob" may return more than one match to the path provided by the
+     user, but we are only interested in the first match.  */
+  return gdb::unique_xmalloc_ptr<char> (xstrdup (glob.pathv ()[0]));
+}
diff --git a/gdb/common/gdb_tilde_expand.h b/gdb/common/gdb_tilde_expand.h
index d0dfb37857..22860d3969 100644
--- a/gdb/common/gdb_tilde_expand.h
+++ b/gdb/common/gdb_tilde_expand.h
@@ -24,4 +24,8 @@
    the full path.  */
 extern std::string gdb_tilde_expand (const char *dir);
 
+/* Same as GDB_TILDE_EXPAND, but return the full path as a
+   gdb::unique_xmalloc_ptr<char>.  */
+extern gdb::unique_xmalloc_ptr<char> gdb_tilde_expand_up (const char *dir);
+
 #endif /* ! GDB_TILDE_EXPAND_H */
diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
new file mode 100644
index 0000000000..fc51edffa9
--- /dev/null
+++ b/gdb/common/pathstuff.c
@@ -0,0 +1,140 @@
+/* Path manipulation routines for GDB and gdbserver.
+
+   Copyright (C) 1986-2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#include "common-defs.h"
+#include "pathstuff.h"
+#include "host-defs.h"
+#include "filenames.h"
+#include "gdb_tilde_expand.h"
+
+gdb::unique_xmalloc_ptr<char>
+gdb_realpath (const char *filename)
+{
+/* On most hosts, we rely on canonicalize_file_name to compute
+   the FILENAME's realpath.
+
+   But the situation is slightly more complex on Windows, due to some
+   versions of GCC which were reported to generate paths where
+   backlashes (the directory separator) were doubled.  For instance:
+      c:\\some\\double\\slashes\\dir
+   ... instead of ...
+      c:\some\double\slashes\dir
+   Those double-slashes were getting in the way when comparing paths,
+   for instance when trying to insert a breakpoint as follow:
+      (gdb) b c:/some/double/slashes/dir/foo.c:4
+      No source file named c:/some/double/slashes/dir/foo.c:4.
+      (gdb) b c:\some\double\slashes\dir\foo.c:4
+      No source file named c:\some\double\slashes\dir\foo.c:4.
+   To prevent this from happening, we need this function to always
+   strip those extra backslashes.  While canonicalize_file_name does
+   perform this simplification, it only works when the path is valid.
+   Since the simplification would be useful even if the path is not
+   valid (one can always set a breakpoint on a file, even if the file
+   does not exist locally), we rely instead on GetFullPathName to
+   perform the canonicalization.  */
+
+#if defined (_WIN32)
+  {
+    char buf[MAX_PATH];
+    DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
+
+    /* The file system is case-insensitive but case-preserving.
+       So it is important we do not lowercase the path.  Otherwise,
+       we might not be able to display the original casing in a given
+       path.  */
+    if (len > 0 && len < MAX_PATH)
+      return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
+  }
+#else
+  {
+    char *rp = canonicalize_file_name (filename);
+
+    if (rp != NULL)
+      return gdb::unique_xmalloc_ptr<char> (rp);
+  }
+#endif
+
+  /* This system is a lost cause, just dup the buffer.  */
+  return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
+}
+
+/* See common/pathstuff.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_realpath_keepfile (const char *filename)
+{
+  const char *base_name = lbasename (filename);
+  char *dir_name;
+  char *result;
+
+  /* Extract the basename of filename, and return immediately
+     a copy of filename if it does not contain any directory prefix.  */
+  if (base_name == filename)
+    return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
+
+  dir_name = (char *) alloca ((size_t) (base_name - filename + 2));
+  /* Allocate enough space to store the dir_name + plus one extra
+     character sometimes needed under Windows (see below), and
+     then the closing \000 character.  */
+  strncpy (dir_name, filename, base_name - filename);
+  dir_name[base_name - filename] = '\000';
+
+#ifdef HAVE_DOS_BASED_FILE_SYSTEM
+  /* We need to be careful when filename is of the form 'd:foo', which
+     is equivalent of d:./foo, which is totally different from d:/foo.  */
+  if (strlen (dir_name) == 2 && isalpha (dir_name[0]) && dir_name[1] == ':')
+    {
+      dir_name[2] = '.';
+      dir_name[3] = '\000';
+    }
+#endif
+
+  /* Canonicalize the directory prefix, and build the resulting
+     filename.  If the dirname realpath already contains an ending
+     directory separator, avoid doubling it.  */
+  gdb::unique_xmalloc_ptr<char> path_storage = gdb_realpath (dir_name);
+  const char *real_path = path_storage.get ();
+  if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
+    result = concat (real_path, base_name, (char *) NULL);
+  else
+    result = concat (real_path, SLASH_STRING, base_name, (char *) NULL);
+
+  return gdb::unique_xmalloc_ptr<char> (result);
+}
+
+/* See common/pathstuff.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_abspath (const char *path)
+{
+  gdb_assert (path != NULL && path[0] != '\0');
+
+  if (path[0] == '~')
+    return gdb_tilde_expand_up (path);
+
+  if (IS_ABSOLUTE_PATH (path))
+    return gdb::unique_xmalloc_ptr<char> (xstrdup (path));
+
+  /* Beware the // my son, the Emacs barfs, the botch that catch...  */
+  return gdb::unique_xmalloc_ptr<char>
+    (concat (current_directory,
+	     IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
+	     ? "" : SLASH_STRING,
+	     path, (char *) NULL));
+}
diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h
new file mode 100644
index 0000000000..909fd786bb
--- /dev/null
+++ b/gdb/common/pathstuff.h
@@ -0,0 +1,39 @@
+/* Path manipulation routines for GDB and gdbserver.
+
+   Copyright (C) 1986-2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#ifndef PATHSTUFF_H
+#define PATHSTUFF_H
+
+/* Path utilities.  */
+
+extern gdb::unique_xmalloc_ptr<char> gdb_realpath (const char *filename);
+
+/* Return a copy of FILENAME, with its directory prefix canonicalized
+   by gdb_realpath.  */
+
+extern gdb::unique_xmalloc_ptr<char>
+  gdb_realpath_keepfile (const char *filename);
+
+/* Return PATH in absolute form, performing tilde-expansion if necessary.
+   PATH cannot be NULL or the empty string.
+   This does not resolve symlinks however, use gdb_realpath for that.  */
+
+extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);
+
+#endif /* PATHSTUFF_H */
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 82e63d895f..3a1bb987b2 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -41,6 +41,7 @@
 #include "valprint.h"
 #include "common/gdb_optional.h"
 #include "common/gdb_unlinker.h"
+#include "common/pathstuff.h"
 
 \f
 
diff --git a/gdb/defs.h b/gdb/defs.h
index 4fb2129b30..61be475858 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -423,10 +423,6 @@ enum info_proc_what
     IP_ALL
   };
 
-/* * String containing the current directory (what getwd would return).  */
-
-extern char *current_directory;
-
 /* * Default radixes for input and output.  Only some values supported.  */
 extern unsigned input_radix;
 extern unsigned output_radix;
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index d651725bbe..a392423a13 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -86,6 +86,7 @@
 #include <cmath>
 #include <set>
 #include <forward_list>
+#include "common/pathstuff.h"
 
 /* When == 1, print basic high level tracing messages.
    When > 1, be more verbose.
diff --git a/gdb/exec.c b/gdb/exec.c
index c8c32ecc27..a5d071ec51 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -44,6 +44,7 @@
 #include <sys/stat.h>
 #include "solist.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 void (*deprecated_file_changed_hook) (const char *);
 
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 3ce086d70f..4ac068c6dd 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -209,6 +209,7 @@ SFILES = \
 	$(srcdir)/common/gdb_tilde_expand.c \
 	$(srcdir)/common/gdb_vecs.c \
 	$(srcdir)/common/new-op.c \
+	$(srcdir)/common/pathstuff.c \
 	$(srcdir)/common/print-utils.c \
 	$(srcdir)/common/ptid.c \
 	$(srcdir)/common/rsp-low.c \
@@ -256,6 +257,7 @@ OBS = \
 	mem-break.o \
 	new-op.o \
 	notif.o \
+	pathstuff.o \
 	print-utils.o \
 	ptid.o \
 	regcache.o \
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index cb02b58507..f931273fa3 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -56,6 +56,10 @@
       break;					\
     }
 
+/* String containing the current directory (what getwd would return).  */
+
+char *current_directory;
+
 /* The environment to pass to the inferior when creating it.  */
 
 static gdb_environ our_environ;
@@ -3539,6 +3543,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));
+    }
+
   while (*next_arg != NULL && **next_arg == '-')
     {
       if (strcmp (*next_arg, "--version") == 0)
diff --git a/gdb/guile/scm-safe-call.c b/gdb/guile/scm-safe-call.c
index a9ce7b72c1..2cba399e23 100644
--- a/gdb/guile/scm-safe-call.c
+++ b/gdb/guile/scm-safe-call.c
@@ -23,6 +23,7 @@
 #include "defs.h"
 #include "filenames.h"
 #include "guile-internal.h"
+#include "common/pathstuff.h"
 
 /* Struct to marshall args to scscm_safe_call_body.  */
 
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 794c97b48a..f178244f05 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -46,6 +46,7 @@
 #include <ctype.h>
 #include "nat/linux-namespaces.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 /* GNU/Linux libthread_db support.
 
diff --git a/gdb/main.c b/gdb/main.c
index 3c98787edb..189266f90e 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -46,6 +46,7 @@
 #include "infrun.h"
 #include "signals-state-save-restore.h"
 #include <vector>
+#include "common/pathstuff.h"
 
 /* The selected interpreter.  This will be used as a set command
    variable, so it should always be malloc'ed - since
diff --git a/gdb/nto-tdep.c b/gdb/nto-tdep.c
index 03b2d1e96d..ab0db19b48 100644
--- a/gdb/nto-tdep.c
+++ b/gdb/nto-tdep.c
@@ -31,6 +31,7 @@
 #include "solib-svr4.h"
 #include "gdbcore.h"
 #include "objfiles.h"
+#include "common/pathstuff.h"
 
 #define QNX_NOTE_NAME	"QNX"
 #define QNX_INFO_SECT_NAME "QNX_info"
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 70e369b8b4..a9aaf89540 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -52,6 +52,7 @@
 #include "solist.h"
 #include "gdb_bfd.h"
 #include "btrace.h"
+#include "common/pathstuff.h"
 
 #include <vector>
 
diff --git a/gdb/source.c b/gdb/source.c
index 009bec5285..77f5e8d4d4 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -44,6 +44,7 @@
 #include "readline/readline.h"
 #include "common/enum-flags.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 #define OPEN_MODE (O_RDONLY | O_BINARY)
 #define FDOPEN_MODE FOPEN_RB
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 0fd3f3a30f..567195304f 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -66,6 +66,7 @@
 #include "filename-seen-cache.h"
 #include "arch-utils.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 /* Forward declarations for local functions.  */
 
diff --git a/gdb/utils.c b/gdb/utils.c
index c531748fe4..577f9df4ec 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -70,6 +70,7 @@
 #include "common/gdb_optional.h"
 #include "cp-support.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 #if !HAVE_DECL_MALLOC
 extern PTR malloc ();		/* ARI: PTR */
@@ -2838,57 +2839,6 @@ string_to_core_addr (const char *my_string)
   return addr;
 }
 
-gdb::unique_xmalloc_ptr<char>
-gdb_realpath (const char *filename)
-{
-/* On most hosts, we rely on canonicalize_file_name to compute
-   the FILENAME's realpath.
-
-   But the situation is slightly more complex on Windows, due to some
-   versions of GCC which were reported to generate paths where
-   backlashes (the directory separator) were doubled.  For instance:
-      c:\\some\\double\\slashes\\dir
-   ... instead of ...
-      c:\some\double\slashes\dir
-   Those double-slashes were getting in the way when comparing paths,
-   for instance when trying to insert a breakpoint as follow:
-      (gdb) b c:/some/double/slashes/dir/foo.c:4
-      No source file named c:/some/double/slashes/dir/foo.c:4.
-      (gdb) b c:\some\double\slashes\dir\foo.c:4
-      No source file named c:\some\double\slashes\dir\foo.c:4.
-   To prevent this from happening, we need this function to always
-   strip those extra backslashes.  While canonicalize_file_name does
-   perform this simplification, it only works when the path is valid.
-   Since the simplification would be useful even if the path is not
-   valid (one can always set a breakpoint on a file, even if the file
-   does not exist locally), we rely instead on GetFullPathName to
-   perform the canonicalization.  */
-
-#if defined (_WIN32)
-  {
-    char buf[MAX_PATH];
-    DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
-
-    /* The file system is case-insensitive but case-preserving.
-       So it is important we do not lowercase the path.  Otherwise,
-       we might not be able to display the original casing in a given
-       path.  */
-    if (len > 0 && len < MAX_PATH)
-      return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
-  }
-#else
-  {
-    char *rp = canonicalize_file_name (filename);
-
-    if (rp != NULL)
-      return gdb::unique_xmalloc_ptr<char> (rp);
-  }
-#endif
-
-  /* This system is a lost cause, just dup the buffer.  */
-  return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
-}
-
 #if GDB_SELF_TEST
 
 static void
@@ -2925,74 +2875,6 @@ gdb_realpath_tests ()
 
 #endif /* GDB_SELF_TEST */
 
-/* Return a copy of FILENAME, with its directory prefix canonicalized
-   by gdb_realpath.  */
-
-gdb::unique_xmalloc_ptr<char>
-gdb_realpath_keepfile (const char *filename)
-{
-  const char *base_name = lbasename (filename);
-  char *dir_name;
-  char *result;
-
-  /* Extract the basename of filename, and return immediately 
-     a copy of filename if it does not contain any directory prefix.  */
-  if (base_name == filename)
-    return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
-
-  dir_name = (char *) alloca ((size_t) (base_name - filename + 2));
-  /* Allocate enough space to store the dir_name + plus one extra
-     character sometimes needed under Windows (see below), and
-     then the closing \000 character.  */
-  strncpy (dir_name, filename, base_name - filename);
-  dir_name[base_name - filename] = '\000';
-
-#ifdef HAVE_DOS_BASED_FILE_SYSTEM
-  /* We need to be careful when filename is of the form 'd:foo', which
-     is equivalent of d:./foo, which is totally different from d:/foo.  */
-  if (strlen (dir_name) == 2 && isalpha (dir_name[0]) && dir_name[1] == ':')
-    {
-      dir_name[2] = '.';
-      dir_name[3] = '\000';
-    }
-#endif
-
-  /* Canonicalize the directory prefix, and build the resulting
-     filename.  If the dirname realpath already contains an ending
-     directory separator, avoid doubling it.  */
-  gdb::unique_xmalloc_ptr<char> path_storage = gdb_realpath (dir_name);
-  const char *real_path = path_storage.get ();
-  if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
-    result = concat (real_path, base_name, (char *) NULL);
-  else
-    result = concat (real_path, SLASH_STRING, base_name, (char *) NULL);
-
-  return gdb::unique_xmalloc_ptr<char> (result);
-}
-
-/* Return PATH in absolute form, performing tilde-expansion if necessary.
-   PATH cannot be NULL or the empty string.
-   This does not resolve symlinks however, use gdb_realpath for that.  */
-
-gdb::unique_xmalloc_ptr<char>
-gdb_abspath (const char *path)
-{
-  gdb_assert (path != NULL && path[0] != '\0');
-
-  if (path[0] == '~')
-    return gdb::unique_xmalloc_ptr<char> (tilde_expand (path));
-
-  if (IS_ABSOLUTE_PATH (path))
-    return gdb::unique_xmalloc_ptr<char> (xstrdup (path));
-
-  /* Beware the // my son, the Emacs barfs, the botch that catch...  */
-  return gdb::unique_xmalloc_ptr<char>
-    (concat (current_directory,
-	     IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
-	     ? "" : SLASH_STRING,
-	     path, (char *) NULL));
-}
-
 ULONGEST
 align_up (ULONGEST v, int n)
 {
diff --git a/gdb/utils.h b/gdb/utils.h
index b234762929..8ca3eb0369 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -295,12 +295,6 @@ extern struct cleanup *make_bpstat_clear_actions_cleanup (void);
 \f
 /* Path utilities.  */
 
-extern gdb::unique_xmalloc_ptr<char> gdb_realpath (const char *);
-
-extern gdb::unique_xmalloc_ptr<char> gdb_realpath_keepfile (const char *);
-
-extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *);
-
 extern int gdb_filename_fnmatch (const char *pattern, const char *string,
 				 int flags);
 
-- 
2.14.3

[PATCH 0/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

Second version.

These two patches fix the issue pointed by Simon on IRC, that
gdbserver doesn't recognize filename-only binaries anymore:

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

The first one is a preparation patch (and can go in independently),
which moves path-manipulation functions from utils.c to a new
common/pathstuff.c, making them available for gdbserver as well.

The second patch is the fix itself.

More details in each message.

Re: [PATCH v2 2/2] Make gdbserver work with filename-only binaries

[Simon Marchi]

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

Re: [PATCH 1/2] Create new common/pathstuff.[ch]

[Joel Brobecker]

Hi Sergio,


On Fri, Feb 09, 2018 at 08:42:40PM -0500, Sergio Durigan Junior wrote:
> This commit moves the path manipulation routines found on utils.c to a
> new common/pathstuff.c, and updates the Makefile.in's accordingly.
> The routines moved are "gdb_realpath", "gdb_realpath_keepfile" and
> "gdb_abspath".
> 
> This will be needed because gdbserver will have to call "gdb_abspath"
> on my next patch, which implements a way to expand the path of the
> inferior provided by the user in order to allow specifying just the
> binary name when starting gdbserver, like:
> 
>   $ gdbserver :1234 a.out
> 
> With the recent addition of the startup-with-shell feature on
> gdbserver, this scenario doesn't work anymore if the user doesn't have
> the current directory listed in the PATH variable.
> 
> I had to do a minor adjustment on "gdb_abspath" because we don't have
> access to "tilde_expand" on gdbserver, so now the function is using
> "gdb_tilde_expand" instead.  Otherwise, the code is the same.
> 
> Regression tested on the BuildBot, without regressions.
> 
> gdb/ChangeLog:
> 2018-02-09  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* Makefile.in (SFILES): Add "common/pathstuff.c".
> 	(HFILES_NO_SRCDIR): Add "common/pathstuff.h".
> 	(COMMON_OBS): Add "pathstuff.o".
> 	* common/pathstuff.c: New file.
> 	* common/pathstuff.h: New file.
> 	* utils.c (gdb_realpath): Move to "common/pathstuff.c".
> 	(gdb_realpath_keepfile): Likewise.
> 	(gdb_abspath): Likewise.
> 	* utils.h: Include "common/pathstuff.h".
> 	(gdb_realpath): Move to "common/pathstuff.h".
> 	(gdb_realpath_keepfile): Likewise.
> 	(gdb_abspath): Likewise.
> 
> gdb/gdbserver/ChangeLog:
> 2018-02-09  Sergio Durigan Junior  <sergiodj@redhat.com>

Thanks for doing this work!

> 	* Makefile.in (SFILES): Add "$(srcdir)/common/pathstuff.c".
> 	(OBJS): Add "pathstuff.o".
> +++ b/gdb/common/pathstuff.c
> @@ -0,0 +1,144 @@
[...]
> +gdb::unique_xmalloc_ptr<char>
> +gdb_realpath (const char *filename)

I realize you are just moving the function, but the function's missing
some documentation. I think it would be useful to add.

What lead me to this is the fact that there is one particularly
important element, is that this function used the current working
directory to expand relative paths. In the case of GDB, I verified
that when the user does a "cd DIR", GDB updates both current_directory
and its actual current working directory (in other words, we always
maintain the property current_directory == getcwd ().

In GDBserver, however, it doesn't seem to be the case. So I think
we need to be explicit about that, because calls to gdb_realpath
and gdb_abspath with the same filename  might actually return
the path to two different files if the conditions are right!

Ideally, I think we would want gdb_realpath and gdb_abspath to
return the same value. But, if we are interested, I suggest we
discuss that separately from this thread. This is potentially
disruptive (and potentially in a good way ;-)).

> +/* Return PATH in absolute form, performing tilde-expansion if necessary.
> +   PATH cannot be NULL or the empty string.
> +   This does not resolve symlinks however, use gdb_realpath for that.  */
> +
> +extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);

Similar to the above, I think we should be clear that the expansion
of relative path is done relative to the current_directory (or
the current working directory if NULL). Something like that.


-- 
Joel

Re: [PATCH 2/2] Make gdbserver work with filename-only binaries

[Joel Brobecker]

> Ah, I guess I didn't consider this (obvious) scenario for gdbserver.  I
> was thinking that gdbserver (before the startup-with-shell feature)
> would not work with binaries in PATH...

It would, but it would be at the discretion of each target
implementation. linux-low was doing it by calling execv first,
which doesn't search the PATH, and then calling execvp if
the first execv call failed with a "not found" error.

> > I didn't think this through completely, but maybe we could do
> > something simpler, if the program_name doesn't contain a directory
> > separator and the file exists in the current working directory, we
> > add "./" in front of it when passing it to the shell?  I think all
> > three use cases would work:
> >
> > - gdbserver :1234 foo (foo in current directory)
> > - gdbserver :1234 foo (foo in PATH)
> > - gdbserver :1234 ./foo
> 
> So, what do you think of checking if the file exists in the CWD (and is
> executable), and prefixing it with current_directory, as I'm doing with
> this patch?  I personally prefer to be more verbose, so using the full
> path is better IMHO than just adding "./".

I agree with Simon that we shouldn't (and don't really need to) touch
the file when it contains some directory information in it. As far as
I can tell, it already works as is, both with and without shell startup.

    $ gdbserver --once :4444 simple/simple_main
    Process simple/simple_main created; pid = 15400
    Listening on port 4444

    $ gdbserver --no-startup-with-shell --once :4444 simple/simple_main
    Process simple/simple_main created; pid = 15432
    Listening on port 4444

So, I think the only case that we need to worry about is the case
where exec_file is a basename. For that, I think the following test
should be sufficient:

    if (lbasename (exec_file) == exec_file)

... If it returns anything else than exec file, it's either a full
path, or a relative path with some directory information in it.

I agree with you, Sergio, that I think that expanding to the absolute
path would be cleaner. I'm not sure whether adding "." + SLASH_STRING
is going to work everywhere... I'd rather not ask myself the question,
and just expand to a full path, since you've done the work to do it
anyways.

-- 
Joel

Re: [PATCH v2 2/2] Make gdbserver work with filename-only binaries

[Pedro Alves]

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

Re: [PATCH v2 2/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

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/

Re: [PATCH 1/2] Create new common/pathstuff.[ch]

[Sergio Durigan Junior]

On Wednesday, February 21 2018, Joel Brobecker wrote:

> Hi Sergio,

Hi Joel,

>
> On Fri, Feb 09, 2018 at 08:42:40PM -0500, Sergio Durigan Junior wrote:
>> This commit moves the path manipulation routines found on utils.c to a
>> new common/pathstuff.c, and updates the Makefile.in's accordingly.
>> The routines moved are "gdb_realpath", "gdb_realpath_keepfile" and
>> "gdb_abspath".
>> 
>> This will be needed because gdbserver will have to call "gdb_abspath"
>> on my next patch, which implements a way to expand the path of the
>> inferior provided by the user in order to allow specifying just the
>> binary name when starting gdbserver, like:
>> 
>>   $ gdbserver :1234 a.out
>> 
>> With the recent addition of the startup-with-shell feature on
>> gdbserver, this scenario doesn't work anymore if the user doesn't have
>> the current directory listed in the PATH variable.
>> 
>> I had to do a minor adjustment on "gdb_abspath" because we don't have
>> access to "tilde_expand" on gdbserver, so now the function is using
>> "gdb_tilde_expand" instead.  Otherwise, the code is the same.
>> 
>> Regression tested on the BuildBot, without regressions.
>> 
>> gdb/ChangeLog:
>> 2018-02-09  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	* Makefile.in (SFILES): Add "common/pathstuff.c".
>> 	(HFILES_NO_SRCDIR): Add "common/pathstuff.h".
>> 	(COMMON_OBS): Add "pathstuff.o".
>> 	* common/pathstuff.c: New file.
>> 	* common/pathstuff.h: New file.
>> 	* utils.c (gdb_realpath): Move to "common/pathstuff.c".
>> 	(gdb_realpath_keepfile): Likewise.
>> 	(gdb_abspath): Likewise.
>> 	* utils.h: Include "common/pathstuff.h".
>> 	(gdb_realpath): Move to "common/pathstuff.h".
>> 	(gdb_realpath_keepfile): Likewise.
>> 	(gdb_abspath): Likewise.
>> 
>> gdb/gdbserver/ChangeLog:
>> 2018-02-09  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> Thanks for doing this work!

No problem.

>> 	* Makefile.in (SFILES): Add "$(srcdir)/common/pathstuff.c".
>> 	(OBJS): Add "pathstuff.o".
>> +++ b/gdb/common/pathstuff.c
>> @@ -0,0 +1,144 @@
> [...]
>> +gdb::unique_xmalloc_ptr<char>
>> +gdb_realpath (const char *filename)
>
> I realize you are just moving the function, but the function's missing
> some documentation. I think it would be useful to add.
>
> What lead me to this is the fact that there is one particularly
> important element, is that this function used the current working
> directory to expand relative paths. In the case of GDB, I verified
> that when the user does a "cd DIR", GDB updates both current_directory
> and its actual current working directory (in other words, we always
> maintain the property current_directory == getcwd ().
>
> In GDBserver, however, it doesn't seem to be the case. So I think
> we need to be explicit about that, because calls to gdb_realpath
> and gdb_abspath with the same filename  might actually return
> the path to two different files if the conditions are right!

Exactly.  GDB has different concepts for "inferior CWD" and "GDB CWD".
gdbserver, after my patch which implemented the inferior CWD handling,
just cares about the "inferior CWD".  But now, with the current patch
we're discussing, gdbserver will have a "current_directory" variable and
will be more aware of its own CWD.

> Ideally, I think we would want gdb_realpath and gdb_abspath to
> return the same value. But, if we are interested, I suggest we
> discuss that separately from this thread. This is potentially
> disruptive (and potentially in a good way ;-)).

Yes, that's a good point.  I will include a comment about this.

>> +/* Return PATH in absolute form, performing tilde-expansion if necessary.
>> +   PATH cannot be NULL or the empty string.
>> +   This does not resolve symlinks however, use gdb_realpath for that.  */
>> +
>> +extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);
>
> Similar to the above, I think we should be clear that the expansion
> of relative path is done relative to the current_directory (or
> the current working directory if NULL). Something like that.

Consider it done.

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/

Re: [PATCH v2 2/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

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/

[PATCH v3 1/2] Create new common/pathstuff.[ch]

[Sergio Durigan Junior]

Changes from v2:

- Added comments to the path manipulation functions.


This commit moves the path manipulation routines found on utils.c to a
new common/pathstuff.c, and updates the Makefile.in's accordingly.
The routines moved are "gdb_realpath", "gdb_realpath_keepfile" and
"gdb_abspath".

This will be needed because gdbserver will have to call "gdb_abspath"
on my next patch, which implements a way to expand the path of the
inferior provided by the user in order to allow specifying just the
binary name when starting gdbserver, like:

  $ gdbserver :1234 a.out

With the recent addition of the startup-with-shell feature on
gdbserver, this scenario doesn't work anymore if the user doesn't have
the current directory listed in the PATH variable.

I had to do a minor adjustment on "gdb_abspath" because we don't have
access to "tilde_expand" on gdbserver, so now the function is using
"gdb_tilde_expand" instead.  Otherwise, the code is the same.

Regression tested on the BuildBot, without regressions.

gdb/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* Makefile.in (COMMON_SFILES): Add "common/pathstuff.c".
	(HFILES_NO_SRCDIR): Add "common/pathstuff.h".
	* auto-load.c: Include "common/pathstuff.h".
	* common/common-def.h (current_directory): Move here.
	* common/gdb_tilde_expand.c (gdb_tilde_expand_up): New
	function.
	* common/gdb_tilde_expand.h (gdb_tilde_expand_up): New
	prototype.
	* common/pathstuff.c: New file.
	* common/pathstuff.h: New file.
	* compile/compile.c: Include "common/pathstuff.h".
	* defs.h (current_directory): Move to "common/common-defs.h".
	* dwarf2read.c: Include "common/pathstuff.h".
	* exec.c: Likewise.
	* guile/scm-safe-call.c: Likewise.
	* linux-thread-db.c: Likewise.
	* main.c: Likewise.
	* nto-tdep.c: Likewise.
	* objfiles.c: Likewise.
	* source.c: Likewise.
	* symtab.c: Likewise.
	* utils.c: Include "common/pathstuff.h".
	(gdb_realpath): Move to "common/pathstuff.c".
	(gdb_realpath_keepfile): Likewise.
	(gdb_abspath): Likewise.
	* utils.h (gdb_realpath): Move to "common/pathstuff.h".
	(gdb_realpath_keepfile): Likewise.
	(gdb_abspath): Likewise.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* Makefile.in (SFILES): Add "$(srcdir)/common/pathstuff.c".
	(OBJS): Add "pathstuff.o".
	* server.c (current_directory): New global variable.
	(captured_main): Initialize "current_directory".
---
 gdb/Makefile.in               |   2 +
 gdb/auto-load.c               |   1 +
 gdb/common/common-defs.h      |   3 +
 gdb/common/gdb_tilde_expand.c |  13 ++++
 gdb/common/gdb_tilde_expand.h |   4 ++
 gdb/common/pathstuff.c        | 142 ++++++++++++++++++++++++++++++++++++++++++
 gdb/common/pathstuff.h        |  49 +++++++++++++++
 gdb/compile/compile.c         |   1 +
 gdb/defs.h                    |   4 --
 gdb/dwarf2read.c              |   1 +
 gdb/exec.c                    |   1 +
 gdb/gdbserver/Makefile.in     |   2 +
 gdb/gdbserver/server.c        |  11 ++++
 gdb/guile/scm-safe-call.c     |   1 +
 gdb/linux-thread-db.c         |   1 +
 gdb/main.c                    |   1 +
 gdb/nto-tdep.c                |   1 +
 gdb/objfiles.c                |   1 +
 gdb/source.c                  |   1 +
 gdb/symtab.c                  |   1 +
 gdb/utils.c                   | 120 +----------------------------------
 gdb/utils.h                   |   6 --
 22 files changed, 238 insertions(+), 129 deletions(-)
 create mode 100644 gdb/common/pathstuff.c
 create mode 100644 gdb/common/pathstuff.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 1c58b9270d..19be64f226 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -955,6 +955,7 @@ COMMON_SFILES = \
 	common/gdb_tilde_expand.c \
 	common/gdb_vecs.c \
 	common/new-op.c \
+	common/pathstuff.c \
 	common/print-utils.c \
 	common/ptid.c \
 	common/rsp-low.c \
@@ -1429,6 +1430,7 @@ HFILES_NO_SRCDIR = \
 	common/gdb_wait.h \
 	common/common-inferior.h \
 	common/host-defs.h \
+	common/pathstuff.h \
 	common/print-utils.h \
 	common/ptid.h \
 	common/queue.h \
diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index b79341faf6..9dd8754e1a 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -40,6 +40,7 @@
 #include "extension.h"
 #include "gdb/section-scripts.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 /* The section to look in for auto-loaded scripts (in file formats that
    support sections).
diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
index acbc32ca69..881a4eaaff 100644
--- a/gdb/common/common-defs.h
+++ b/gdb/common/common-defs.h
@@ -91,4 +91,7 @@
 /* Pull in gdb::unique_xmalloc_ptr.  */
 #include "common/gdb_unique_ptr.h"
 
+/* String containing the current directory (what getwd would return).  */
+extern char *current_directory;
+
 #endif /* COMMON_DEFS_H */
diff --git a/gdb/common/gdb_tilde_expand.c b/gdb/common/gdb_tilde_expand.c
index b4f371464d..fcb97961ac 100644
--- a/gdb/common/gdb_tilde_expand.c
+++ b/gdb/common/gdb_tilde_expand.c
@@ -80,3 +80,16 @@ gdb_tilde_expand (const char *dir)
 
   return expanded_dir;
 }
+
+/* See common/gdb_tilde_expand.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_tilde_expand_up (const char *dir)
+{
+  gdb_glob glob (dir, GLOB_TILDE_CHECK, NULL);
+
+  gdb_assert (glob.pathc () > 0);
+  /* "glob" may return more than one match to the path provided by the
+     user, but we are only interested in the first match.  */
+  return gdb::unique_xmalloc_ptr<char> (xstrdup (glob.pathv ()[0]));
+}
diff --git a/gdb/common/gdb_tilde_expand.h b/gdb/common/gdb_tilde_expand.h
index d0dfb37857..22860d3969 100644
--- a/gdb/common/gdb_tilde_expand.h
+++ b/gdb/common/gdb_tilde_expand.h
@@ -24,4 +24,8 @@
    the full path.  */
 extern std::string gdb_tilde_expand (const char *dir);
 
+/* Same as GDB_TILDE_EXPAND, but return the full path as a
+   gdb::unique_xmalloc_ptr<char>.  */
+extern gdb::unique_xmalloc_ptr<char> gdb_tilde_expand_up (const char *dir);
+
 #endif /* ! GDB_TILDE_EXPAND_H */
diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
new file mode 100644
index 0000000000..02f6e44794
--- /dev/null
+++ b/gdb/common/pathstuff.c
@@ -0,0 +1,142 @@
+/* Path manipulation routines for GDB and gdbserver.
+
+   Copyright (C) 1986-2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#include "common-defs.h"
+#include "pathstuff.h"
+#include "host-defs.h"
+#include "filenames.h"
+#include "gdb_tilde_expand.h"
+
+/* See common/pathstuff.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_realpath (const char *filename)
+{
+/* On most hosts, we rely on canonicalize_file_name to compute
+   the FILENAME's realpath.
+
+   But the situation is slightly more complex on Windows, due to some
+   versions of GCC which were reported to generate paths where
+   backlashes (the directory separator) were doubled.  For instance:
+      c:\\some\\double\\slashes\\dir
+   ... instead of ...
+      c:\some\double\slashes\dir
+   Those double-slashes were getting in the way when comparing paths,
+   for instance when trying to insert a breakpoint as follow:
+      (gdb) b c:/some/double/slashes/dir/foo.c:4
+      No source file named c:/some/double/slashes/dir/foo.c:4.
+      (gdb) b c:\some\double\slashes\dir\foo.c:4
+      No source file named c:\some\double\slashes\dir\foo.c:4.
+   To prevent this from happening, we need this function to always
+   strip those extra backslashes.  While canonicalize_file_name does
+   perform this simplification, it only works when the path is valid.
+   Since the simplification would be useful even if the path is not
+   valid (one can always set a breakpoint on a file, even if the file
+   does not exist locally), we rely instead on GetFullPathName to
+   perform the canonicalization.  */
+
+#if defined (_WIN32)
+  {
+    char buf[MAX_PATH];
+    DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
+
+    /* The file system is case-insensitive but case-preserving.
+       So it is important we do not lowercase the path.  Otherwise,
+       we might not be able to display the original casing in a given
+       path.  */
+    if (len > 0 && len < MAX_PATH)
+      return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
+  }
+#else
+  {
+    char *rp = canonicalize_file_name (filename);
+
+    if (rp != NULL)
+      return gdb::unique_xmalloc_ptr<char> (rp);
+  }
+#endif
+
+  /* This system is a lost cause, just dup the buffer.  */
+  return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
+}
+
+/* See common/pathstuff.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_realpath_keepfile (const char *filename)
+{
+  const char *base_name = lbasename (filename);
+  char *dir_name;
+  char *result;
+
+  /* Extract the basename of filename, and return immediately
+     a copy of filename if it does not contain any directory prefix.  */
+  if (base_name == filename)
+    return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
+
+  dir_name = (char *) alloca ((size_t) (base_name - filename + 2));
+  /* Allocate enough space to store the dir_name + plus one extra
+     character sometimes needed under Windows (see below), and
+     then the closing \000 character.  */
+  strncpy (dir_name, filename, base_name - filename);
+  dir_name[base_name - filename] = '\000';
+
+#ifdef HAVE_DOS_BASED_FILE_SYSTEM
+  /* We need to be careful when filename is of the form 'd:foo', which
+     is equivalent of d:./foo, which is totally different from d:/foo.  */
+  if (strlen (dir_name) == 2 && isalpha (dir_name[0]) && dir_name[1] == ':')
+    {
+      dir_name[2] = '.';
+      dir_name[3] = '\000';
+    }
+#endif
+
+  /* Canonicalize the directory prefix, and build the resulting
+     filename.  If the dirname realpath already contains an ending
+     directory separator, avoid doubling it.  */
+  gdb::unique_xmalloc_ptr<char> path_storage = gdb_realpath (dir_name);
+  const char *real_path = path_storage.get ();
+  if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
+    result = concat (real_path, base_name, (char *) NULL);
+  else
+    result = concat (real_path, SLASH_STRING, base_name, (char *) NULL);
+
+  return gdb::unique_xmalloc_ptr<char> (result);
+}
+
+/* See common/pathstuff.h.  */
+
+gdb::unique_xmalloc_ptr<char>
+gdb_abspath (const char *path)
+{
+  gdb_assert (path != NULL && path[0] != '\0');
+
+  if (path[0] == '~')
+    return gdb_tilde_expand_up (path);
+
+  if (IS_ABSOLUTE_PATH (path))
+    return gdb::unique_xmalloc_ptr<char> (xstrdup (path));
+
+  /* Beware the // my son, the Emacs barfs, the botch that catch...  */
+  return gdb::unique_xmalloc_ptr<char>
+    (concat (current_directory,
+	     IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
+	     ? "" : SLASH_STRING,
+	     path, (char *) NULL));
+}
diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h
new file mode 100644
index 0000000000..3cb02c86d6
--- /dev/null
+++ b/gdb/common/pathstuff.h
@@ -0,0 +1,49 @@
+/* Path manipulation routines for GDB and gdbserver.
+
+   Copyright (C) 1986-2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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/>.  */
+
+#ifndef PATHSTUFF_H
+#define PATHSTUFF_H
+
+/* Path utilities.  */
+
+/* Return the real path of FILENAME, expanding all the symbolic links.
+
+   Contrary to "gdb_abspath", this function does not use
+   CURRENT_DIRECTORY for path expansion.  Instead, it relies on the
+   current working directory (CWD) of GDB or gdbserver.  */
+
+extern gdb::unique_xmalloc_ptr<char> gdb_realpath (const char *filename);
+
+/* Return a copy of FILENAME, with its directory prefix canonicalized
+   by gdb_realpath.  */
+
+extern gdb::unique_xmalloc_ptr<char>
+  gdb_realpath_keepfile (const char *filename);
+
+/* Return PATH in absolute form, performing tilde-expansion if necessary.
+   PATH cannot be NULL or the empty string.
+   This does not resolve symlinks however, use gdb_realpath for that.
+
+   Contrary to "gdb_realpath", this function uses CURRENT_DIRECTORY
+   for the path expansion.  This may lead to scenarios the current
+   working directory (CWD) is different than CURRENT_DIRECTORY.  */
+
+extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);
+
+#endif /* PATHSTUFF_H */
diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
index 70c4570de7..7f35272872 100644
--- a/gdb/compile/compile.c
+++ b/gdb/compile/compile.c
@@ -41,6 +41,7 @@
 #include "valprint.h"
 #include "common/gdb_optional.h"
 #include "common/gdb_unlinker.h"
+#include "common/pathstuff.h"
 
 \f
 
diff --git a/gdb/defs.h b/gdb/defs.h
index c85bf2cf11..a924573b57 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -402,10 +402,6 @@ enum info_proc_what
     IP_ALL
   };
 
-/* * String containing the current directory (what getwd would return).  */
-
-extern char *current_directory;
-
 /* * Default radixes for input and output.  Only some values supported.  */
 extern unsigned input_radix;
 extern unsigned output_radix;
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 1e4376e4de..9825117fa7 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -87,6 +87,7 @@
 #include <set>
 #include <forward_list>
 #include "rust-lang.h"
+#include "common/pathstuff.h"
 
 /* When == 1, print basic high level tracing messages.
    When > 1, be more verbose.
diff --git a/gdb/exec.c b/gdb/exec.c
index 0b4237ff63..6b910e8f54 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -45,6 +45,7 @@
 #include <sys/stat.h>
 #include "solist.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 void (*deprecated_file_changed_hook) (const char *);
 
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index 2dbf9ae63d..f2936920fe 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -209,6 +209,7 @@ SFILES = \
 	$(srcdir)/common/gdb_tilde_expand.c \
 	$(srcdir)/common/gdb_vecs.c \
 	$(srcdir)/common/new-op.c \
+	$(srcdir)/common/pathstuff.c \
 	$(srcdir)/common/print-utils.c \
 	$(srcdir)/common/ptid.c \
 	$(srcdir)/common/rsp-low.c \
@@ -249,6 +250,7 @@ OBS = \
 	common/gdb_tilde_expand.o \
 	common/gdb_vecs.o \
 	common/new-op.o \
+	common/pathstuff.o \
 	common/print-utils.o \
 	common/ptid.o \
 	common/rsp-low.o \
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index cb02b58507..922d5269b3 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -56,6 +56,10 @@
       break;					\
     }
 
+/* String containing the current directory (what getwd would return).  */
+
+char *current_directory;
+
 /* The environment to pass to the inferior when creating it.  */
 
 static gdb_environ our_environ;
@@ -3539,6 +3543,13 @@ captured_main (int argc, char *argv[])
   const char *selftest_filter = NULL;
 #endif
 
+  current_directory = getcwd (NULL, 0);
+  if (current_directory == NULL)
+    {
+      error (_("%s: error finding working directory"),
+	     safe_strerror (errno));
+    }
+
   while (*next_arg != NULL && **next_arg == '-')
     {
       if (strcmp (*next_arg, "--version") == 0)
diff --git a/gdb/guile/scm-safe-call.c b/gdb/guile/scm-safe-call.c
index a9ce7b72c1..2cba399e23 100644
--- a/gdb/guile/scm-safe-call.c
+++ b/gdb/guile/scm-safe-call.c
@@ -23,6 +23,7 @@
 #include "defs.h"
 #include "filenames.h"
 #include "guile-internal.h"
+#include "common/pathstuff.h"
 
 /* Struct to marshall args to scscm_safe_call_body.  */
 
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 5ba4c03df6..10c85f56ad 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -46,6 +46,7 @@
 #include <ctype.h>
 #include "nat/linux-namespaces.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 /* GNU/Linux libthread_db support.
 
diff --git a/gdb/main.c b/gdb/main.c
index 3c98787edb..189266f90e 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -46,6 +46,7 @@
 #include "infrun.h"
 #include "signals-state-save-restore.h"
 #include <vector>
+#include "common/pathstuff.h"
 
 /* The selected interpreter.  This will be used as a set command
    variable, so it should always be malloc'ed - since
diff --git a/gdb/nto-tdep.c b/gdb/nto-tdep.c
index 8eb864b871..82a4fcbbb9 100644
--- a/gdb/nto-tdep.c
+++ b/gdb/nto-tdep.c
@@ -32,6 +32,7 @@
 #include "gdbcore.h"
 #include "objfiles.h"
 #include "source.h"
+#include "common/pathstuff.h"
 
 #define QNX_NOTE_NAME	"QNX"
 #define QNX_INFO_SECT_NAME "QNX_info"
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 70e369b8b4..a9aaf89540 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -52,6 +52,7 @@
 #include "solist.h"
 #include "gdb_bfd.h"
 #include "btrace.h"
+#include "common/pathstuff.h"
 
 #include <vector>
 
diff --git a/gdb/source.c b/gdb/source.c
index 8a27b2e666..04ee3b33d2 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -44,6 +44,7 @@
 #include "readline/readline.h"
 #include "common/enum-flags.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 #define OPEN_MODE (O_RDONLY | O_BINARY)
 #define FDOPEN_MODE FOPEN_RB
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 0fd3f3a30f..567195304f 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -66,6 +66,7 @@
 #include "filename-seen-cache.h"
 #include "arch-utils.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 /* Forward declarations for local functions.  */
 
diff --git a/gdb/utils.c b/gdb/utils.c
index c531748fe4..577f9df4ec 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -70,6 +70,7 @@
 #include "common/gdb_optional.h"
 #include "cp-support.h"
 #include <algorithm>
+#include "common/pathstuff.h"
 
 #if !HAVE_DECL_MALLOC
 extern PTR malloc ();		/* ARI: PTR */
@@ -2838,57 +2839,6 @@ string_to_core_addr (const char *my_string)
   return addr;
 }
 
-gdb::unique_xmalloc_ptr<char>
-gdb_realpath (const char *filename)
-{
-/* On most hosts, we rely on canonicalize_file_name to compute
-   the FILENAME's realpath.
-
-   But the situation is slightly more complex on Windows, due to some
-   versions of GCC which were reported to generate paths where
-   backlashes (the directory separator) were doubled.  For instance:
-      c:\\some\\double\\slashes\\dir
-   ... instead of ...
-      c:\some\double\slashes\dir
-   Those double-slashes were getting in the way when comparing paths,
-   for instance when trying to insert a breakpoint as follow:
-      (gdb) b c:/some/double/slashes/dir/foo.c:4
-      No source file named c:/some/double/slashes/dir/foo.c:4.
-      (gdb) b c:\some\double\slashes\dir\foo.c:4
-      No source file named c:\some\double\slashes\dir\foo.c:4.
-   To prevent this from happening, we need this function to always
-   strip those extra backslashes.  While canonicalize_file_name does
-   perform this simplification, it only works when the path is valid.
-   Since the simplification would be useful even if the path is not
-   valid (one can always set a breakpoint on a file, even if the file
-   does not exist locally), we rely instead on GetFullPathName to
-   perform the canonicalization.  */
-
-#if defined (_WIN32)
-  {
-    char buf[MAX_PATH];
-    DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
-
-    /* The file system is case-insensitive but case-preserving.
-       So it is important we do not lowercase the path.  Otherwise,
-       we might not be able to display the original casing in a given
-       path.  */
-    if (len > 0 && len < MAX_PATH)
-      return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
-  }
-#else
-  {
-    char *rp = canonicalize_file_name (filename);
-
-    if (rp != NULL)
-      return gdb::unique_xmalloc_ptr<char> (rp);
-  }
-#endif
-
-  /* This system is a lost cause, just dup the buffer.  */
-  return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
-}
-
 #if GDB_SELF_TEST
 
 static void
@@ -2925,74 +2875,6 @@ gdb_realpath_tests ()
 
 #endif /* GDB_SELF_TEST */
 
-/* Return a copy of FILENAME, with its directory prefix canonicalized
-   by gdb_realpath.  */
-
-gdb::unique_xmalloc_ptr<char>
-gdb_realpath_keepfile (const char *filename)
-{
-  const char *base_name = lbasename (filename);
-  char *dir_name;
-  char *result;
-
-  /* Extract the basename of filename, and return immediately 
-     a copy of filename if it does not contain any directory prefix.  */
-  if (base_name == filename)
-    return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
-
-  dir_name = (char *) alloca ((size_t) (base_name - filename + 2));
-  /* Allocate enough space to store the dir_name + plus one extra
-     character sometimes needed under Windows (see below), and
-     then the closing \000 character.  */
-  strncpy (dir_name, filename, base_name - filename);
-  dir_name[base_name - filename] = '\000';
-
-#ifdef HAVE_DOS_BASED_FILE_SYSTEM
-  /* We need to be careful when filename is of the form 'd:foo', which
-     is equivalent of d:./foo, which is totally different from d:/foo.  */
-  if (strlen (dir_name) == 2 && isalpha (dir_name[0]) && dir_name[1] == ':')
-    {
-      dir_name[2] = '.';
-      dir_name[3] = '\000';
-    }
-#endif
-
-  /* Canonicalize the directory prefix, and build the resulting
-     filename.  If the dirname realpath already contains an ending
-     directory separator, avoid doubling it.  */
-  gdb::unique_xmalloc_ptr<char> path_storage = gdb_realpath (dir_name);
-  const char *real_path = path_storage.get ();
-  if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
-    result = concat (real_path, base_name, (char *) NULL);
-  else
-    result = concat (real_path, SLASH_STRING, base_name, (char *) NULL);
-
-  return gdb::unique_xmalloc_ptr<char> (result);
-}
-
-/* Return PATH in absolute form, performing tilde-expansion if necessary.
-   PATH cannot be NULL or the empty string.
-   This does not resolve symlinks however, use gdb_realpath for that.  */
-
-gdb::unique_xmalloc_ptr<char>
-gdb_abspath (const char *path)
-{
-  gdb_assert (path != NULL && path[0] != '\0');
-
-  if (path[0] == '~')
-    return gdb::unique_xmalloc_ptr<char> (tilde_expand (path));
-
-  if (IS_ABSOLUTE_PATH (path))
-    return gdb::unique_xmalloc_ptr<char> (xstrdup (path));
-
-  /* Beware the // my son, the Emacs barfs, the botch that catch...  */
-  return gdb::unique_xmalloc_ptr<char>
-    (concat (current_directory,
-	     IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
-	     ? "" : SLASH_STRING,
-	     path, (char *) NULL));
-}
-
 ULONGEST
 align_up (ULONGEST v, int n)
 {
diff --git a/gdb/utils.h b/gdb/utils.h
index b234762929..8ca3eb0369 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -295,12 +295,6 @@ extern struct cleanup *make_bpstat_clear_actions_cleanup (void);
 \f
 /* Path utilities.  */
 
-extern gdb::unique_xmalloc_ptr<char> gdb_realpath (const char *);
-
-extern gdb::unique_xmalloc_ptr<char> gdb_realpath_keepfile (const char *);
-
-extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *);
-
 extern int gdb_filename_fnmatch (const char *pattern, const char *string,
 				 int flags);
 
-- 
2.14.3

[PATCH v3 0/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

Third version.

These two patches fix the issue pointed by Simon on IRC, that
gdbserver doesn't recognize filename-only binaries anymore:

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

The first one is a preparation patch (and can go in independently),
which moves path-manipulation functions from utils.c to a new
common/pathstuff.c, making them available for gdbserver as well.

The second patch is the fix itself.

More details in each message.

[PATCH v3 2/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

Changes from v2:

- Updated commit log.

- s/True/true

- Implement Simon's idea to not modify the path is there's any path
  separator char.

- Implement Simon's idea to use a class for "program_path".

- s/warning/error when getcwd returns NULL.  Reorder error message.

- Don't restrict the test to native-gdbserver.

- New testsuite helper "with_cwd" which takes care of switching to a
  directory, executing commands, and restoring the original CWD.

- Don't use runto_main; instead, use gdb_breakpoint + continue.


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 including "." 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 verify if the program path provided by the
user (or by the remote protocol) contains a directory separator
character.  If it doesn't, it means we're dealing with a filename-only
binary, so we call "gdb_abspath" to properly expand it and transform
it into a full path.  Otherwise, we leave the program path untouched.
This mimicks the behaviour seen on GDB (look at "openp" and
"attach_inferior", for example).

I am also submitting a testcase which exercises the scenario described
above.  This test requires gdbserver to be executed in a different CWD
than the original, so I also created a helper function, "with_cwd" (on
testsuite/lib/gdb.exp), which takes care of cd'ing into and out of the
specified dir.

Built and regtested on BuildBot, without regressions.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Simon Marchi  <simon.marchi@polymtl.ca>

	* 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.
	* common/pathstuff.c (contains_dir_separator): New function.
	* common/pathstuff.h (contains_dir_separator): New prototype.
	* source.c: Don't include "sys/stat.h".
	(is_regular_file): Move to "common/common-utils.c".

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* server.c: Include "filenames.h" and "pathstuff.h".
	(program_name): Delete variable.
	(program_path): New anonymous class.
	(get_exec_wrapper): Use "program_path" instead of
	"program_name".
	(handle_v_run): Likewise.
	(captured_main): Likewise.
	(process_serial_event): Likewise.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.server/abspath.exp: New file.
	* lib/gdb.exp (with_cwd): New procedure.
---
 gdb/common/common-utils.c            | 32 ++++++++++++++++++++++
 gdb/common/common-utils.h            |  5 ++++
 gdb/common/pathstuff.c               | 14 ++++++++++
 gdb/common/pathstuff.h               |  4 +++
 gdb/gdbserver/server.c               | 53 +++++++++++++++++++++++++++---------
 gdb/source.c                         | 34 -----------------------
 gdb/testsuite/gdb.server/abspath.exp | 51 ++++++++++++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp            | 24 ++++++++++++++++
 8 files changed, 170 insertions(+), 47 deletions(-)
 create mode 100644 gdb/testsuite/gdb.server/abspath.exp

diff --git a/gdb/common/common-utils.c b/gdb/common/common-utils.c
index ae2dd9db2b..80de826ba7 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..5408c35469 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/common/pathstuff.c b/gdb/common/pathstuff.c
index 02f6e44794..fc574dc32e 100644
--- a/gdb/common/pathstuff.c
+++ b/gdb/common/pathstuff.c
@@ -140,3 +140,17 @@ gdb_abspath (const char *path)
 	     ? "" : SLASH_STRING,
 	     path, (char *) NULL));
 }
+
+/* See common/pathstuff.h.  */
+
+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 3cb02c86d6..9f261273e6 100644
--- a/gdb/common/pathstuff.h
+++ b/gdb/common/pathstuff.h
@@ -46,4 +46,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 922d5269b3..7745027628 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"
 
@@ -112,7 +114,35 @@ static int vCont_supported;
    space randomization feature before starting an inferior.  */
 int disable_randomization = 1;
 
-static char *program_name = NULL;
+static struct {
+  /* Set the PROGRAM_PATH.  Here we adjust the path of the provided
+     binary if needed.  */
+  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 ());
+      }
+  }
+
+  /* Return the PROGRAM_PATH.  */
+  char *get ()
+  { return m_path.get (); }
+
+private:
+  /* The program name, adjusted if needed.  */
+  gdb::unique_xmalloc_ptr<char> m_path;
+} program_path;
 static std::vector<char *> program_args;
 static std::string wrapper_argv;
 
@@ -269,10 +299,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.  */
@@ -3003,7 +3033,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);
@@ -3011,16 +3041,13 @@ handle_v_run (char *own_buf)
 	}
     }
   else
-    {
-      xfree (program_name);
-      program_name = new_program_name;
-    }
+    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)
     {
@@ -3765,13 +3792,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);
 
       /* 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
@@ -4288,9 +4315,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)
 	    {
-	      create_inferior (program_name, program_args);
+	      create_inferior (program_path.get (), program_args);
 
 	      if (last_status.kind == TARGET_WAITKIND_STOPPED)
 		{
diff --git a/gdb/source.c b/gdb/source.c
index 04ee3b33d2..6979fb2817 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)
 }
 \f
 
-/* 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..beb1d96209
--- /dev/null
+++ b/gdb/testsuite/gdb.server/abspath.exp
@@ -0,0 +1,51 @@
+# 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
+}
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+    return -1
+}
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+set target_exec [gdbserver_download_current_prog]
+
+# Switch to where $target_exec lives, and execute gdbserver from
+# there.
+with_cwd "[file dirname $target_exec]" {
+    set target_execname [file tail $target_exec]
+    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
+
+    gdb_breakpoint main
+    gdb_test "continue" "Breakpoint $decimal.* main.*" "continue to main"
+}
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 3cd10dcafb..97b519aed7 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2054,6 +2054,30 @@ proc save_vars { vars body } {
     }
 }
 
+# Run tests in BODY with the current working directory (CWD) set to
+# DIR.  When BODY is finished, restore the original CWD.  Return the
+# result of BODY.
+#
+# This procedure doesn't check if DIR is a valid directory, so you
+# have to make sure of that.
+
+proc with_cwd { dir body } {
+    set saved_dir [pwd]
+    verbose -log "Switching to directory $dir (saved CWD: $saved_dir)."
+    cd $dir
+
+    set code [catch {uplevel 1 $body} result]
+
+    verbose -log "Switching back to $saved_dir."
+    cd $saved_dir
+
+    if {$code == 1} {
+	global errorInfo errorCode
+	return -code $code -errorinfo $errorInfo -errorcode $errorCode $result
+    } else {
+	return -code $code $result
+    }
+}
 
 # Run tests in BODY with GDB prompt and variable $gdb_prompt set to
 # PROMPT.  When BODY is finished, restore GDB prompt and variable
-- 
2.14.3

Re: [PATCH v2 2/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

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/

Re: [PATCH v3 2/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

On Tuesday, February 27 2018, I wrote:

> 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 including "." 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 verify if the program path provided by the
> user (or by the remote protocol) contains a directory separator
> character.  If it doesn't, it means we're dealing with a filename-only
> binary, so we call "gdb_abspath" to properly expand it and transform
> it into a full path.  Otherwise, we leave the program path untouched.
> This mimicks the behaviour seen on GDB (look at "openp" and
> "attach_inferior", for example).
>
> I am also submitting a testcase which exercises the scenario described
> above.  This test requires gdbserver to be executed in a different CWD
> than the original, so I also created a helper function, "with_cwd" (on
> testsuite/lib/gdb.exp), which takes care of cd'ing into and out of the
> specified dir.

This part is still giving me a few headaches.  I've just noticed that
two builders reported the new test as FAIL.  When I run it here, I can't
reproduce it, which makes me wonder that it's racy.  I don't know if the
culprit is the new "with_cwd" logic or not.  Curiously, both builders
reporting the failure are running on s390x
(Debian-s390x-native-extended-gdbserver-m64 and RHEL-s390x-m64).

Here's an excerpt of report from one of them,
Debian-s390x-native-extended-gdbserver-m64:

----------------
(gdb) file /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath/abspath
Reading symbols from /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath/abspath...done.
(gdb) set remote exec-file /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath/abspath
(gdb) set remote exec-file /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath/abspath
(gdb) disconnect
Ending remote debugging.
(gdb) PASS: gdb.server/abspath.exp: disconnect
Switching to directory /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath (saved CWD: /home/dje/debian-jessie-s390x-1/debian
-s390x-native-extended-gdbserver/build/gdb/testsuite).
spawn /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/../gdbserver/gdbserver --once :2347 abspath
Can't bind address: Address already in use.
Exiting
Port 2347 is already in use.
spawn /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/../gdbserver/gdbserver --once :2348 abspath
Process /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath/abspath created; pid = 21406
Listening on port 2348
target extended-remote localhost:2348
Remote debugging using localhost:2348
Reading /lib/ld64.so.1 from remote target...
warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
Reading /lib/ld64.so.1 from remote target...
Reading symbols from target:/lib/ld64.so.1...Reading /lib/ld-2.19.so from remote target...
Reading /lib/.debug/ld-2.19.so from remote target...
(no debugging symbols found)...done.
0x000003fffdfd9280 in ?? () from target:/lib/ld64.so.1
Protocol error: qXfer:btrace-conf (read-btrace-conf) conflicting enabled responses.
(gdb) break main
Breakpoint 1 at 0x800005bc: file /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.server/normal.c, line 23.
(gdb) continue
The program is not being run.
(gdb) FAIL: gdb.server/abspath.exp: continue to main (the program is no longer running)
Switching back to /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite.
----------------

They're both running the testsuite in parallel mode (-j8), but when I
run it locally in parallel I can't reproduce (even when I run in a
loop).

Anyway, I'll try to find out why this happens.  I don't see how changing
the CWD in a test could impact its results, so I'm guessing there's
something else at play here.

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/

Re: [PATCH v3 1/2] Create new common/pathstuff.[ch]

[Simon Marchi]

On 2018-02-27 10:27 PM, Sergio Durigan Junior wrote:
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index cb02b58507..922d5269b3 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -56,6 +56,10 @@
>        break;					\
>      }
>  
> +/* String containing the current directory (what getwd would return).  */
> +
> +char *current_directory;
> +
>  /* The environment to pass to the inferior when creating it.  */
>  
>  static gdb_environ our_environ;
> @@ -3539,6 +3543,13 @@ captured_main (int argc, char *argv[])
>    const char *selftest_filter = NULL;
>  #endif
>  
> +  current_directory = getcwd (NULL, 0);
> +  if (current_directory == NULL)
> +    {
> +      error (_("%s: error finding working directory"),
> +	     safe_strerror (errno));
> +    }

Just one thing, I think Pedro suggested to put the variable string at the end:

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

Simon

Re: [PATCH v3 2/2] Make gdbserver work with filename-only binaries

[Simon Marchi]

On 2018-02-27 10:58 PM, Sergio Durigan Junior wrote:
> On Tuesday, February 27 2018, I wrote:
> 
>> 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 including "." 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 verify if the program path provided by the
>> user (or by the remote protocol) contains a directory separator
>> character.  If it doesn't, it means we're dealing with a filename-only
>> binary, so we call "gdb_abspath" to properly expand it and transform
>> it into a full path.  Otherwise, we leave the program path untouched.
>> This mimicks the behaviour seen on GDB (look at "openp" and
>> "attach_inferior", for example).
>>
>> I am also submitting a testcase which exercises the scenario described
>> above.  This test requires gdbserver to be executed in a different CWD
>> than the original, so I also created a helper function, "with_cwd" (on
>> testsuite/lib/gdb.exp), which takes care of cd'ing into and out of the
>> specified dir.
> 
> This part is still giving me a few headaches.  I've just noticed that
> two builders reported the new test as FAIL.  When I run it here, I can't
> reproduce it, which makes me wonder that it's racy.  I don't know if the
> culprit is the new "with_cwd" logic or not.  Curiously, both builders
> reporting the failure are running on s390x
> (Debian-s390x-native-extended-gdbserver-m64 and RHEL-s390x-m64).
> 
> Here's an excerpt of report from one of them,
> Debian-s390x-native-extended-gdbserver-m64:
> 
> ----------------
> (gdb) file /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath/abspath
> Reading symbols from /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath/abspath...done.
> (gdb) set remote exec-file /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath/abspath
> (gdb) set remote exec-file /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath/abspath
> (gdb) disconnect
> Ending remote debugging.
> (gdb) PASS: gdb.server/abspath.exp: disconnect
> Switching to directory /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath (saved CWD: /home/dje/debian-jessie-s390x-1/debian
> -s390x-native-extended-gdbserver/build/gdb/testsuite).
> spawn /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/../gdbserver/gdbserver --once :2347 abspath
> Can't bind address: Address already in use.
> Exiting
> Port 2347 is already in use.
> spawn /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/../gdbserver/gdbserver --once :2348 abspath
> Process /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath/abspath created; pid = 21406
> Listening on port 2348
> target extended-remote localhost:2348
> Remote debugging using localhost:2348
> Reading /lib/ld64.so.1 from remote target...
> warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
> Reading /lib/ld64.so.1 from remote target...
> Reading symbols from target:/lib/ld64.so.1...Reading /lib/ld-2.19.so from remote target...
> Reading /lib/.debug/ld-2.19.so from remote target...
> (no debugging symbols found)...done.
> 0x000003fffdfd9280 in ?? () from target:/lib/ld64.so.1
> Protocol error: qXfer:btrace-conf (read-btrace-conf) conflicting enabled responses.
> (gdb) break main
> Breakpoint 1 at 0x800005bc: file /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.server/normal.c, line 23.
> (gdb) continue
> The program is not being run.
> (gdb) FAIL: gdb.server/abspath.exp: continue to main (the program is no longer running)
> Switching back to /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver/build/gdb/testsuite.
> ----------------
> 
> They're both running the testsuite in parallel mode (-j8), but when I
> run it locally in parallel I can't reproduce (even when I run in a
> loop).
> 
> Anyway, I'll try to find out why this happens.  I don't see how changing
> the CWD in a test could impact its results, so I'm guessing there's
> something else at play here.
> 
> Thanks,
> 

Maybe it's just because of this issue?  Note the btrace-related
error message.

https://sourceware.org/ml/gdb-patches/2018-02/msg00352.html

Re: [PATCH v3 2/2] Make gdbserver work with filename-only binaries

[Simon Marchi]

On 2018-02-27 10:27 PM, Sergio Durigan Junior wrote:
> diff --git a/gdb/testsuite/gdb.server/abspath.exp b/gdb/testsuite/gdb.server/abspath.exp
> new file mode 100644
> index 0000000000..beb1d96209
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/abspath.exp
> @@ -0,0 +1,51 @@
> +# 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
> +}
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
> +    return -1
> +}
> +
> +# Make sure we're disconnected, in case we're testing with an
> +# extended-remote board, therefore already connected.
> +gdb_test "disconnect" ".*"
> +
> +set target_exec [gdbserver_download_current_prog]
> +
> +# Switch to where $target_exec lives, and execute gdbserver from
> +# there.
> +with_cwd "[file dirname $target_exec]" {
> +    set target_execname [file tail $target_exec]
> +    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
> +
> +    gdb_breakpoint main
> +    gdb_test "continue" "Breakpoint $decimal.* main.*" "continue to main"
> +}

The patch LGTM, just a note about the test.

I think this won't work when testing with a remote target (wher gdbserver
is started on another machine.  with_cwd will try to cd into an directory
that exists on the other machine, or an "empty string" directory (I am not sure
what gdbserver_download_current_prog returns), so it will probably crash.

It would complicate the test quite a bit to make it work with a remote
target, and wouldn't add much value: when testing with a remote target,
gdbserver is started with a filename-only argument (that's how I
stumbled on the issue).  So if that breaks, all the other tests will fail.

A counter argument for that would be that the remote board file is subject
to change. if we change it so that it invokes gdbserver by passing
non-filename-only paths, the feature would not be tested anymore...

At the minimum, I think we need to skip this test is target is a remote
one ([is_remote target]).  If you want to go further and make the test
work with a remote target, then go ahead, but I would be fine with
just skipping it.

Simon

RE: [PATCH v3 2/2] Make gdbserver work with filename-only binaries

[Metzger, Markus T]

Hello,

This ...

> > 0x000003fffdfd9280 in ?? () from target:/lib/ld64.so.1 Protocol error:
> > qXfer:btrace-conf (read-btrace-conf) conflicting enabled responses.

... is caused by a regression from removing the btrace_supported target
method for targets that do not define HAVE_LINUX_BTRACE.

A proposed fix is here:
https://sourceware.org/ml/gdb-patches/2018-02/msg00420.html

Maciej already noted the missing _() around string literals.

Regards,
Markus.

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Simon Marchi
> Sent: 28 February 2018 06:33
> To: Sergio Durigan Junior <sergiodj@redhat.com>; GDB Patches <gdb-
> patches@sourceware.org>
> Cc: Pedro Alves <palves@redhat.com>; Joel Brobecker
> <brobecker@adacore.com>
> Subject: Re: [PATCH v3 2/2] Make gdbserver work with filename-only binaries
> 
> On 2018-02-27 10:58 PM, Sergio Durigan Junior wrote:
> > On Tuesday, February 27 2018, I wrote:
> >
> >> 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 including "." 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 verify if the program path provided by
> >> the user (or by the remote protocol) contains a directory separator
> >> character.  If it doesn't, it means we're dealing with a
> >> filename-only binary, so we call "gdb_abspath" to properly expand it
> >> and transform it into a full path.  Otherwise, we leave the program path
> untouched.
> >> This mimicks the behaviour seen on GDB (look at "openp" and
> >> "attach_inferior", for example).
> >>
> >> I am also submitting a testcase which exercises the scenario
> >> described above.  This test requires gdbserver to be executed in a
> >> different CWD than the original, so I also created a helper function,
> >> "with_cwd" (on testsuite/lib/gdb.exp), which takes care of cd'ing
> >> into and out of the specified dir.
> >
> > This part is still giving me a few headaches.  I've just noticed that
> > two builders reported the new test as FAIL.  When I run it here, I
> > can't reproduce it, which makes me wonder that it's racy.  I don't
> > know if the culprit is the new "with_cwd" logic or not.  Curiously,
> > both builders reporting the failure are running on s390x
> > (Debian-s390x-native-extended-gdbserver-m64 and RHEL-s390x-m64).
> >
> > Here's an excerpt of report from one of them,
> > Debian-s390x-native-extended-gdbserver-m64:
> >
> > ----------------
> > (gdb) file
> > /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver
> > /build/gdb/testsuite/outputs/gdb.server/abspath/abspath
> > Reading symbols from /home/dje/debian-jessie-s390x-1/debian-s390x-native-
> extended-
> gdbserver/build/gdb/testsuite/outputs/gdb.server/abspath/abspath...done.
> > (gdb) set remote exec-file
> > /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver
> > /build/gdb/testsuite/outputs/gdb.server/abspath/abspath
> > (gdb) set remote exec-file
> > /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver
> > /build/gdb/testsuite/outputs/gdb.server/abspath/abspath
> > (gdb) disconnect
> > Ending remote debugging.
> > (gdb) PASS: gdb.server/abspath.exp: disconnect Switching to directory
> > /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver
> > /build/gdb/testsuite/outputs/gdb.server/abspath (saved CWD:
> > /home/dje/debian-jessie-s390x-1/debian
> > -s390x-native-extended-gdbserver/build/gdb/testsuite).
> > spawn
> > /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-
> gdbserver/build/gdb/testsuite/../gdbserver/gdbserver --once :2347 abspath
> Can't bind address: Address already in use.
> > Exiting
> > Port 2347 is already in use.
> > spawn
> > /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver
> > /build/gdb/testsuite/../gdbserver/gdbserver --once :2348 abspath
> > Process
> > /home/dje/debian-jessie-s390x-1/debian-s390x-native-extended-gdbserver
> > /build/gdb/testsuite/outputs/gdb.server/abspath/abspath created; pid =
> 21406 Listening on port 2348 target extended-remote localhost:2348 Remote
> debugging using localhost:2348 Reading /lib/ld64.so.1 from remote target...
> > warning: File transfers from remote targets can be slow. Use "set sysroot" to
> access files locally instead.
> > Reading /lib/ld64.so.1 from remote target...
> > Reading symbols from target:/lib/ld64.so.1...Reading /lib/ld-2.19.so from
> remote target...
> > Reading /lib/.debug/ld-2.19.so from remote target...
> > (no debugging symbols found)...done.
> > 0x000003fffdfd9280 in ?? () from target:/lib/ld64.so.1 Protocol error:
> > qXfer:btrace-conf (read-btrace-conf) conflicting enabled responses.
> > (gdb) break main
> > Breakpoint 1 at 0x800005bc: file /home/dje/debian-jessie-s390x-1/debian-
> s390x-native-extended-gdbserver/build/gdb/testsuite/../../../binutils-
> gdb/gdb/testsuite/gdb.server/normal.c, line 23.
> > (gdb) continue
> > The program is not being run.
> > (gdb) FAIL: gdb.server/abspath.exp: continue to main (the program is
> > no longer running) Switching back to /home/dje/debian-jessie-s390x-1/debian-
> s390x-native-extended-gdbserver/build/gdb/testsuite.
> > ----------------
> >
> > They're both running the testsuite in parallel mode (-j8), but when I
> > run it locally in parallel I can't reproduce (even when I run in a
> > loop).
> >
> > Anyway, I'll try to find out why this happens.  I don't see how
> > changing the CWD in a test could impact its results, so I'm guessing
> > there's something else at play here.
> >
> > Thanks,
> >
> 
> Maybe it's just because of this issue?  Note the btrace-related error message.
> 
> https://sourceware.org/ml/gdb-patches/2018-02/msg00352.html
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v3 2/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

On Wednesday, February 28 2018, Simon Marchi wrote:

> On 2018-02-27 10:27 PM, Sergio Durigan Junior wrote:
>> diff --git a/gdb/testsuite/gdb.server/abspath.exp b/gdb/testsuite/gdb.server/abspath.exp
>> new file mode 100644
>> index 0000000000..beb1d96209
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.server/abspath.exp
>> @@ -0,0 +1,51 @@
>> +# 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
>> +}
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
>> +    return -1
>> +}
>> +
>> +# Make sure we're disconnected, in case we're testing with an
>> +# extended-remote board, therefore already connected.
>> +gdb_test "disconnect" ".*"
>> +
>> +set target_exec [gdbserver_download_current_prog]
>> +
>> +# Switch to where $target_exec lives, and execute gdbserver from
>> +# there.
>> +with_cwd "[file dirname $target_exec]" {
>> +    set target_execname [file tail $target_exec]
>> +    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
>> +
>> +    gdb_breakpoint main
>> +    gdb_test "continue" "Breakpoint $decimal.* main.*" "continue to main"
>> +}
>
> The patch LGTM, just a note about the test.
>
> I think this won't work when testing with a remote target (wher gdbserver
> is started on another machine.  with_cwd will try to cd into an directory
> that exists on the other machine, or an "empty string" directory (I am not sure
> what gdbserver_download_current_prog returns), so it will probably crash.
>
> It would complicate the test quite a bit to make it work with a remote
> target, and wouldn't add much value: when testing with a remote target,
> gdbserver is started with a filename-only argument (that's how I
> stumbled on the issue).  So if that breaks, all the other tests will fail.
>
> A counter argument for that would be that the remote board file is subject
> to change. if we change it so that it invokes gdbserver by passing
> non-filename-only paths, the feature would not be tested anymore...
>
> At the minimum, I think we need to skip this test is target is a remote
> one ([is_remote target]).  If you want to go further and make the test
> work with a remote target, then go ahead, but I would be fine with
> just skipping it.

Thanks, Simon.  I'll skip the test for remote targets for now.  I'll
push it afterwards.

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/

Re: [PATCH v3 2/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

On Wednesday, February 28 2018, Markus T. Metzger wrote:

> Hello,
>
> This ...
>
>> > 0x000003fffdfd9280 in ?? () from target:/lib/ld64.so.1 Protocol error:
>> > qXfer:btrace-conf (read-btrace-conf) conflicting enabled responses.
>
> ... is caused by a regression from removing the btrace_supported target
> method for targets that do not define HAVE_LINUX_BTRACE.
>
> A proposed fix is here:
> https://sourceware.org/ml/gdb-patches/2018-02/msg00420.html
>
> Maciej already noted the missing _() around string literals.

Thanks, Markus and Simon.  Indeed, the problem lies elsewhere.

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

Re: [PATCH v3 1/2] Create new common/pathstuff.[ch]

[Sergio Durigan Junior]

On Tuesday, February 27 2018, I wrote:

> Changes from v2:
>
> - Added comments to the path manipulation functions.
>
>
> This commit moves the path manipulation routines found on utils.c to a
> new common/pathstuff.c, and updates the Makefile.in's accordingly.
> The routines moved are "gdb_realpath", "gdb_realpath_keepfile" and
> "gdb_abspath".
>
> This will be needed because gdbserver will have to call "gdb_abspath"
> on my next patch, which implements a way to expand the path of the
> inferior provided by the user in order to allow specifying just the
> binary name when starting gdbserver, like:
>
>   $ gdbserver :1234 a.out
>
> With the recent addition of the startup-with-shell feature on
> gdbserver, this scenario doesn't work anymore if the user doesn't have
> the current directory listed in the PATH variable.
>
> I had to do a minor adjustment on "gdb_abspath" because we don't have
> access to "tilde_expand" on gdbserver, so now the function is using
> "gdb_tilde_expand" instead.  Otherwise, the code is the same.
>
> Regression tested on the BuildBot, without regressions.

Pushed.

b4987c956dfa44ca9fd8552f63e15f5fa094b2a4

Thanks,

>
> gdb/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* Makefile.in (COMMON_SFILES): Add "common/pathstuff.c".
> 	(HFILES_NO_SRCDIR): Add "common/pathstuff.h".
> 	* auto-load.c: Include "common/pathstuff.h".
> 	* common/common-def.h (current_directory): Move here.
> 	* common/gdb_tilde_expand.c (gdb_tilde_expand_up): New
> 	function.
> 	* common/gdb_tilde_expand.h (gdb_tilde_expand_up): New
> 	prototype.
> 	* common/pathstuff.c: New file.
> 	* common/pathstuff.h: New file.
> 	* compile/compile.c: Include "common/pathstuff.h".
> 	* defs.h (current_directory): Move to "common/common-defs.h".
> 	* dwarf2read.c: Include "common/pathstuff.h".
> 	* exec.c: Likewise.
> 	* guile/scm-safe-call.c: Likewise.
> 	* linux-thread-db.c: Likewise.
> 	* main.c: Likewise.
> 	* nto-tdep.c: Likewise.
> 	* objfiles.c: Likewise.
> 	* source.c: Likewise.
> 	* symtab.c: Likewise.
> 	* utils.c: Include "common/pathstuff.h".
> 	(gdb_realpath): Move to "common/pathstuff.c".
> 	(gdb_realpath_keepfile): Likewise.
> 	(gdb_abspath): Likewise.
> 	* utils.h (gdb_realpath): Move to "common/pathstuff.h".
> 	(gdb_realpath_keepfile): Likewise.
> 	(gdb_abspath): Likewise.
>
> gdb/gdbserver/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* Makefile.in (SFILES): Add "$(srcdir)/common/pathstuff.c".
> 	(OBJS): Add "pathstuff.o".
> 	* server.c (current_directory): New global variable.
> 	(captured_main): Initialize "current_directory".
> ---
>  gdb/Makefile.in               |   2 +
>  gdb/auto-load.c               |   1 +
>  gdb/common/common-defs.h      |   3 +
>  gdb/common/gdb_tilde_expand.c |  13 ++++
>  gdb/common/gdb_tilde_expand.h |   4 ++
>  gdb/common/pathstuff.c        | 142 ++++++++++++++++++++++++++++++++++++++++++
>  gdb/common/pathstuff.h        |  49 +++++++++++++++
>  gdb/compile/compile.c         |   1 +
>  gdb/defs.h                    |   4 --
>  gdb/dwarf2read.c              |   1 +
>  gdb/exec.c                    |   1 +
>  gdb/gdbserver/Makefile.in     |   2 +
>  gdb/gdbserver/server.c        |  11 ++++
>  gdb/guile/scm-safe-call.c     |   1 +
>  gdb/linux-thread-db.c         |   1 +
>  gdb/main.c                    |   1 +
>  gdb/nto-tdep.c                |   1 +
>  gdb/objfiles.c                |   1 +
>  gdb/source.c                  |   1 +
>  gdb/symtab.c                  |   1 +
>  gdb/utils.c                   | 120 +----------------------------------
>  gdb/utils.h                   |   6 --
>  22 files changed, 238 insertions(+), 129 deletions(-)
>  create mode 100644 gdb/common/pathstuff.c
>  create mode 100644 gdb/common/pathstuff.h
>
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 1c58b9270d..19be64f226 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -955,6 +955,7 @@ COMMON_SFILES = \
>  	common/gdb_tilde_expand.c \
>  	common/gdb_vecs.c \
>  	common/new-op.c \
> +	common/pathstuff.c \
>  	common/print-utils.c \
>  	common/ptid.c \
>  	common/rsp-low.c \
> @@ -1429,6 +1430,7 @@ HFILES_NO_SRCDIR = \
>  	common/gdb_wait.h \
>  	common/common-inferior.h \
>  	common/host-defs.h \
> +	common/pathstuff.h \
>  	common/print-utils.h \
>  	common/ptid.h \
>  	common/queue.h \
> diff --git a/gdb/auto-load.c b/gdb/auto-load.c
> index b79341faf6..9dd8754e1a 100644
> --- a/gdb/auto-load.c
> +++ b/gdb/auto-load.c
> @@ -40,6 +40,7 @@
>  #include "extension.h"
>  #include "gdb/section-scripts.h"
>  #include <algorithm>
> +#include "common/pathstuff.h"
>  
>  /* The section to look in for auto-loaded scripts (in file formats that
>     support sections).
> diff --git a/gdb/common/common-defs.h b/gdb/common/common-defs.h
> index acbc32ca69..881a4eaaff 100644
> --- a/gdb/common/common-defs.h
> +++ b/gdb/common/common-defs.h
> @@ -91,4 +91,7 @@
>  /* Pull in gdb::unique_xmalloc_ptr.  */
>  #include "common/gdb_unique_ptr.h"
>  
> +/* String containing the current directory (what getwd would return).  */
> +extern char *current_directory;
> +
>  #endif /* COMMON_DEFS_H */
> diff --git a/gdb/common/gdb_tilde_expand.c b/gdb/common/gdb_tilde_expand.c
> index b4f371464d..fcb97961ac 100644
> --- a/gdb/common/gdb_tilde_expand.c
> +++ b/gdb/common/gdb_tilde_expand.c
> @@ -80,3 +80,16 @@ gdb_tilde_expand (const char *dir)
>  
>    return expanded_dir;
>  }
> +
> +/* See common/gdb_tilde_expand.h.  */
> +
> +gdb::unique_xmalloc_ptr<char>
> +gdb_tilde_expand_up (const char *dir)
> +{
> +  gdb_glob glob (dir, GLOB_TILDE_CHECK, NULL);
> +
> +  gdb_assert (glob.pathc () > 0);
> +  /* "glob" may return more than one match to the path provided by the
> +     user, but we are only interested in the first match.  */
> +  return gdb::unique_xmalloc_ptr<char> (xstrdup (glob.pathv ()[0]));
> +}
> diff --git a/gdb/common/gdb_tilde_expand.h b/gdb/common/gdb_tilde_expand.h
> index d0dfb37857..22860d3969 100644
> --- a/gdb/common/gdb_tilde_expand.h
> +++ b/gdb/common/gdb_tilde_expand.h
> @@ -24,4 +24,8 @@
>     the full path.  */
>  extern std::string gdb_tilde_expand (const char *dir);
>  
> +/* Same as GDB_TILDE_EXPAND, but return the full path as a
> +   gdb::unique_xmalloc_ptr<char>.  */
> +extern gdb::unique_xmalloc_ptr<char> gdb_tilde_expand_up (const char *dir);
> +
>  #endif /* ! GDB_TILDE_EXPAND_H */
> diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
> new file mode 100644
> index 0000000000..02f6e44794
> --- /dev/null
> +++ b/gdb/common/pathstuff.c
> @@ -0,0 +1,142 @@
> +/* Path manipulation routines for GDB and gdbserver.
> +
> +   Copyright (C) 1986-2018 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +#include "common-defs.h"
> +#include "pathstuff.h"
> +#include "host-defs.h"
> +#include "filenames.h"
> +#include "gdb_tilde_expand.h"
> +
> +/* See common/pathstuff.h.  */
> +
> +gdb::unique_xmalloc_ptr<char>
> +gdb_realpath (const char *filename)
> +{
> +/* On most hosts, we rely on canonicalize_file_name to compute
> +   the FILENAME's realpath.
> +
> +   But the situation is slightly more complex on Windows, due to some
> +   versions of GCC which were reported to generate paths where
> +   backlashes (the directory separator) were doubled.  For instance:
> +      c:\\some\\double\\slashes\\dir
> +   ... instead of ...
> +      c:\some\double\slashes\dir
> +   Those double-slashes were getting in the way when comparing paths,
> +   for instance when trying to insert a breakpoint as follow:
> +      (gdb) b c:/some/double/slashes/dir/foo.c:4
> +      No source file named c:/some/double/slashes/dir/foo.c:4.
> +      (gdb) b c:\some\double\slashes\dir\foo.c:4
> +      No source file named c:\some\double\slashes\dir\foo.c:4.
> +   To prevent this from happening, we need this function to always
> +   strip those extra backslashes.  While canonicalize_file_name does
> +   perform this simplification, it only works when the path is valid.
> +   Since the simplification would be useful even if the path is not
> +   valid (one can always set a breakpoint on a file, even if the file
> +   does not exist locally), we rely instead on GetFullPathName to
> +   perform the canonicalization.  */
> +
> +#if defined (_WIN32)
> +  {
> +    char buf[MAX_PATH];
> +    DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
> +
> +    /* The file system is case-insensitive but case-preserving.
> +       So it is important we do not lowercase the path.  Otherwise,
> +       we might not be able to display the original casing in a given
> +       path.  */
> +    if (len > 0 && len < MAX_PATH)
> +      return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
> +  }
> +#else
> +  {
> +    char *rp = canonicalize_file_name (filename);
> +
> +    if (rp != NULL)
> +      return gdb::unique_xmalloc_ptr<char> (rp);
> +  }
> +#endif
> +
> +  /* This system is a lost cause, just dup the buffer.  */
> +  return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
> +}
> +
> +/* See common/pathstuff.h.  */
> +
> +gdb::unique_xmalloc_ptr<char>
> +gdb_realpath_keepfile (const char *filename)
> +{
> +  const char *base_name = lbasename (filename);
> +  char *dir_name;
> +  char *result;
> +
> +  /* Extract the basename of filename, and return immediately
> +     a copy of filename if it does not contain any directory prefix.  */
> +  if (base_name == filename)
> +    return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
> +
> +  dir_name = (char *) alloca ((size_t) (base_name - filename + 2));
> +  /* Allocate enough space to store the dir_name + plus one extra
> +     character sometimes needed under Windows (see below), and
> +     then the closing \000 character.  */
> +  strncpy (dir_name, filename, base_name - filename);
> +  dir_name[base_name - filename] = '\000';
> +
> +#ifdef HAVE_DOS_BASED_FILE_SYSTEM
> +  /* We need to be careful when filename is of the form 'd:foo', which
> +     is equivalent of d:./foo, which is totally different from d:/foo.  */
> +  if (strlen (dir_name) == 2 && isalpha (dir_name[0]) && dir_name[1] == ':')
> +    {
> +      dir_name[2] = '.';
> +      dir_name[3] = '\000';
> +    }
> +#endif
> +
> +  /* Canonicalize the directory prefix, and build the resulting
> +     filename.  If the dirname realpath already contains an ending
> +     directory separator, avoid doubling it.  */
> +  gdb::unique_xmalloc_ptr<char> path_storage = gdb_realpath (dir_name);
> +  const char *real_path = path_storage.get ();
> +  if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
> +    result = concat (real_path, base_name, (char *) NULL);
> +  else
> +    result = concat (real_path, SLASH_STRING, base_name, (char *) NULL);
> +
> +  return gdb::unique_xmalloc_ptr<char> (result);
> +}
> +
> +/* See common/pathstuff.h.  */
> +
> +gdb::unique_xmalloc_ptr<char>
> +gdb_abspath (const char *path)
> +{
> +  gdb_assert (path != NULL && path[0] != '\0');
> +
> +  if (path[0] == '~')
> +    return gdb_tilde_expand_up (path);
> +
> +  if (IS_ABSOLUTE_PATH (path))
> +    return gdb::unique_xmalloc_ptr<char> (xstrdup (path));
> +
> +  /* Beware the // my son, the Emacs barfs, the botch that catch...  */
> +  return gdb::unique_xmalloc_ptr<char>
> +    (concat (current_directory,
> +	     IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
> +	     ? "" : SLASH_STRING,
> +	     path, (char *) NULL));
> +}
> diff --git a/gdb/common/pathstuff.h b/gdb/common/pathstuff.h
> new file mode 100644
> index 0000000000..3cb02c86d6
> --- /dev/null
> +++ b/gdb/common/pathstuff.h
> @@ -0,0 +1,49 @@
> +/* Path manipulation routines for GDB and gdbserver.
> +
> +   Copyright (C) 1986-2018 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   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/>.  */
> +
> +#ifndef PATHSTUFF_H
> +#define PATHSTUFF_H
> +
> +/* Path utilities.  */
> +
> +/* Return the real path of FILENAME, expanding all the symbolic links.
> +
> +   Contrary to "gdb_abspath", this function does not use
> +   CURRENT_DIRECTORY for path expansion.  Instead, it relies on the
> +   current working directory (CWD) of GDB or gdbserver.  */
> +
> +extern gdb::unique_xmalloc_ptr<char> gdb_realpath (const char *filename);
> +
> +/* Return a copy of FILENAME, with its directory prefix canonicalized
> +   by gdb_realpath.  */
> +
> +extern gdb::unique_xmalloc_ptr<char>
> +  gdb_realpath_keepfile (const char *filename);
> +
> +/* Return PATH in absolute form, performing tilde-expansion if necessary.
> +   PATH cannot be NULL or the empty string.
> +   This does not resolve symlinks however, use gdb_realpath for that.
> +
> +   Contrary to "gdb_realpath", this function uses CURRENT_DIRECTORY
> +   for the path expansion.  This may lead to scenarios the current
> +   working directory (CWD) is different than CURRENT_DIRECTORY.  */
> +
> +extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);
> +
> +#endif /* PATHSTUFF_H */
> diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c
> index 70c4570de7..7f35272872 100644
> --- a/gdb/compile/compile.c
> +++ b/gdb/compile/compile.c
> @@ -41,6 +41,7 @@
>  #include "valprint.h"
>  #include "common/gdb_optional.h"
>  #include "common/gdb_unlinker.h"
> +#include "common/pathstuff.h"
>  
>  \f
>  
> diff --git a/gdb/defs.h b/gdb/defs.h
> index c85bf2cf11..a924573b57 100644
> --- a/gdb/defs.h
> +++ b/gdb/defs.h
> @@ -402,10 +402,6 @@ enum info_proc_what
>      IP_ALL
>    };
>  
> -/* * String containing the current directory (what getwd would return).  */
> -
> -extern char *current_directory;
> -
>  /* * Default radixes for input and output.  Only some values supported.  */
>  extern unsigned input_radix;
>  extern unsigned output_radix;
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 1e4376e4de..9825117fa7 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -87,6 +87,7 @@
>  #include <set>
>  #include <forward_list>
>  #include "rust-lang.h"
> +#include "common/pathstuff.h"
>  
>  /* When == 1, print basic high level tracing messages.
>     When > 1, be more verbose.
> diff --git a/gdb/exec.c b/gdb/exec.c
> index 0b4237ff63..6b910e8f54 100644
> --- a/gdb/exec.c
> +++ b/gdb/exec.c
> @@ -45,6 +45,7 @@
>  #include <sys/stat.h>
>  #include "solist.h"
>  #include <algorithm>
> +#include "common/pathstuff.h"
>  
>  void (*deprecated_file_changed_hook) (const char *);
>  
> diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
> index 2dbf9ae63d..f2936920fe 100644
> --- a/gdb/gdbserver/Makefile.in
> +++ b/gdb/gdbserver/Makefile.in
> @@ -209,6 +209,7 @@ SFILES = \
>  	$(srcdir)/common/gdb_tilde_expand.c \
>  	$(srcdir)/common/gdb_vecs.c \
>  	$(srcdir)/common/new-op.c \
> +	$(srcdir)/common/pathstuff.c \
>  	$(srcdir)/common/print-utils.c \
>  	$(srcdir)/common/ptid.c \
>  	$(srcdir)/common/rsp-low.c \
> @@ -249,6 +250,7 @@ OBS = \
>  	common/gdb_tilde_expand.o \
>  	common/gdb_vecs.o \
>  	common/new-op.o \
> +	common/pathstuff.o \
>  	common/print-utils.o \
>  	common/ptid.o \
>  	common/rsp-low.o \
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index cb02b58507..922d5269b3 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -56,6 +56,10 @@
>        break;					\
>      }
>  
> +/* String containing the current directory (what getwd would return).  */
> +
> +char *current_directory;
> +
>  /* The environment to pass to the inferior when creating it.  */
>  
>  static gdb_environ our_environ;
> @@ -3539,6 +3543,13 @@ captured_main (int argc, char *argv[])
>    const char *selftest_filter = NULL;
>  #endif
>  
> +  current_directory = getcwd (NULL, 0);
> +  if (current_directory == NULL)
> +    {
> +      error (_("%s: error finding working directory"),
> +	     safe_strerror (errno));
> +    }
> +
>    while (*next_arg != NULL && **next_arg == '-')
>      {
>        if (strcmp (*next_arg, "--version") == 0)
> diff --git a/gdb/guile/scm-safe-call.c b/gdb/guile/scm-safe-call.c
> index a9ce7b72c1..2cba399e23 100644
> --- a/gdb/guile/scm-safe-call.c
> +++ b/gdb/guile/scm-safe-call.c
> @@ -23,6 +23,7 @@
>  #include "defs.h"
>  #include "filenames.h"
>  #include "guile-internal.h"
> +#include "common/pathstuff.h"
>  
>  /* Struct to marshall args to scscm_safe_call_body.  */
>  
> diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
> index 5ba4c03df6..10c85f56ad 100644
> --- a/gdb/linux-thread-db.c
> +++ b/gdb/linux-thread-db.c
> @@ -46,6 +46,7 @@
>  #include <ctype.h>
>  #include "nat/linux-namespaces.h"
>  #include <algorithm>
> +#include "common/pathstuff.h"
>  
>  /* GNU/Linux libthread_db support.
>  
> diff --git a/gdb/main.c b/gdb/main.c
> index 3c98787edb..189266f90e 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -46,6 +46,7 @@
>  #include "infrun.h"
>  #include "signals-state-save-restore.h"
>  #include <vector>
> +#include "common/pathstuff.h"
>  
>  /* The selected interpreter.  This will be used as a set command
>     variable, so it should always be malloc'ed - since
> diff --git a/gdb/nto-tdep.c b/gdb/nto-tdep.c
> index 8eb864b871..82a4fcbbb9 100644
> --- a/gdb/nto-tdep.c
> +++ b/gdb/nto-tdep.c
> @@ -32,6 +32,7 @@
>  #include "gdbcore.h"
>  #include "objfiles.h"
>  #include "source.h"
> +#include "common/pathstuff.h"
>  
>  #define QNX_NOTE_NAME	"QNX"
>  #define QNX_INFO_SECT_NAME "QNX_info"
> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> index 70e369b8b4..a9aaf89540 100644
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
> @@ -52,6 +52,7 @@
>  #include "solist.h"
>  #include "gdb_bfd.h"
>  #include "btrace.h"
> +#include "common/pathstuff.h"
>  
>  #include <vector>
>  
> diff --git a/gdb/source.c b/gdb/source.c
> index 8a27b2e666..04ee3b33d2 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -44,6 +44,7 @@
>  #include "readline/readline.h"
>  #include "common/enum-flags.h"
>  #include <algorithm>
> +#include "common/pathstuff.h"
>  
>  #define OPEN_MODE (O_RDONLY | O_BINARY)
>  #define FDOPEN_MODE FOPEN_RB
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 0fd3f3a30f..567195304f 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -66,6 +66,7 @@
>  #include "filename-seen-cache.h"
>  #include "arch-utils.h"
>  #include <algorithm>
> +#include "common/pathstuff.h"
>  
>  /* Forward declarations for local functions.  */
>  
> diff --git a/gdb/utils.c b/gdb/utils.c
> index c531748fe4..577f9df4ec 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -70,6 +70,7 @@
>  #include "common/gdb_optional.h"
>  #include "cp-support.h"
>  #include <algorithm>
> +#include "common/pathstuff.h"
>  
>  #if !HAVE_DECL_MALLOC
>  extern PTR malloc ();		/* ARI: PTR */
> @@ -2838,57 +2839,6 @@ string_to_core_addr (const char *my_string)
>    return addr;
>  }
>  
> -gdb::unique_xmalloc_ptr<char>
> -gdb_realpath (const char *filename)
> -{
> -/* On most hosts, we rely on canonicalize_file_name to compute
> -   the FILENAME's realpath.
> -
> -   But the situation is slightly more complex on Windows, due to some
> -   versions of GCC which were reported to generate paths where
> -   backlashes (the directory separator) were doubled.  For instance:
> -      c:\\some\\double\\slashes\\dir
> -   ... instead of ...
> -      c:\some\double\slashes\dir
> -   Those double-slashes were getting in the way when comparing paths,
> -   for instance when trying to insert a breakpoint as follow:
> -      (gdb) b c:/some/double/slashes/dir/foo.c:4
> -      No source file named c:/some/double/slashes/dir/foo.c:4.
> -      (gdb) b c:\some\double\slashes\dir\foo.c:4
> -      No source file named c:\some\double\slashes\dir\foo.c:4.
> -   To prevent this from happening, we need this function to always
> -   strip those extra backslashes.  While canonicalize_file_name does
> -   perform this simplification, it only works when the path is valid.
> -   Since the simplification would be useful even if the path is not
> -   valid (one can always set a breakpoint on a file, even if the file
> -   does not exist locally), we rely instead on GetFullPathName to
> -   perform the canonicalization.  */
> -
> -#if defined (_WIN32)
> -  {
> -    char buf[MAX_PATH];
> -    DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
> -
> -    /* The file system is case-insensitive but case-preserving.
> -       So it is important we do not lowercase the path.  Otherwise,
> -       we might not be able to display the original casing in a given
> -       path.  */
> -    if (len > 0 && len < MAX_PATH)
> -      return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
> -  }
> -#else
> -  {
> -    char *rp = canonicalize_file_name (filename);
> -
> -    if (rp != NULL)
> -      return gdb::unique_xmalloc_ptr<char> (rp);
> -  }
> -#endif
> -
> -  /* This system is a lost cause, just dup the buffer.  */
> -  return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
> -}
> -
>  #if GDB_SELF_TEST
>  
>  static void
> @@ -2925,74 +2875,6 @@ gdb_realpath_tests ()
>  
>  #endif /* GDB_SELF_TEST */
>  
> -/* Return a copy of FILENAME, with its directory prefix canonicalized
> -   by gdb_realpath.  */
> -
> -gdb::unique_xmalloc_ptr<char>
> -gdb_realpath_keepfile (const char *filename)
> -{
> -  const char *base_name = lbasename (filename);
> -  char *dir_name;
> -  char *result;
> -
> -  /* Extract the basename of filename, and return immediately 
> -     a copy of filename if it does not contain any directory prefix.  */
> -  if (base_name == filename)
> -    return gdb::unique_xmalloc_ptr<char> (xstrdup (filename));
> -
> -  dir_name = (char *) alloca ((size_t) (base_name - filename + 2));
> -  /* Allocate enough space to store the dir_name + plus one extra
> -     character sometimes needed under Windows (see below), and
> -     then the closing \000 character.  */
> -  strncpy (dir_name, filename, base_name - filename);
> -  dir_name[base_name - filename] = '\000';
> -
> -#ifdef HAVE_DOS_BASED_FILE_SYSTEM
> -  /* We need to be careful when filename is of the form 'd:foo', which
> -     is equivalent of d:./foo, which is totally different from d:/foo.  */
> -  if (strlen (dir_name) == 2 && isalpha (dir_name[0]) && dir_name[1] == ':')
> -    {
> -      dir_name[2] = '.';
> -      dir_name[3] = '\000';
> -    }
> -#endif
> -
> -  /* Canonicalize the directory prefix, and build the resulting
> -     filename.  If the dirname realpath already contains an ending
> -     directory separator, avoid doubling it.  */
> -  gdb::unique_xmalloc_ptr<char> path_storage = gdb_realpath (dir_name);
> -  const char *real_path = path_storage.get ();
> -  if (IS_DIR_SEPARATOR (real_path[strlen (real_path) - 1]))
> -    result = concat (real_path, base_name, (char *) NULL);
> -  else
> -    result = concat (real_path, SLASH_STRING, base_name, (char *) NULL);
> -
> -  return gdb::unique_xmalloc_ptr<char> (result);
> -}
> -
> -/* Return PATH in absolute form, performing tilde-expansion if necessary.
> -   PATH cannot be NULL or the empty string.
> -   This does not resolve symlinks however, use gdb_realpath for that.  */
> -
> -gdb::unique_xmalloc_ptr<char>
> -gdb_abspath (const char *path)
> -{
> -  gdb_assert (path != NULL && path[0] != '\0');
> -
> -  if (path[0] == '~')
> -    return gdb::unique_xmalloc_ptr<char> (tilde_expand (path));
> -
> -  if (IS_ABSOLUTE_PATH (path))
> -    return gdb::unique_xmalloc_ptr<char> (xstrdup (path));
> -
> -  /* Beware the // my son, the Emacs barfs, the botch that catch...  */
> -  return gdb::unique_xmalloc_ptr<char>
> -    (concat (current_directory,
> -	     IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1])
> -	     ? "" : SLASH_STRING,
> -	     path, (char *) NULL));
> -}
> -
>  ULONGEST
>  align_up (ULONGEST v, int n)
>  {
> diff --git a/gdb/utils.h b/gdb/utils.h
> index b234762929..8ca3eb0369 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -295,12 +295,6 @@ extern struct cleanup *make_bpstat_clear_actions_cleanup (void);
>  \f
>  /* Path utilities.  */
>  
> -extern gdb::unique_xmalloc_ptr<char> gdb_realpath (const char *);
> -
> -extern gdb::unique_xmalloc_ptr<char> gdb_realpath_keepfile (const char *);
> -
> -extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *);
> -
>  extern int gdb_filename_fnmatch (const char *pattern, const char *string,
>  				 int flags);
>  
> -- 
> 2.14.3

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

Re: [PATCH v3 2/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

On Wednesday, February 28 2018, I wrote:

> On Wednesday, February 28 2018, Simon Marchi wrote:
>
>> On 2018-02-27 10:27 PM, Sergio Durigan Junior wrote:
>>> diff --git a/gdb/testsuite/gdb.server/abspath.exp b/gdb/testsuite/gdb.server/abspath.exp
>>> new file mode 100644
>>> index 0000000000..beb1d96209
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.server/abspath.exp
>>> @@ -0,0 +1,51 @@
>>> +# 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
>>> +}
>>> +
>>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
>>> +    return -1
>>> +}
>>> +
>>> +# Make sure we're disconnected, in case we're testing with an
>>> +# extended-remote board, therefore already connected.
>>> +gdb_test "disconnect" ".*"
>>> +
>>> +set target_exec [gdbserver_download_current_prog]
>>> +
>>> +# Switch to where $target_exec lives, and execute gdbserver from
>>> +# there.
>>> +with_cwd "[file dirname $target_exec]" {
>>> +    set target_execname [file tail $target_exec]
>>> +    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
>>> +
>>> +    gdb_breakpoint main
>>> +    gdb_test "continue" "Breakpoint $decimal.* main.*" "continue to main"
>>> +}
>>
>> The patch LGTM, just a note about the test.
>>
>> I think this won't work when testing with a remote target (wher gdbserver
>> is started on another machine.  with_cwd will try to cd into an directory
>> that exists on the other machine, or an "empty string" directory (I am not sure
>> what gdbserver_download_current_prog returns), so it will probably crash.
>>
>> It would complicate the test quite a bit to make it work with a remote
>> target, and wouldn't add much value: when testing with a remote target,
>> gdbserver is started with a filename-only argument (that's how I
>> stumbled on the issue).  So if that breaks, all the other tests will fail.
>>
>> A counter argument for that would be that the remote board file is subject
>> to change. if we change it so that it invokes gdbserver by passing
>> non-filename-only paths, the feature would not be tested anymore...
>>
>> At the minimum, I think we need to skip this test is target is a remote
>> one ([is_remote target]).  If you want to go further and make the test
>> work with a remote target, then go ahead, but I would be fine with
>> just skipping it.
>
> Thanks, Simon.  I'll skip the test for remote targets for now.  I'll
> push it afterwards.

Pushed.

25e3c82c0e927398e759e2d5e35623012b8683f7

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/

Re: [PATCH v3 1/2] Create new common/pathstuff.[ch]

[Sergio Durigan Junior]

On Wednesday, February 28 2018, Simon Marchi wrote:

> On 2018-02-27 10:27 PM, Sergio Durigan Junior wrote:
>> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
>> index cb02b58507..922d5269b3 100644
>> --- a/gdb/gdbserver/server.c
>> +++ b/gdb/gdbserver/server.c
>> @@ -56,6 +56,10 @@
>>        break;					\
>>      }
>>  
>> +/* String containing the current directory (what getwd would return).  */
>> +
>> +char *current_directory;
>> +
>>  /* The environment to pass to the inferior when creating it.  */
>>  
>>  static gdb_environ our_environ;
>> @@ -3539,6 +3543,13 @@ captured_main (int argc, char *argv[])
>>    const char *selftest_filter = NULL;
>>  #endif
>>  
>> +  current_directory = getcwd (NULL, 0);
>> +  if (current_directory == NULL)
>> +    {
>> +      error (_("%s: error finding working directory"),
>> +	     safe_strerror (errno));
>> +    }
>
> Just one thing, I think Pedro suggested to put the variable string at the end:
>
>    error (_("Could not find working directory: %s"),
>        safe_strerror (errno));

Ah, that's right, sorry about this, I got confused.  And I didn't get
this message on my INBOX, so I went ahead and pushed the commit without
this change.  I took the liberty to push an obvious commit now fixing
this.

815615463b1171cbff7c5e54f62fc708cc1bbc99

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/

[obvious/pushed] Change order of error message printed when gdbserver can't find CWD

[Sergio Durigan Junior]

I forgot to address Pedro's comment about my last patch and change the
order of the message printed when getcwd returns NULL on gdbserver.
This obvious commit does it.

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

	* server.c (captured_main): Change order of error message printed
	when the current working directory cannot be found.
---
 gdb/gdbserver/ChangeLog | 5 +++++
 gdb/gdbserver/server.c  | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 8c3b02e5e4..ac88a9d778 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,8 @@
+2018-02-28  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	* server.c (captured_main): Change order of error message printed
+	when the current working directory cannot be found.
+
 2018-02-28  Sergio Durigan Junior  <sergiodj@redhat.com>
 
 	* server.c: Include "filenames.h" and "pathstuff.h".
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 7745027628..f373d8a1e8 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -3573,7 +3573,7 @@ captured_main (int argc, char *argv[])
   current_directory = getcwd (NULL, 0);
   if (current_directory == NULL)
     {
-      error (_("%s: error finding working directory"),
+      error (_("Could not find current working directory: %s"),
 	     safe_strerror (errno));
     }
 
-- 
2.14.3

Re: [PATCH v3 0/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

On Tuesday, February 27 2018, I wrote:

> Third version.
>
> These two patches fix the issue pointed by Simon on IRC, that
> gdbserver doesn't recognize filename-only binaries anymore:
>
>   $ gdbserver :1234 a.out
>   /bin/bash: line 0: exec: a.out: not found
>   During startup program exited with code 127.
>   Exiting
>
> The first one is a preparation patch (and can go in independently),
> which moves path-manipulation functions from utils.c to a new
> common/pathstuff.c, making them available for gdbserver as well.
>
> The second patch is the fix itself.
>
> More details in each message.

Simon reminded me that this patch is also a good fit for the 8.1 branch,
so I went ahead and pushed it there.

506817a3abd98859eb3474389e756c0253cc28a1
2441702a72f324e41a1624dc042b334f375e2d81
6d607b8812b35ff36fbbad2915696f6669f86a32

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/

Re: [PATCH v3 0/2] Make gdbserver work with filename-only binaries

[Joel Brobecker]

> Simon reminded me that this patch is also a good fit for the 8.1 branch,
> so I went ahead and pushed it there.
> 
> 506817a3abd98859eb3474389e756c0253cc28a1
> 2441702a72f324e41a1624dc042b334f375e2d81
> 6d607b8812b35ff36fbbad2915696f6669f86a32

Thanks for getting this through, Sergio and Simon.

Just a quick reminder that, now that the .0 is out, all new patches
pushed on the branch should have a corresponding PR number, with
the target milestone set to 8.1. This is to be able to give users
an actionable list of PRs they can look at if they are wondering
what the difference between 8.0 and 8.1 is. Is there one for this
issue? If not, it's good enough to create one after the fact, as
long as the PR points to the various discussions and maybe gives
the SHA1 of the various commits, I think we're good.

Thank you!
-- 
Joel

Re: [PATCH v3 0/2] Make gdbserver work with filename-only binaries

[Christophe Lyon]

On 1 March 2018 at 03:55, Joel Brobecker <brobecker@adacore.com> wrote:
>> Simon reminded me that this patch is also a good fit for the 8.1 branch,
>> so I went ahead and pushed it there.
>>
>> 506817a3abd98859eb3474389e756c0253cc28a1
>> 2441702a72f324e41a1624dc042b334f375e2d81
>> 6d607b8812b35ff36fbbad2915696f6669f86a32
>

Hi,

These new patches seem to cause problems with building for ming (using
i686-w64-mingw32-g++):

/gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
gdb_realpath(const char*)':
/gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in this scope
     char buf[MAX_PATH];
              ^
/gdb/common/pathstuff.c:57:5: error: 'DWORD' was not declared in this scope
     DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
     ^
/gdb/common/pathstuff.c:57:11: error: expected ';' before 'len'
     DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
           ^
/gdb/common/pathstuff.c:63:9: error: 'len' was not declared in this scope
     if (len > 0 && len < MAX_PATH)
         ^
/gdb/common/pathstuff.c:64:54: error: 'buf' was not declared in this scope
       return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
                                                      ^
make[2]: *** [pathstuff.o] Error 1

I saw this while rebuilding branch 8.1, I didn't check master.

Sorry if this has already been reported, I can't find any mention of
this problem in the list archives.

I suspect there's already a recommended way of handling MAX_PATH cross-platform.

Thanks

Christophe

> Thanks for getting this through, Sergio and Simon.
>
> Just a quick reminder that, now that the .0 is out, all new patches
> pushed on the branch should have a corresponding PR number, with
> the target milestone set to 8.1. This is to be able to give users
> an actionable list of PRs they can look at if they are wondering
> what the difference between 8.0 and 8.1 is. Is there one for this
> issue? If not, it's good enough to create one after the fact, as
> long as the PR points to the various discussions and maybe gives
> the SHA1 of the various commits, I think we're good.
>
> Thank you!
> --
> Joel

Re: [PATCH v3 0/2] Make gdbserver work with filename-only binaries

[Simon Marchi]

On 2018-03-01 08:08, Christophe Lyon wrote:
> These new patches seem to cause problems with building for ming (using
> i686-w64-mingw32-g++):
> 
> /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
> gdb_realpath(const char*)':
> /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in 
> this scope
>      char buf[MAX_PATH];
>               ^
> /gdb/common/pathstuff.c:57:5: error: 'DWORD' was not declared in this 
> scope
>      DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
>      ^
> /gdb/common/pathstuff.c:57:11: error: expected ';' before 'len'
>      DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
>            ^
> /gdb/common/pathstuff.c:63:9: error: 'len' was not declared in this 
> scope
>      if (len > 0 && len < MAX_PATH)
>          ^
> /gdb/common/pathstuff.c:64:54: error: 'buf' was not declared in this 
> scope
>        return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
>                                                       ^
> make[2]: *** [pathstuff.o] Error 1
> 
> I saw this while rebuilding branch 8.1, I didn't check master.
> 
> Sorry if this has already been reported, I can't find any mention of
> this problem in the list archives.
> 
> I suspect there's already a recommended way of handling MAX_PATH 
> cross-platform.
> 
> Thanks
> 
> Christophe

MAX_PATH is windows specific, note that this code is in an #if defined 
(_WIN32) guard.  I think we just need to add #include <windows.h> with 
the same guard.

Simon

Re: [PATCH v3 0/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

On Wednesday, February 28 2018, Joel Brobecker wrote:

>> Simon reminded me that this patch is also a good fit for the 8.1 branch,
>> so I went ahead and pushed it there.
>> 
>> 506817a3abd98859eb3474389e756c0253cc28a1
>> 2441702a72f324e41a1624dc042b334f375e2d81
>> 6d607b8812b35ff36fbbad2915696f6669f86a32
>
> Thanks for getting this through, Sergio and Simon.
>
> Just a quick reminder that, now that the .0 is out, all new patches
> pushed on the branch should have a corresponding PR number, with
> the target milestone set to 8.1. This is to be able to give users
> an actionable list of PRs they can look at if they are wondering
> what the difference between 8.0 and 8.1 is. Is there one for this
> issue? If not, it's good enough to create one after the fact, as
> long as the PR points to the various discussions and maybe gives
> the SHA1 of the various commits, I think we're good.

Hi Joel,

Sorry about this, I forgot about the need to create a PR when the
release is out.  There is no PR for this bug, so I created one following
the specifications:

https://sourceware.org/bugzilla/show_bug.cgi?id=22907

Please let me know if there's something else I can do.

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/

Re: [PATCH v3 0/2] Make gdbserver work with filename-only binaries

[Sergio Durigan Junior]

On Thursday, March 01 2018, Christophe Lyon wrote:

> On 1 March 2018 at 03:55, Joel Brobecker <brobecker@adacore.com> wrote:
>>> Simon reminded me that this patch is also a good fit for the 8.1 branch,
>>> so I went ahead and pushed it there.
>>>
>>> 506817a3abd98859eb3474389e756c0253cc28a1
>>> 2441702a72f324e41a1624dc042b334f375e2d81
>>> 6d607b8812b35ff36fbbad2915696f6669f86a32
>>
>
> Hi,
>
> These new patches seem to cause problems with building for ming (using
> i686-w64-mingw32-g++):

I'm taking a look at this right now.  Thanks for reporting.

> /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
> gdb_realpath(const char*)':
> /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in this scope
>      char buf[MAX_PATH];
>               ^
> /gdb/common/pathstuff.c:57:5: error: 'DWORD' was not declared in this scope
>      DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
>      ^
> /gdb/common/pathstuff.c:57:11: error: expected ';' before 'len'
>      DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
>            ^
> /gdb/common/pathstuff.c:63:9: error: 'len' was not declared in this scope
>      if (len > 0 && len < MAX_PATH)
>          ^
> /gdb/common/pathstuff.c:64:54: error: 'buf' was not declared in this scope
>        return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
>                                                       ^
> make[2]: *** [pathstuff.o] Error 1
>
> I saw this while rebuilding branch 8.1, I didn't check master.
>
> Sorry if this has already been reported, I can't find any mention of
> this problem in the list archives.
>
> I suspect there's already a recommended way of handling MAX_PATH cross-platform.
>
> Thanks
>
> Christophe
>
>> Thanks for getting this through, Sergio and Simon.
>>
>> Just a quick reminder that, now that the .0 is out, all new patches
>> pushed on the branch should have a corresponding PR number, with
>> the target milestone set to 8.1. This is to be able to give users
>> an actionable list of PRs they can look at if they are wondering
>> what the difference between 8.0 and 8.1 is. Is there one for this
>> issue? If not, it's good enough to create one after the fact, as
>> long as the PR points to the various discussions and maybe gives
>> the SHA1 of the various commits, I think we're good.
>>
>> Thank you!
>> --
>> Joel

-- 
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] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Sergio Durigan Junior]

commit b4987c956dfa44ca9fd8552f63e15f5fa094b2a4
Author: Sergio Durigan Junior <sergiodj@redhat.com>
Date:   Fri Feb 9 18:44:59 2018 -0500

    Create new common/pathstuff.[ch]

Introduced a regression when compiling for mingw*:

  /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
  gdb_realpath(const char*)':
  /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in this scope
       char buf[MAX_PATH];
		^
  /gdb/common/pathstuff.c:57:5: error: 'DWORD' was not declared in this scope
       DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
       ^
  /gdb/common/pathstuff.c:57:11: error: expected ';' before 'len'
       DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
	     ^
  /gdb/common/pathstuff.c:63:9: error: 'len' was not declared in this scope
       if (len > 0 && len < MAX_PATH)
	   ^
  /gdb/common/pathstuff.c:64:54: error: 'buf' was not declared in this scope
	 return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
							^
  make[2]: *** [pathstuff.o] Error 1

The proper fix is to conditionally include "<windows.h>".  This commit
does that, without introducing any regressions as per tests made by
our BuildBot.

gdb/ChangeLog:
2018-03-01  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR gdb/22907
	* common/pathstuff.c: Conditionally include "<windows.h>".
---
 gdb/common/pathstuff.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
index fc574dc32e..8c4093fc38 100644
--- a/gdb/common/pathstuff.c
+++ b/gdb/common/pathstuff.c
@@ -23,6 +23,10 @@
 #include "filenames.h"
 #include "gdb_tilde_expand.h"
 
+#ifdef USE_WIN32API
+#include <windows.h>
+#endif
+
 /* See common/pathstuff.h.  */
 
 gdb::unique_xmalloc_ptr<char>
-- 
2.14.3

Re: [PATCH] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Simon Marchi]

On 2018-03-01 15:20, Sergio Durigan Junior wrote:
> commit b4987c956dfa44ca9fd8552f63e15f5fa094b2a4
> Author: Sergio Durigan Junior <sergiodj@redhat.com>
> Date:   Fri Feb 9 18:44:59 2018 -0500
> 
>     Create new common/pathstuff.[ch]
> 
> Introduced a regression when compiling for mingw*:
> 
>   /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
>   gdb_realpath(const char*)':
>   /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in
> this scope
>        char buf[MAX_PATH];
> 		^
>   /gdb/common/pathstuff.c:57:5: error: 'DWORD' was not declared in this 
> scope
>        DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
>        ^
>   /gdb/common/pathstuff.c:57:11: error: expected ';' before 'len'
>        DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
> 	     ^
>   /gdb/common/pathstuff.c:63:9: error: 'len' was not declared in this 
> scope
>        if (len > 0 && len < MAX_PATH)
> 	   ^
>   /gdb/common/pathstuff.c:64:54: error: 'buf' was not declared in this 
> scope
> 	 return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
> 							^
>   make[2]: *** [pathstuff.o] Error 1
> 
> The proper fix is to conditionally include "<windows.h>".  This commit
> does that, without introducing any regressions as per tests made by
> our BuildBot.
> 
> gdb/ChangeLog:
> 2018-03-01  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	PR gdb/22907
> 	* common/pathstuff.c: Conditionally include "<windows.h>".
> ---
>  gdb/common/pathstuff.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
> index fc574dc32e..8c4093fc38 100644
> --- a/gdb/common/pathstuff.c
> +++ b/gdb/common/pathstuff.c
> @@ -23,6 +23,10 @@
>  #include "filenames.h"
>  #include "gdb_tilde_expand.h"
> 
> +#ifdef USE_WIN32API
> +#include <windows.h>
> +#endif
> +
>  /* See common/pathstuff.h.  */
> 
>  gdb::unique_xmalloc_ptr<char>

Christopher, does that fix the issue for you?  If so, the patch LGTM.

Simon

Re: [PATCH v3 0/2] Make gdbserver work with filename-only binaries

[Joel Brobecker]

> Sorry about this, I forgot about the need to create a PR when the
> release is out.  There is no PR for this bug, so I created one following
> the specifications:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=22907
> 
> Please let me know if there's something else I can do.

No worries. This is perfect, thank you!

-- 
Joel

Re: [PATCH] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Yao Qi]

Sergio Durigan Junior <sergiodj@redhat.com> writes:

> Introduced a regression when compiling for mingw*:
>
>   /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
>   gdb_realpath(const char*)':
>   /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in this scope
>        char buf[MAX_PATH];

Fedora-x86_64-w64-mingw32 catches this build failure
https://gdb-build.sergiodj.net/builders/Fedora-x86_64-w64-mingw32/builds/1113/steps/compile%20gdb/logs/stdio
but it doesn't send notification on build failure.  Could you change the
buildbot config to send notification on build failure?  The same as
other builders do.

-- 
Yao (齐尧)

Re: [PATCH] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Christophe Lyon]

On 1 March 2018 at 21:46, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> On 2018-03-01 15:20, Sergio Durigan Junior wrote:
>>
>> commit b4987c956dfa44ca9fd8552f63e15f5fa094b2a4
>> Author: Sergio Durigan Junior <sergiodj@redhat.com>
>> Date:   Fri Feb 9 18:44:59 2018 -0500
>>
>>     Create new common/pathstuff.[ch]
>>
>> Introduced a regression when compiling for mingw*:
>>
>>   /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
>>   gdb_realpath(const char*)':
>>   /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in
>> this scope
>>        char buf[MAX_PATH];
>>                 ^
>>   /gdb/common/pathstuff.c:57:5: error: 'DWORD' was not declared in this
>> scope
>>        DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
>>        ^
>>   /gdb/common/pathstuff.c:57:11: error: expected ';' before 'len'
>>        DWORD len = GetFullPathName (filename, MAX_PATH, buf, NULL);
>>              ^
>>   /gdb/common/pathstuff.c:63:9: error: 'len' was not declared in this
>> scope
>>        if (len > 0 && len < MAX_PATH)
>>            ^
>>   /gdb/common/pathstuff.c:64:54: error: 'buf' was not declared in this
>> scope
>>          return gdb::unique_xmalloc_ptr<char> (xstrdup (buf));
>>                                                         ^
>>   make[2]: *** [pathstuff.o] Error 1
>>
>> The proper fix is to conditionally include "<windows.h>".  This commit
>> does that, without introducing any regressions as per tests made by
>> our BuildBot.
>>
>> gdb/ChangeLog:
>> 2018-03-01  Sergio Durigan Junior  <sergiodj@redhat.com>
>>
>>         PR gdb/22907
>>         * common/pathstuff.c: Conditionally include "<windows.h>".
>> ---
>>  gdb/common/pathstuff.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/gdb/common/pathstuff.c b/gdb/common/pathstuff.c
>> index fc574dc32e..8c4093fc38 100644
>> --- a/gdb/common/pathstuff.c
>> +++ b/gdb/common/pathstuff.c
>> @@ -23,6 +23,10 @@
>>  #include "filenames.h"
>>  #include "gdb_tilde_expand.h"
>>
>> +#ifdef USE_WIN32API
>> +#include <windows.h>
>> +#endif
>> +
>>  /* See common/pathstuff.h.  */
>>
>>  gdb::unique_xmalloc_ptr<char>
>
>
> Christopher, does that fix the issue for you?  If so, the patch LGTM.
>

Yes, I applied it to my gdb-8.1-branch , and the build now succeeds.
Thanks

> Simon

Re: [PATCH] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Sergio Durigan Junior]

On Friday, March 02 2018, Yao Qi wrote:

> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
>> Introduced a regression when compiling for mingw*:
>>
>>   /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
>>   gdb_realpath(const char*)':
>>   /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in this scope
>>        char buf[MAX_PATH];
>
> Fedora-x86_64-w64-mingw32 catches this build failure
> https://gdb-build.sergiodj.net/builders/Fedora-x86_64-w64-mingw32/builds/1113/steps/compile%20gdb/logs/stdio
> but it doesn't send notification on build failure.  Could you change the
> buildbot config to send notification on build failure?  The same as
> other builders do.

Yeah, it's on my TODO list.  I've also noticed this after Christophe
reported the failure.  Thanks, Yao.

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

Re: [PATCH] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Sergio Durigan Junior]

On Friday, March 02 2018, Christophe Lyon wrote:

> On 1 March 2018 at 21:46, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>> Christopher, does that fix the issue for you?  If so, the patch LGTM.
>>
>
> Yes, I applied it to my gdb-8.1-branch , and the build now succeeds.
> Thanks

Thanks, Christophe and Simon.

Pushed to master:

ab818ade016bcd794980438775e15c7a74f054f9

And to the 8.1 branch:

a74e30b243dec8b38c8c769da04635210f9ef986

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

Re: [PATCH] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Sergio Durigan Junior]

On Friday, March 02 2018, I wrote:

> On Friday, March 02 2018, Yao Qi wrote:
>
>> Sergio Durigan Junior <sergiodj@redhat.com> writes:
>>
>>> Introduced a regression when compiling for mingw*:
>>>
>>>   /gdb/common/pathstuff.c: In function 'gdb::unique_xmalloc_ptr<char>
>>>   gdb_realpath(const char*)':
>>>   /gdb/common/pathstuff.c:56:14: error: 'MAX_PATH' was not declared in this scope
>>>        char buf[MAX_PATH];
>>
>> Fedora-x86_64-w64-mingw32 catches this build failure
>> https://gdb-build.sergiodj.net/builders/Fedora-x86_64-w64-mingw32/builds/1113/steps/compile%20gdb/logs/stdio
>> but it doesn't send notification on build failure.  Could you change the
>> buildbot config to send notification on build failure?  The same as
>> other builders do.
>
> Yeah, it's on my TODO list.  I've also noticed this after Christophe
> reported the failure.  Thanks, Yao.

E-mail notifications enabled for this builder.

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/

Re: [PATCH] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Simon Marchi <simon.marchi@ericsson.com>,	Pedro Alves <palves@redhat.com>,	Joel Brobecker <brobecker@adacore.com>,	Christophe Lyon <christophe.lyon@linaro.org>,	Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Thu,  1 Mar 2018 15:20:05 -0500
> 
> +#ifdef USE_WIN32API
> +#include <windows.h>
> +#endif

Isn't USE_WIN32API specific to gdbserver?  If so, perhaps it's better
to use __MINGW32__ or _WIN32 instead?

Re: [PATCH] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Simon Marchi]

On 2018-03-02 08:32, Eli Zaretskii wrote:
>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: Simon Marchi <simon.marchi@ericsson.com>,	Pedro Alves 
>> <palves@redhat.com>,	Joel Brobecker 
>> <brobecker@adacore.com>,	Christophe Lyon 
>> <christophe.lyon@linaro.org>,	Sergio Durigan Junior 
>> <sergiodj@redhat.com>
>> Date: Thu,  1 Mar 2018 15:20:05 -0500
>> 
>> +#ifdef USE_WIN32API
>> +#include <windows.h>
>> +#endif
> 
> Isn't USE_WIN32API specific to gdbserver?  If so, perhaps it's better
> to use __MINGW32__ or _WIN32 instead?

I see it defined in both gdb and gdbserver:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/configure.ac;h=698fc7b83456f8c5a63ae0050dc8ec65069290f7;hb=HEAD#l1908
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/gdbserver/configure.ac;h=99801681ff47ee8dcd9ad2e5ae282dcd113c83e4;hb=HEAD#l281

Simon

Re: [PATCH] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Sergio Durigan Junior]

On Friday, March 02 2018, Simon Marchi wrote:

> On 2018-03-02 08:32, Eli Zaretskii wrote:
>>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>>> Cc: Simon Marchi <simon.marchi@ericsson.com>,	Pedro Alves
>>> <palves@redhat.com>,	Joel Brobecker <brobecker@adacore.com>,
>>> Christophe Lyon <christophe.lyon@linaro.org>,	Sergio Durigan
>>> Junior <sergiodj@redhat.com>
>>> Date: Thu,  1 Mar 2018 15:20:05 -0500
>>>
>>> +#ifdef USE_WIN32API
>>> +#include <windows.h>
>>> +#endif
>>
>> Isn't USE_WIN32API specific to gdbserver?  If so, perhaps it's better
>> to use __MINGW32__ or _WIN32 instead?
>
> I see it defined in both gdb and gdbserver:
>
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/configure.ac;h=698fc7b83456f8c5a63ae0050dc8ec65069290f7;hb=HEAD#l1908
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/gdbserver/configure.ac;h=99801681ff47ee8dcd9ad2e5ae282dcd113c83e4;hb=HEAD#l281

It's also present on common/filestuff.c, which is shared.  That's
actually where I took the idea from.

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

Re: [PATCH] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  gdb-patches@sourceware.org,  simon.marchi@ericsson.com,  palves@redhat.com,  brobecker@adacore.com,  christophe.lyon@linaro.org
> Date: Fri, 02 Mar 2018 13:20:38 -0500
> 
> >>> +#ifdef USE_WIN32API
> >>> +#include <windows.h>
> >>> +#endif
> >>
> >> Isn't USE_WIN32API specific to gdbserver?  If so, perhaps it's better
> >> to use __MINGW32__ or _WIN32 instead?
> >
> > I see it defined in both gdb and gdbserver:
> >
> > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/configure.ac;h=698fc7b83456f8c5a63ae0050dc8ec65069290f7;hb=HEAD#l1908
> > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/gdbserver/configure.ac;h=99801681ff47ee8dcd9ad2e5ae282dcd113c83e4;hb=HEAD#l281
> 
> It's also present on common/filestuff.c, which is shared.  That's
> actually where I took the idea from.

Those could be mistakes due to moving stuff into common/.  So I think
we should decide what that symbol means and how to use it, and then
see if the practice somehow deviates from those rules.

I asked my question because AFAIR this symbol used to be only in
gdbserver sources.  If nowadays we can use it anywhere, then I'd
suggest to replace it with something common with GDB, because the
meaning of USE_WIN32API in the context of GDB is unclear to me.

Re: [PATCH] Conditionally include "<windows.h>" on common/pathstuff.c (and unbreak build on mingw*)

[Yao Qi]

Sergio Durigan Junior <sergiodj@redhat.com> writes:

> E-mail notifications enabled for this builder.

Thank you, Sergio.

-- 
Yao (齐尧)