From patchwork Wed Feb 21 03:58:27 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Brobecker X-Patchwork-Id: 25986 Received: (qmail 37151 invoked by alias); 21 Feb 2018 03:58:36 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 37141 invoked by uid 89); 21 Feb 2018 03:58:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=Stat X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 21 Feb 2018 03:58:34 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 736D811621B; Tue, 20 Feb 2018 22:58:32 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id f+Rs6+VI9sAh; Tue, 20 Feb 2018 22:58:32 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id B56B5116210; Tue, 20 Feb 2018 22:58:31 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 5D0E283054; Wed, 21 Feb 2018 07:58:27 +0400 (+04) Date: Wed, 21 Feb 2018 07:58:27 +0400 From: Joel Brobecker To: Sergio Durigan Junior Cc: GDB Patches , palves@redhat.com Subject: [RFC] "gdbserver ... BASENAME_EXE" no longer works (was: "[PATCH 5/6] Share fork_inferior et al with gdbserver") Message-ID: <20180221035827.ae265ol4k5jthhp2@adacore.com> References: <1482464361-4068-1-git-send-email-sergiodj@redhat.com> <1482464361-4068-6-git-send-email-sergiodj@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1482464361-4068-6-git-send-email-sergiodj@redhat.com> User-Agent: NeoMutt/20170113 (1.7.2) 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. > [...] From 2f13f758b07ec2f65892f3708b5f44a922fceef8 Mon Sep 17 00:00:00 2001 From: Joel Brobecker 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 +#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