Re: [PATCH v6 3/4] Share fork_inferior et al with gdbserver

Sergio Durigan Junior
2017-05-31T03:43:49Z

Hey Pedro,

Sorry for the long delay in getting back to this. Comments below.

On Friday, May 05 2017, Pedro Alves wrote:

> On 05/04/2017 06:29 AM, Sergio Durigan Junior wrote:
>
>> diff --git a/gdb/common/common-inferior.h b/gdb/common/common-inferior.h
>> new file mode 100644
>> index 0000000..206a36a
>> --- /dev/null
>> +++ b/gdb/common/common-inferior.h
>> @@ -0,0 +1,75 @@
>> +/* Variables that describe the inferior process running under GDB and
>> + GDBserver: Where it is, why it stopped, and how to step it.
>
> Stale comment.

Replaced by:

/* Functions to deal with the inferior being executed on GDB or
GDBserver.

Sorry, couldn't think of a better description.

>> +
>> + Copyright (C) 1986-2017 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 COMMON_INFERIOR_H
>> +#define COMMON_INFERIOR_H
>> +
>> +/* Number of traps that happen between exec'ing the shell to run an
>> + inferior and when we finally get to the inferior code, not counting
>> + the exec for the shell. This is 1 on all supported
>> + implementations. */
>> +#define START_INFERIOR_TRAPS_EXPECTED 1
>
> The copy in gdb/inferior.h should be removed instead of being left
> duplicated.

Sorry, a mistake here. Removed the copy in gdb/inferior.h.

>> +
>> +/* Whether to start up the debuggee under a shell.
>> +
>> + If startup-with-shell is set, GDB's "run" will attempt to start up
>> + the debuggee under a shell. This also happens when using GDBserver
>> + under extended remote mode.
>> +
>> + This is in order for argument-expansion to occur. E.g.,
>> +
>> + (gdb) run *
>> +
>> + The "*" gets expanded by the shell into a list of files.
>> +
>> + While this is a nice feature, it may be handy to bypass the shell
>> + in some cases. To disable this feature, do "set startup-with-shell
>> + false".
>> +
>> + The catch-exec traps expected during start-up will be one more if
>> + the target is started up with a shell. */
>> +extern int startup_with_shell;
>> +
>> +/* Perform any necessary tasks before a fork/vfork takes place. ARGS
>> + is a string containing all the arguments received by the inferior.
>> + This function is mainly used by fork_inferior. */
>> +extern void prefork_hook (const char *args);
>> +
>> +/* Perform any necessary tasks after a fork/vfork takes place. This
>> + function is mainly used by fork_inferior. */
>> +extern void postfork_hook (pid_t pid);
>> +
>> +/* Perform any necessary tasks *on the child* after a fork/vfork takes
>> + place. DEBUG_FORK is the number of seconds that we should sleep
>> + before exec'ing (see fork_inferior).
>> +
>> + This function is mainly used by fork_inferior. */
>> +extern void postfork_child_hook (int debug_fork);
>
> Wouldn't it better to put these in nat/fork-inferior.h instead?
> Likewise START_INFERIOR_TRAPS_EXPECTED, maybe?

I remember struggling to decide where to put these functions. In the
end, I decided to put them on commom/common-inferior.h for a very good
reason that I now do not remember anymore :-/. Anyway, since I can't
really argue against your point, I will do what you're suggesting. Only
get_exec_wrapper and get_exec_file will remain in common-inferior.h

>> +
>> +/* Return the exec wrapper to be used when starting the inferior, or NULL
>> + otherwise. */
>> +extern const char *get_exec_wrapper (void);
>
> Should really drop the "(void)" in all new code throughout the series.

Oh, OK. I wasn't sure if we were following this practice for all
functions or only for methods.

