[RFC] "gdbserver ... BASENAME_EXE" no longer works (was: "[PATCH 5/6] Share fork_inferior et al with gdbserver")

[Joel Brobecker <brobecker at adacore dot com>]

Hello,

I just noticed that the following patch...

    commit 2090129c36c7e582943b7d300968d19b46160d84
    Date:   Thu Dec 22 21:11:11 2016 -0500
    Subject: Share fork_inferior et al with gdbserver

... caused a change of behavior in GDBserver, where the following
no longer works (unless '.' is in your PATH, but for me, that's
not a good idea):

    $ gdbserver  --once :4444 simple_main
    zsh:1: command not found: simple_main
    During startup program exited with code 127.
    Exiting

Prior to the change we we able to start simple_main without problems:

    $ gdbserver --once :4444 simple_main
    Process simple_main created; pid = 26579
    Listening on port 4444

Was that intentional? Reading the revision log, there is no mention
of this, and this was a fairly natural thing to be doing. This also
matches something we do with GDBserver as well, so it would make
the two tools consistent in that regard.

Attached is a preliminary hack meant to help me explore what would
be needed to bring this feature back. If we agree we want the feature
back, and that I'm doing it at the right location, I'll split it,
finish the C++-ification of the execv_argv class, and then resubmit
as an official RFA.

Comments on the code welcome, of course! (I feel like a C++ dummy,
sometimes).

Thanks!

On Thu, Dec 22, 2016 at 10:39:20PM -0500, Sergio Durigan Junior wrote:
> This is the most important (and the biggest, sorry) patch of the
> series.  It moves fork_inferior from gdb/fork-child.c to
> common/common-fork-child.c and makes all the necessary adjustments to
> both GDB and gdbserver to make sure everything works OK.
> [...]

-- 
Joel

>From 2f13f758b07ec2f65892f3708b5f44a922fceef8 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 20 Feb 2018 17:20:08 +0400
Subject: [PATCH] WIP: GDBserver no longer working when given an executable's
 basename

Recent versions of GDB are no longer able to start an executable
when the path to that executable is a basename. Eg:

    $ gdbserver  --once :4444 simple_main
    zsh:1: command not found: simple_main
    During startup program exited with code 127.
    Exiting

This is a regression which was introduced by the following commit:

    commit 2090129c36c7e582943b7d300968d19b46160d84
    Date:   Thu Dec 22 21:11:11 2016 -0500
    Subject: Share fork_inferior et al with gdbserver

Prior to this patch, each host that GDBserver was supporting
had its own create_inferior code, but the code in linux-low.c
prior to the change gives the idea of what one might be doing:

      execv (program, allargs);
      if (errno == ENOENT)
        execvp (program, allargs);

First we do an "execv", which doesn't search the path, but just
uses the program name as is. This is how our scenario used to work.
But if the executable couldn't be located with just the executable
name, then we'd try with execvp, which searches the PATH.

Today, this code has been factorized into a common implementation
of fork_inferior, which basically does:

      if (exec_fun != NULL)
        (*exec_fun) (argv[0], &argv[0], env);
      else
        execvp (argv[0], &argv[0]);

(The "exec_fun" case is only used in the case of Darwin, where
we have to provide the equivalent of the execvp function).

One could feel tempted to implement the same kind of approach
in fork_inferior, but it's not going to work, because we now
support running the inferior through the shell (so the execv
would apply to the shell, not the inferior).

Instead, what this patch does is explicitly handle the case where
the executable name is a basename, and that filename exists in
the current working directory - in that case, we implicitly
prepend that working directory and use that to execute it.

This is only a prototype, as you will see that there is a bit of
a C++-ification happening in execv_argv to make memory management
a little easier. If the principle of the patch is accepted, I will
likely split the patches in two, and also probably C++-ify the
code that quotes the arguments.

This patch has been validated on x86_64-linux, using both
the official testsuite as well as AdaCore's testsuite.
We ran the testsuite first in standalone native mode, and
then using the native-gdbserver board file.

Change-Id: I97268aacc724e07b3f8aff8d69b59c9f65615257
---
 gdb/common/filestuff.c  | 31 ++++++++++++++++++++
 gdb/common/filestuff.h  |  6 ++++
 gdb/nat/fork-inferior.c | 75 ++++++++++++++++++++++++++++++++++---------------
 gdb/source.c            | 33 ----------------------
 4 files changed, 90 insertions(+), 55 deletions(-)

