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

Pedro Alves
2017-05-05T19:05:04Z

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.

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

> +
> +/* 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?

> +
> +/* 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.

> +
> +/* 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;

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

Avoid unnecessary temporaries:

{
ret += s;
ret += ' ';
}

> +
> + /* 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?

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

> + 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?

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

> + 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?

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

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

> @@ -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?

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

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

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

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

> +
> +/* 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.

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

> +
> +#include <vector>

Why? Nothing uses std::vector here.

> +
> +/* 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>.

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

> /* Return true if the target stack has a non-default
> "to_terminal_ours" method. */
>

Thanks,
Pedro Alves