>> +
>> +/* Return the name of the executable file as a string.
>> + ERR nonzero means get error if there is none specified;
>> + otherwise return 0 in that case. */
>> +extern char *get_exec_file (int err);
>> +
>> +#endif /* ! COMMON_INFERIOR_H */
>
>
>> +/* See common/common-utils.h. */
>> +
>> +std::string
>> +stringify_argv (const std::vector<char *> &args)
>> +{
>> + std::string ret ("");
>
> std::string ret;

Done.

>> +
>> + if (!args.empty ())
>> + {
>> + for (auto s : args)
>> + if (s != NULL)
>> + ret += s + std::string (" ");
>
> Avoid unnecessary temporaries:
>
> {
> ret += s;
> ret += ' ';
> }

Done.

>> +
>> + /* Erase the last whitespace. */
>> + ret.erase (ret.end () - 1);
>> + }
>> +
>> + return ret;
>> +}
>
>
>
>>
>> -/* Accept NTRAPS traps from the inferior. */
>> +/* See common/common-inferior.h. */
>>
>> void
>> -startup_inferior (int ntraps)
>> +postfork_child_hook (int debug_fork)
>> {
>> - int pending_execs = ntraps;
>> - int terminal_initted = 0;
>> - ptid_t resume_ptid;
>> -
>> - if (startup_with_shell)
>> - {
>> - /* One trap extra for exec'ing the shell. */
>> - pending_execs++;
>> - }
>> + /* This is set to the result of setpgrp, which if vforked, will be
>> + visible to you in the parent process. It's only used by humans
>> + for debugging. */
>> + static int debug_setpgrp = 657473;
>>
>> - if (target_supports_multi_process ())
>> - resume_ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
>> - else
>> - resume_ptid = minus_one_ptid;
>> + /* Make sure we switch to main_ui here in order to be able to
>> + use the fprintf_unfiltered/warning/error functions. */
>> + current_ui = main_ui;
>>
>> - /* The process was started by the fork that created it, but it will
>> - have stopped one instruction after execing the shell. Here we
>> - must get it up to actual execution of the real program. */
>> + /* Close all file descriptors except those that gdb inherited
>> + (usually 0/1/2), so they don't leak to the inferior. Note
>> + that this closes the file descriptors of all secondary
>> + UIs. */
>> + close_most_fds ();
>
> Any reason this closing isn't done in the common code instead of
> having it done on both hook implementations?

You're right, close_most_fds can be shared between them.

>>
>> - if (exec_wrapper)
>> - pending_execs++;
>> + if (debug_fork)
>> + sleep (debug_fork);
>
> Similarly, why not leave this sleep to the common code? It's
> missing on the gdbserver side, AFAICS.

Right, I clearly got confused about debug_fork and its usefulness on
gdbserver. Moved to common code, and removed the "debug_fork" parameter
from postfork_child_hook.

>> + case TARGET_WAITKIND_SPURIOUS:
>> + case TARGET_WAITKIND_LOADED:
>> + case TARGET_WAITKIND_FORKED:
>> + case TARGET_WAITKIND_VFORKED:
>> + case TARGET_WAITKIND_SYSCALL_ENTRY:
>> + case TARGET_WAITKIND_SYSCALL_RETURN:
>> + case TARGET_WAITKIND_VFORK_DONE:
>> + case TARGET_WAITKIND_IGNORE:
>> + case TARGET_WAITKIND_NO_HISTORY:
>> + case TARGET_WAITKIND_NO_RESUMED:
>> + case TARGET_WAITKIND_THREAD_CREATED:
>> + case TARGET_WAITKIND_THREAD_EXITED:
>
> You added several of these at the same time (but they don't really
> make sense here). Did you run into some of them in gdbserver?

No. At the time I thought it would be better to cover all the
TARGET_WAITKIND_* entries here. Some of them seemed to make sense to
me, like the TARGET_WAITKIND_THREAD_* ones.

Based on your comment, I removed the ones that I had added:

case TARGET_WAITKIND_VFORK_DONE:
case TARGET_WAITKIND_IGNORE:
case TARGET_WAITKIND_NO_HISTORY:
case TARGET_WAITKIND_NO_RESUMED:
case TARGET_WAITKIND_THREAD_CREATED:
case TARGET_WAITKIND_THREAD_EXITED:

>> index 4ea7913..8aa85db 100644
>> --- a/gdb/gdbserver/configure.ac
>> +++ b/gdb/gdbserver/configure.ac
>> @@ -462,7 +462,9 @@ esac],
>>
>> if $want_ipa ; then
>> if $have_ipa ; then
>> - IPA_DEPFILES="$ipa_obj"
>> + # Needed because safe_strerror's definition is host-dependent
>
> Why do we end up needing safe_strerror in the IPA in the first place?