diff --git a/gdb/common/filestuff.c b/gdb/common/filestuff.c
index f5a754ffa66..e6e046dea12 100644
--- a/gdb/common/filestuff.c
+++ b/gdb/common/filestuff.c
@@ -145,6 +145,37 @@ fdwalk (int (*func) (void *, int), void *arg)
 
 #endif /* HAVE_FDWALK */
 
+/* See filestuff.h.  */
+
+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;
+}
+
 
 
 /* A vector holding all the fds open when notice_open_fds was called.  We
diff --git a/gdb/common/filestuff.h b/gdb/common/filestuff.h
index 92a2a5f4c70..64a0b963308 100644
--- a/gdb/common/filestuff.h
+++ b/gdb/common/filestuff.h
@@ -19,6 +19,12 @@
 #ifndef FILESTUFF_H
 #define FILESTUFF_H
 
+/* 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 int is_regular_file (const char *name, int *errno_ptr);
+
 /* Note all the file descriptors which are open when this is called.
    These file descriptors will not be closed by close_most_fds.  */
 
diff --git a/gdb/nat/fork-inferior.c b/gdb/nat/fork-inferior.c
index 8b59387fa5a..4f089121fb9 100644
--- a/gdb/nat/fork-inferior.c
+++ b/gdb/nat/fork-inferior.c
@@ -27,6 +27,8 @@
 #include "signals-state-save-restore.h"
 #include "gdb_tilde_expand.h"
 #include <vector>
+#include "filenames.h"
+#include "host-defs.h"
 
 extern char **environ;
 
@@ -42,7 +44,7 @@ public:
   /* EXEC_FILE is the file to run.  ALLARGS is a string containing the
      arguments to the program.  If starting with a shell, SHELL_FILE
      is the shell to run.  Otherwise, SHELL_FILE is NULL.  */