This is needed because I moved the definition of
trace_start_error_with_name from the old gdb/fork-child.c to
common/common-utils.c. This function which uses safe_strerror, and
common/common-utils.c is compiled by IPA.

An option would be to keep these trace_start_error.* functions in
nat/fork-inferior.c, but I think it is more logical to keep them on
common-utils.c.

Now, I can obviously expand the comment if that's what you meant.

>> + strerror_obj="`echo $srv_host_obs | sed 's/\(.*-strerror\)\.o/\1-ipa.o/'`"
>> + IPA_DEPFILES="$ipa_obj $strerror_obj"
>> extra_libraries="$extra_libraries libinproctrace.so"
>> else
>> AC_MSG_ERROR([inprocess agent not supported for this target])
>
>
>
>> -#if defined(__UCLIBC__) && defined(HAS_NOMMU)
>> - pid = vfork ();
>
> Does fork_inferior end up always using vfork on no-MMU
> ports somehow?

Sorry, I am not sure. How would I go about finding that?

>
>> -#else
>> - pid = fork ();
>> -#endif
>
>> +const char *
>> +get_exec_wrapper (void)
>> {
>> - std::vector<char *> new_argv;
>> + static std::string ret;
>> + static bool initialized_p = false;
>>
>> - if (!wrapper_argv.empty ())
>> - new_argv.insert (new_argv.begin (),
>> - wrapper_argv.begin (),
>> - wrapper_argv.end ());
>> + if (wrapper_argv.empty ())
>> + return NULL;
>>
>> - for (int i = 0; argv[i] != NULL; ++i)
>> - new_argv.push_back (argv[i]);
>> + if (!initialized_p)
>> + {
>> + for (auto s : wrapper_argv)
>> + ret += s + std::string (" ");
>
> Avoid unnecessary temporaries.

Done.

>>
>> - new_argv.push_back (NULL);
>> + /* Erase the last whitespace. */
>> + ret.erase (ret.end () - 1);
>> +
>> + initialized_p = true;
>> + }
>> +
>> + return ret.c_str ();
>> +}
>
> I'm actually having a bit of trouble understanding
> why do we need the wrapper_argv global plus this initialized_p
> indirection, instead of just building the exec wrapper string
> in captured_main.

Alright, I'm now using a global std::string wrapper_argv which is
constructed on captured_main, and get_exec_wrapper now just returns the
c_str () version of it, or NULL if the wrapper_argv.size () is zero.

>
>> @@ -2848,6 +2893,7 @@ handle_v_run (char *own_buf)
>> {
>
>
>> + /* FIXME: Fail request if out of memory instead of dying. */
>> size_t len = (next_p - p) / 2;
>
> Did you really mean to add that comment?

No. Removed.

>
>> diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
>> index d5fee38..5087614 100644
>> --- a/gdb/gdbserver/server.h
>> +++ b/gdb/gdbserver/server.h
>> @@ -132,6 +132,7 @@ extern int in_queued_stop_replies (ptid_t ptid);
>> #include "utils.h"
>> #include "debug.h"
>> #include "gdb_vecs.h"
>> +#include <vector>
>>
>
> Why?

Leftover from a previous patch. Removed.

>> /* Maximum number of bytes to read/write at once. The value here
>> is chosen to fill up a packet (the headers account for the 32). */
>> @@ -148,4 +149,13 @@ extern int in_queued_stop_replies (ptid_t ptid);
>> /* Definition for any syscall, used for unfiltered syscall reporting. */
>> #define ANY_SYSCALL (-2)
>>
>> +/* After fork_inferior has been called, we need to adjust a few
>> + signals and call startup_inferior. This is done here. PID is the
>> + pid of the new inferior, and PROGRAM is its name. */
>> +extern void post_fork_inferior (int pid, const char *program);
>> +
>> +/* Get the 'struct gdb_environ *' being used in the current
>> + session. */
>> +extern struct gdb_environ *get_environ (void);
>> +
>> #endif /* SERVER_H */
>
>> +
>> +/* See target/target.h. */
>> +
>> +void
>> +target_terminal_init (void)
>> +{
>> + /* To be implemented. */
>> +}
>> +
>> +/* See target/target.h. */
>> +
>> +void
>> +target_terminal_inferior (void)
>> +{
>> + /* To be implemented. */
>> +}
>> +
>> +/* See target/target.h. */
>> +
>> +void
>> +target_terminal_ours (void)
>> +{
>> + /* To be implemented. */
>> +}
>
> s/To be implemented/Not necessary/ or some such.

Changed to:

/* Placeholder needed because of fork_inferior. Not necessary on
GDBserver. */

>> diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
>> index b4ded31..48804c5 100644
>> --- a/gdb/gdbserver/utils.h
>> +++ b/gdb/gdbserver/utils.h
>> @@ -19,6 +19,8 @@
>> #ifndef UTILS_H
>> #define UTILS_H
>>
>> +#include <vector>
>> +
>
> Why?

Sigh... Leftover. Removed. Thanks, and sorry.

>> char *paddress (CORE_ADDR addr);
>> char *pfildes (gdb_fildes_t fd);
>>
>
>> static int
>> -win32_create_inferior (char *program, char **program_args)
>> +win32_create_inferior (const char *program,
>> + const std::vector<char *> &program_args)
>> {
>> #ifndef USE_WIN32API
>> char real_path[PATH_MAX];
>> @@ -627,6 +627,8 @@ win32_create_inferior (char *program, char **program_args)
>> int argc;
>> PROCESS_INFORMATION pi;
>> DWORD err;
>> + std::string str_program_args = stringify_argv (program_argv);
>> + char *args = (char *) str_program_args.c_str ();
>
> Can it be const?

It can, but then I'd have to cast it to "char *" when passing to
create_process. CreateProcessA's second argument is of the type LPSTR,
which translates to "char *". So I'd rather keep args as "char *".

>> +
>> +/* When executing a command under the given shell, return non-zero if
>> + the '!' character should be escaped when embedded in a quoted
>> + command-line argument. */
>> +
>> +static int
>> +escape_bang_in_quoted_argument (const char *shell_file)
>> +{
>> + const int shell_file_len = strlen (shell_file);
>> +
>> + /* Bang should be escaped only in C Shells. For now, simply check
>> + that the shell name ends with 'csh', which covers at least csh
>> + and tcsh. This should be good enough for now. */
>> +
>> + if (shell_file_len < 3)
>> + return 0;
>> +
>> + if (shell_file[shell_file_len - 3] == 'c'
>> + && shell_file[shell_file_len - 2] == 's'
>> + && shell_file[shell_file_len - 1] == 'h')
>> + return 1;
>> +
>> + return 0;
>> +}
>
> This function lost some edits done in the execv_argv patch.
>
> bool + size_t.
>
> Please run a diff of the old gdb/fork-child.c against the
> new nat/fork-inferior.c to make sure nothing else was lost.

Thanks, I'll make sure that nothing else is missed.

>
>> diff --git a/gdb/nat/fork-inferior.h b/gdb/nat/fork-inferior.h
>> new file mode 100644
>> index 0000000..6a8528e
>> --- /dev/null
>> +++ b/gdb/nat/fork-inferior.h
>> @@ -0,0 +1,51 @@
> ...
>
>> +
>> +#ifndef FORK_CHILD_H
>> +#define FORK_CHILD_H
>
> Incorrectly named multiple-inclusion guard.

Fixed.

>> +
>> +#include <vector>
>
> Why? Nothing uses std::vector here.

Removed.

>> +
>> +/* Accept NTRAPS traps from the inferior.
>> +
>> + Return the ptid of the inferior being started. */
>> +extern ptid_t startup_inferior (pid_t pid, int ntraps,
>> + struct target_waitstatus *mystatus,
>> + ptid_t *myptid);
>> +
>> +/* Start an inferior Unix child process and sets inferior_ptid to its
>> + pid. EXEC_FILE is the file to run. ALLARGS is a string containing
>> + the arguments to the program. ENV is the environment vector to
>> + pass. SHELL_FILE is the shell file, or NULL if we should pick
>> + one. EXEC_FUN is the exec(2) function to use, or NULL for the default
>> + one. */
>> +
>> +/* This function is NOT reentrant. Some of the variables have been
>> + made static to ensure that they survive the vfork call. */
>> +extern pid_t fork_inferior (const char *exec_file_arg,
>> + const std::string &allargs,
>
> Should include <string>.

Should it? It's compiling fine without it. I included just for the
sake of clarity anyway.

>> + char **env, void (*traceme_fun) (void),
>> + void (*init_trace_fun) (int),
>> + void (*pre_trace_fun) (void),
>> + const char *shell_file_arg,
>> + void (*exec_fun) (const char *file,
>> + char * const *argv,
>> + char * const *env));
>> +
>> +#endif /* ! FORK_CHILD_H */
>
>>
>> diff --git a/gdb/target.h b/gdb/target.h
>> index a971adf..6c995a1 100644
>> --- a/gdb/target.h
>> +++ b/gdb/target.h
>> @@ -1538,17 +1538,6 @@ extern int target_terminal_is_inferior (void);
>>
>> extern int target_terminal_is_ours (void);
>>
>> -/* Initialize the terminal settings we record for the inferior,
>> - before we actually run the inferior. */
>> -
>> -extern void target_terminal_init (void);
>> -
>> -/* Put the inferior's terminal settings into effect. This is
>> - preparation for starting or resuming the inferior. This is a no-op
>> - unless called with the main UI as current UI. */
>> -
>> -extern void target_terminal_inferior (void);
>> -
>> /* Put some of our terminal settings into effect, enough to get proper
>> results from our output, but do not change into or out of RAW mode
>> so that no input is discarded. This is a no-op if terminal_ours
>> @@ -1557,12 +1546,6 @@ extern void target_terminal_inferior (void);
>>
>> extern void target_terminal_ours_for_output (void);
>>
>> -/* Put our terminal settings into effect. First record the inferior's
>> - terminal settings so they can be restored properly later. This is
>> - a no-op unless called with the main UI as current UI. */
>> -
>> -extern void target_terminal_ours (void);
>> -
>
> Please leave breakcrumb comments behind.

Will do.

Now, something that came up while I was testing things with mingw here.
gdb/gdbserver/server.c now calls startup_inferior (define in
nat/fork-inferior.c) directly, instead of doing things on its own. This
is one of the goals of this series, but for targets that don't compile
fork-inferior.c (like Windows) this is an obvious problem. So here's
what I'm doing for the new version of the series: I'll move
startup_inferior to common/ (probably common/common-fork-inferior.c or
some such), so that all targets can have access to it (it's
target-agnostic anyway). If you have any comments about this, please
let me know.

Expect a new version of the series tomorrow.

Thanks,

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