-  execv_argv (const char *exec_file, const std::string &allargs,
+  execv_argv (const std::string &exec_file, const std::string &allargs,
 	      const char *shell_file);
 
   /* Return a pointer to the built argv, in the type expected by
@@ -63,12 +65,12 @@ private:
 
   /* Used when building an argv for a straight execv call, without
      going via the shell.  */
-  void init_for_no_shell (const char *exec_file,
+  void init_for_no_shell (const std::string &exec_file,
 			  const std::string &allargs);
 
   /* Used when building an argv for execing a shell that execs the
      child program.  */
-  void init_for_shell (const char *exec_file,
+  void init_for_shell (const std::string &exec_file,
 		       const std::string &allargs,
 		       const char *shell_file);
 
@@ -93,7 +95,7 @@ private:
    M_STORAGE.  */
 
 void
-execv_argv::init_for_no_shell (const char *exec_file,
+execv_argv::init_for_no_shell (const std::string &exec_file,
 			       const std::string &allargs)
 {
 
@@ -103,7 +105,7 @@ execv_argv::init_for_no_shell (const char *exec_file,
      allocations and string dups when 1 is sufficient.  */
   std::string &args_copy = m_storage = allargs;
 
-  m_argv.push_back (exec_file);
+  m_argv.push_back (exec_file.c_str ());
 
   for (size_t cur_pos = 0; cur_pos < args_copy.size ();)
     {
@@ -163,7 +165,7 @@ escape_bang_in_quoted_argument (const char *shell_file)
 
 /* See declaration.  */
 
-execv_argv::execv_argv (const char *exec_file,
+execv_argv::execv_argv (const std::string &exec_file,
 			const std::string &allargs,
 			const char *shell_file)
 {
@@ -176,7 +178,7 @@ execv_argv::execv_argv (const char *exec_file,
 /* See declaration.  */
 
 void
-execv_argv::init_for_shell (const char *exec_file,
+execv_argv::init_for_shell (const std::string &exec_file,
 			    const std::string &allargs,
 			    const char *shell_file)
 {
@@ -205,7 +207,7 @@ execv_argv::init_for_shell (const char *exec_file,
      on IRIX 4.0.1 can't deal with it.  So we only quote it if we need
      to.  */
   bool need_to_quote;
-  const char *p = exec_file;
+  const char *p = exec_file.c_str();
   while (1)
     {
       switch (*p)
@@ -239,7 +241,7 @@ execv_argv::init_for_shell (const char *exec_file,
   if (need_to_quote)
     {
       shell_command += '\'';
-      for (p = exec_file; *p != '\0'; ++p)
+      for (p = exec_file.c_str (); *p != '\0'; ++p)
 	{
 	  if (*p == '\'')
 	    shell_command += "'\\''";
@@ -295,7 +297,7 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
   /* Set debug_fork then attach to the child while it sleeps, to debug.  */
   int debug_fork = 0;
   const char *shell_file;
-  const char *exec_file;
+  std::string exec_file;
   char **save_our_env;
   int i;
   int save_errno;
@@ -309,6 +311,47 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
   else
     exec_file = exec_file_arg;
 
+  /* Check if the user wants to set a different working directory for
+     the inferior.  */
+  inferior_cwd = get_inferior_cwd ();
+
+  if (inferior_cwd != NULL)
+    {
+      /* Expand before forking because between fork and exec, the child
+	 process may only execute async-signal-safe operations.  */
+      expanded_inferior_cwd = gdb_tilde_expand (inferior_cwd);
+      inferior_cwd = expanded_inferior_cwd.c_str ();
+    }
+
+  /* If EXEC_FILE is a basename, and that executable can be found in
+     the current working directory, then assume this is the executable
+     we want to run (that is, do not search the PATH).  This ensures
+     we preserve the traditional behavior where "gdb BASENAME_EXE" or
+     "gdbserver BASENAME_EXE", where BASENAME_EXE is the basename of
+     an executable, executes the BASENAME_EXE from the current working
+     directory, instead of either finding it on the PATH, or causing
+     an error because it could not be found in the PATH.  */
+  if (exec_file == lbasename (exec_file.c_str ()))
+    {
+      const char *base_dir = inferior_cwd;
+      if (base_dir == NULL)
+	base_dir = getcwd (NULL, 0);
+      if (base_dir != NULL)
+	{
+	  std::string exec_fullpath (base_dir);
+	  if (! IS_DIR_SEPARATOR (base_dir[strlen (base_dir) - 1]))
+	    exec_fullpath.append(SLASH_STRING);
+	  exec_fullpath.append(exec_file);
+
+	  int unused_errno;
+	  if (is_regular_file (exec_fullpath.c_str(), &unused_errno))
+	    exec_file = exec_fullpath;
+	}
+      else
+	warning (_("Error finding name of working directory: %s"),
+		 safe_strerror (errno));
+    }
+
   /* 'startup_with_shell' is declared in inferior.h and bound to the
      "set startup-with-shell" option.  If 0, we'll just do a
      fork/exec, no shell, so don't bother figuring out what shell.  */
@@ -342,18 +385,6 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
      the parent and child flushing the same data after the fork.  */
   gdb_flush_out_err ();
 
-  /* Check if the user wants to set a different working directory for
-     the inferior.  */
-  inferior_cwd = get_inferior_cwd ();
-
-  if (inferior_cwd != NULL)
-    {
-      /* Expand before forking because between fork and exec, the child
-	 process may only execute async-signal-safe operations.  */
-      expanded_inferior_cwd = gdb_tilde_expand (inferior_cwd);
-      inferior_cwd = expanded_inferior_cwd.c_str ();
-    }
-
   /* If there's any initialization of the target layers that must
      happen to prepare to handle the child we're about fork, do it
      now...  */
diff --git a/gdb/source.c b/gdb/source.c
index c6cb860fa64..e1281f978bb 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -669,39 +669,6 @@ info_source_command (const char *ignore, int from_tty)
 }
 
 
-/* Return True if the file NAME exists and is a regular file.
-   If the result is false then *ERRNO_PTR is set to a useful value assuming
-   we're expecting a regular file.  */
-
-static int
-is_regular_file (const char *name, int *errno_ptr)
-{
-  struct stat st;
-  const int status = stat (name, &st);
-
-  /* Stat should never fail except when the file does not exist.
-     If stat fails, analyze the source of error and return True
-     unless the file does not exist, to avoid returning false results
-     on obscure systems where stat does not work as expected.  */
-
-  if (status != 0)
-    {
-      if (errno != ENOENT)
-	return 1;
-      *errno_ptr = ENOENT;
-      return 0;
-    }
-
-  if (S_ISREG (st.st_mode))
-    return 1;
-
-  if (S_ISDIR (st.st_mode))
-    *errno_ptr = EISDIR;
-  else
-    *errno_ptr = EINVAL;
-  return 0;
-}
-
 /* Open a file named STRING, searching path PATH (dir names sep by some char)
    using mode MODE in the calls to open.  You cannot use this function to
    create files (O_CREAT).
-- 
2.11.0