Re: [PATCH v4 4/5] Share fork_inferior et al with gdbserver

Pedro Alves
2017-03-13T17:04:50Z

Hi Sergio,

[I think at least some of my comments I sent earlier
today on v3 still apply, so I won't repeat them.]

On 03/08/2017 05:29 AM, Sergio Durigan Junior wrote:

> diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
> index c9d03a8..74d5269 100644
> --- a/gdb/gdbserver/inferiors.c
> +++ b/gdb/gdbserver/inferiors.c
> @@ -29,6 +29,10 @@ struct thread_info *current_thread;
>
> #define get_thread(inf) ((struct thread_info *)(inf))
>
> +/* Even though we do not use/need INFERIOR_PTID here, we still have to
> + declare it because fork_inferior uses it directly. */
> +ptid_t inferior_ptid;
> +

This is the bit that I hope we can get rid of. Can we try
poking at it a bit more?

> void
> add_inferior_to_list (struct inferior_list *list,
> struct inferior_list_entry *new_inferior)
> @@ -469,6 +473,25 @@ make_cleanup_restore_current_thread (void)
> return make_cleanup (do_restore_current_thread_cleanup, current_thread);
> }
>
> +/* See common/common-inferior.h. */
> +
> +void
> +inferior_appeared (struct inferior *inf, int pid)
> +{
> + /* Placeholder needed for fork_inferior. We do not have to do
> + anything in this case. */
> +}
> +
> +/* See common/common-inferior.h. */
> +
> +struct inferior *
> +current_inferior (void)
> +{
> + /* Placeholder needed for fork_inferior. GDBserver has other
> + functions that serve this purpose. */
> + return NULL;
> +}
> +

And these, which I think are related.

> /* See common/common-gdbthread.h. */
>
> void
> @@ -499,3 +522,11 @@ add_thread_silent (ptid_t ptid)
>
> return add_thread (ptid_build (pid, pid, 0), NULL);
> }
> +
> +/* See common/common-inferior.h. */
> +
> +int
> +have_inferiors (void)
> +{
> + return get_first_process () != NULL;
> +}

> diff --git a/gdb/gdbserver/nto-low.c b/gdb/gdbserver/nto-low.c
> index 6229b4c..8cee363 100644
> --- a/gdb/gdbserver/nto-low.c
> +++ b/gdb/gdbserver/nto-low.c
> @@ -347,14 +347,15 @@ nto_read_auxv_from_initial_stack (CORE_ADDR initial_stack,
> return len_read;
> }
>
> -/* Start inferior specified by PROGRAM passing arguments ALLARGS. */
> +/* Start inferior specified by PROGRAM_ARGV. */
>
> static int
> -nto_create_inferior (char *program, char **allargs)
> +nto_create_inferior (char *program, const std::vector<char *> &program_argv)

The change to the comment no longer reflects the code change.
Spotted a few other places with the same. It'll be worth it
to go over the whole patch and make sure those are kept consistent.
But see further below before embarking in that.

> {
> struct inheritance inherit;
> pid_t pid;
> sigset_t set;
> + std::string program_args = stringify_argv (program_argv);
>
> TRACE ("%s %s\n", __func__, program);
> /* Clear any pending SIGUSR1's but keep the behavior the same. */
> @@ -367,7 +368,7 @@ nto_create_inferior (char *program, char **allargs)
> memset (&inherit, 0, sizeof (inherit));
> inherit.flags |= SPAWN_SETGROUP | SPAWN_HOLD;
> inherit.pgroup = SPAWN_NEWPGROUP;
> - pid = spawnp (program, 0, NULL, &inherit, allargs, 0);
> + pid = spawnp (program, 0, NULL, &inherit, (char *) program_args.c_str (), 0);
> sigprocmask (SIG_BLOCK, &set, NULL);
>
> if (pid == -1)
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c

> @@ -2862,62 +2873,93 @@ handle_v_run (char *own_buf)
> new_argc++;
> }
>
> - new_argv = (char **) calloc (new_argc + 2, sizeof (char *));
> - if (new_argv == NULL)
> - {
> - write_enn (own_buf);
> - return 0;
> - }
> -
> - i = 0;
> - for (p = own_buf + strlen ("vRun;"); *p; p = next_p)
> + for (i = 0, p = own_buf + strlen ("vRun;"); *p; p = next_p, ++i)
> {
> next_p = strchr (p, ';');
> if (next_p == NULL)
> next_p = p + strlen (p);
>
> - if (i == 0 && p == next_p)
> - new_argv[i] = NULL;
> + if (p == next_p)
> + new_argv.push_back ("''");
> else
> {
> /* FIXME: Fail request if out of memory instead of dying. */
> - new_argv[i] = (char *) xmalloc (1 + (next_p - p) / 2);
> - hex2bin (p, (gdb_byte *) new_argv[i], (next_p - p) / 2);
> - new_argv[i][(next_p - p) / 2] = '\0';
> + size_t len = 1 + (next_p - p) / 2;
> + char *s = (char *) xmalloc (len);
> + char *ss = (char *) xmalloc (len * 2);
> + char *tmp_s, *tmp_ss;
> + int need_quote;
> +
> + hex2bin (p, (gdb_byte *) s, (next_p - p) / 2);
> + s[(next_p - p) / 2] = '\0';
> +
> + tmp_s = s;
> + tmp_ss = ss;
> + need_quote = 0;
> + while (*tmp_s != '\0')
> + {
> + switch (*tmp_s)
> + {
> + case '\n':
> + *tmp_ss = '\'';
> + ++tmp_ss;
> + need_quote = 1;
> + break;
> +
> + case '\'':
> + *tmp_ss = '\\';
> + ++tmp_ss;
> + break;
> +
> + default:
> + break;
> + }
> +
> + *tmp_ss = *tmp_s;
> + ++tmp_ss;
> + ++tmp_s;
> + }
> +
> + if (need_quote)
> + *tmp_ss++ = '\'';
> +
> + *tmp_ss = '\0';
> + new_argv.push_back (ss);
> + xfree (s);
> }
>
> if (*next_p)
> next_p++;
> - i++;
> }
> - new_argv[i] = NULL;
>
> - if (new_argv[0] == NULL)
> + if (new_argv.empty () || new_argv[0] == NULL)
> {
> /* GDB didn't specify a program to run. Use the program from the
> last run with the new argument list. */
>
> - if (program_argv == NULL)
> + if (program_argv.empty ())
> {
> write_enn (own_buf);
> - freeargv (new_argv);
> + free_vector_argv (new_argv);
> return 0;
> }
>
> - new_argv[0] = strdup (program_argv[0]);
> - if (new_argv[0] == NULL)
> + new_argv.push_back (strdup (program_argv[0]));
> + if (new_argv.empty () || new_argv[0] == NULL)

On the "empty()" check: you've just pushed an element to the vector,
so the vector can't be empty.

On the "== NULL" check: IIUC, the old NULL check was there to
handle strdup returning NULL due to out-of-memory.
See NULL checks and comments further above in this function.
Now that you're using a std::vector, that doesn't work or make
sense any longer, since if push_back fails to allocate space for
its internal buffer (with operator new), our operator new replacement
(common/new-op.c) calls malloc_failure, which aborts gdbserver.

Not sure it makes sense to handle out-of-memory specially in
the gdb/rsp-facing functions nowadays (maybe git blame/log/patch
submission for that code shows some guidelines). Maybe (or, probably)
it's OK to stop caring about it, but then we should consistently remove
left over code, by using xstrdup instead and remove the NULL checks.

> {
> write_enn (own_buf);
> - freeargv (new_argv);
> + free_vector_argv (new_argv);
> return 0;
> }
> }
>
> /* Free the old argv and install the new one. */
> - freeargv (program_argv);
> + free_vector_argv (program_argv);
> program_argv = new_argv;
>
> - start_inferior (program_argv);
> + create_inferior (program_argv[0],
> + std::vector<char *> (program_argv.begin () + 1,
> + program_argv.end ()));

This creates a copy of the vector. How about keeping program_argv[0]
as a separate variable to avoid this copy? I suspect it'd simplify
some other things in the function.

> if (last_status.kind == TARGET_WAITKIND_STOPPED)
> {
> prepare_resume_reply (own_buf, last_ptid, &last_status);
> @@ -3535,13 +3577,18 @@ captured_main (int argc, char *argv[])
> multi_mode = 1;
> else if (strcmp (*next_arg, "--wrapper") == 0)
> {
> + char **tmp;
> +
> next_arg++;
>
> - wrapper_argv = next_arg;
> + tmp = next_arg;
> while (*next_arg != NULL && strcmp (*next_arg, "--") != 0)
> - next_arg++;
> + {
> + wrapper_argv.push_back (*next_arg);
> + next_arg++;
> + }
>
> - if (next_arg == wrapper_argv || *next_arg == NULL)
> + if (next_arg == tmp || *next_arg == NULL)
> {
> gdbserver_usage (stderr);
> exit (1);
> @@ -3672,8 +3719,13 @@ captured_main (int argc, char *argv[])
> exit (1);
> }
>
> + /* Gather information about the environment. */
> + our_environ = make_environ ();
> + init_environ (our_environ);
> +
> initialize_async_io ();
> initialize_low ();
> + have_job_control ();
> initialize_event_loop ();
> if (target_supports_tracepoints ())
> initialize_tracepoint ();
> @@ -3687,13 +3739,13 @@ captured_main (int argc, char *argv[])
> int i, n;
>
> n = argc - (next_arg - argv);
> - program_argv = XNEWVEC (char *, n + 1);
> for (i = 0; i < n; i++)
> - program_argv[i] = xstrdup (next_arg[i]);
> - program_argv[i] = NULL;
> + program_argv.push_back (xstrdup (next_arg[i]));
>
> /* Wait till we are at first instruction in program. */
> - start_inferior (program_argv);
> + create_inferior (program_argv[0],
> + std::vector<char *> (program_argv.begin () + 1,
> + program_argv.end ()));

Ditto.

>
> /* We are now (hopefully) stopped at the first instruction of
> the target process. This assumes that the target process was
> @@ -4308,9 +4360,11 @@ process_serial_event (void)
> fprintf (stderr, "GDBserver restarting\n");
>
> /* Wait till we are at 1st instruction in prog. */
> - if (program_argv != NULL)
> + if (!program_argv.empty ())
> {
> - start_inferior (program_argv);
> + create_inferior (program_argv[0],
> + std::vector<char *> (program_argv.begin () + 1,
> + program_argv.end ()));
> if (last_status.kind == TARGET_WAITKIND_STOPPED)
> {
> /* Stopped at the first instruction of the target
> diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h
> index d5fee38..5b65597 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>
>
> /* 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,20 @@ extern int in_queued_stop_replies (ptid_t ptid);
> /* Definition for any syscall, used for unfiltered syscall reporting. */
> #define ANY_SYSCALL (-2)
>
> +/* Any pre-processing needed to be done before calling fork_inferior
> + shall be implemented here. ARGV is a vector containing the full
> + argv of the inferior. */
> +extern void pre_fork_inferior (const std::vector<char *> &argv);
> +
> +/* 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 ARGV is the vector containing the full
> + argv of the inferior. */
> +extern void post_fork_inferior (int pid, char *program,
> + const std::vector<char *> &argv);

The "full argv" would include the program name, but ARGV is no
longer including it, right? Or am I confused?

> +
> +/* Get the 'struct gdb_environ *' being used in the current
> + session. */
> +extern struct gdb_environ *get_environ (void);
> +
> #endif /* SERVER_H */
> diff --git a/gdb/gdbserver/spu-low.c b/gdb/gdbserver/spu-low.c
> index 117b871..49cfd65 100644
> --- a/gdb/gdbserver/spu-low.c
> +++ b/gdb/gdbserver/spu-low.c
> @@ -27,6 +27,7 @@
> #include <sys/syscall.h>
> #include "filestuff.h"
> #include "hostio.h"
> +#include "common-inferior.h"
>
> /* Some older glibc versions do not define this. */
> #ifndef __WNOTHREAD
> @@ -261,42 +262,41 @@ spu_proc_xfer_spu (const char *annex, unsigned char *readbuf,
> return ret;
> }
>
> +/* Callback to be used when calling fork_inferior, responsible for
> + actually initiating the tracing of the inferior. */
> +
> +static void
> +spu_ptrace_fun (void)
> +{
> + if (ptrace (PTRACE_TRACEME, 0, 0, 0) < 0)
> + trace_start_error_with_name ("ptrace");
> + if (setpgid (0, 0) < 0)
> + trace_start_error_with_name ("setpgid");
> +}
>
> /* Start an inferior process and returns its pid.
> - ALLARGS is a vector of program-name and args. */
> + PROGRAM_ARGV is a vector of program-name and args. */

Stale?

> static int
> -spu_create_inferior (char *program, char **allargs)
> +spu_create_inferior (char *program, const std::vector<char *> &program_argv)
> {
> int pid;
> ptid_t ptid;
> struct process_info *proc;
> + std::string program_args = stringify_argv (program_argv);
>
> - pid = fork ();
> - if (pid < 0)
> - perror_with_name ("fork");
> -
> - if (pid == 0)
> - {
> - close_most_fds ();
> - ptrace (PTRACE_TRACEME, 0, 0, 0);
> -
> - setpgid (0, 0);
> + pre_fork_inferior (program_argv);
>
> - execv (program, allargs);
> - if (errno == ENOENT)
> - execvp (program, allargs);
> + pid = fork_inferior (program,
> + program_args.c_str (),
> + environ_vector (get_environ ()), spu_ptrace_fun,
> + NULL, NULL, NULL, NULL);
>
> - fprintf (stderr, "Cannot exec %s: %s.\n", program,
> - strerror (errno));
> - fflush (stderr);
> - _exit (0177);
> - }
> + post_fork_inferior (pid, program, program_argv);
>
> - proc = add_process (pid, 0);
> + proc = find_process_pid (pid);
> + gdb_assert (proc != NULL);
> proc->tdesc = tdesc_spu;
>
> - ptid = ptid_build (pid, pid, 0);
> - add_thread (ptid, NULL);
> return pid;
> }
>
> diff --git a/gdb/gdbserver/target.c b/gdb/gdbserver/target.c
> index fda72e8..1e1d193 100644
> --- a/gdb/gdbserver/target.c
> +++ b/gdb/gdbserver/target.c
> @@ -387,3 +387,27 @@ default_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
> (*the_target->sw_breakpoint_from_kind) (0, &size);
> return size;
> }
> +
> +/* 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. */
> +}
> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
> index 3cc2bc4..e47ed78 100644
> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
> @@ -28,6 +28,7 @@
> #include "target/waitstatus.h"
> #include "mem-break.h"
> #include "btrace-common.h"
> +#include <vector>
>
> struct emit_ops;
> struct buffer;
> @@ -73,7 +74,8 @@ struct target_ops
> Returns the new PID on success, -1 on failure. Registers the new
> process with the process list. */
>
> - int (*create_inferior) (char *program, char **args);
> + int (*create_inferior) (char *program,
> + const std::vector<char *> &program_argv);
>
> /* Do additional setup after a new process is created, including
> exec-wrapper completion. */
> @@ -480,8 +482,8 @@ extern struct target_ops *the_target;
>
> void set_target_ops (struct target_ops *);
>
> -#define create_inferior(program, args) \
> - (*the_target->create_inferior) (program, args)
> +#define create_inferior(program, program_argv) \
> + (*the_target->create_inferior) (program, program_argv)
>
> #define target_post_create_inferior() \
> do \
> diff --git a/gdb/gdbserver/terminal.c b/gdb/gdbserver/terminal.c
> index fb06209..ee361d1 100644
> --- a/gdb/gdbserver/terminal.c
> +++ b/gdb/gdbserver/terminal.c
> @@ -18,6 +18,7 @@
>
> #include "server.h"
> #include "common-terminal.h"
> +#include "common-top.h"
>
> /* Placeholders needed by fork_inferior. For now, these functions are
> not needed nor useful to have on gdbserver. When/If we properly
> @@ -34,14 +35,14 @@ new_tty (void)
> /* See common/common-terminal.h. */
>
> void
> -new_tty_prefork (const char *ttyname)
> +tty_prefork_hook (void)
> {
> }
>
> /* See common/common-terminal.h. */
>
> void
> -new_tty_postfork (void)
> +tty_postfork_hook (void)
> {
> }
>
> @@ -67,3 +68,10 @@ get_inferior_io_terminal (void)
> {
> return NULL;
> }
> +
> +/* See common/common-top.h. */
> +
> +void
> +switch_ui_postfork (void)
> +{
> +}
> diff --git a/gdb/gdbserver/utils.c b/gdb/gdbserver/utils.c
> index 307d15a..70e8b18 100644
> --- a/gdb/gdbserver/utils.c
> +++ b/gdb/gdbserver/utils.c
> @@ -137,3 +137,39 @@ pfildes (gdb_fildes_t fd)
> return plongest (fd);
> #endif
> }
> +
> +/* See common/common-utils.h. */
> +
> +void
> +gdb_flush_out_err (void)
> +{
> + fflush (stdout);
> + fflush (stderr);
> +}
> +
> +/* See common/common-utils.h. */
> +
> +void
> +free_vector_argv (std::vector<char *> &v)
> +{
> + for (char *&i : v)

Iterating over pointers, no need for reference indirection,
copy is fine:

for (char *el: v)
xfree (el);

> + xfree (i);
> +
> + v.clear ();
> +}
> +
> +/* See common/common-utils.h. */
> +
> +std::string
> +stringify_argv (const std::vector<char *> &argv)
> +{
> + std::string ret;
> +
> + for (auto s : argv)
> + ret += s + std::string (" ");
> +
> + /* Erase the last whitespace. */
> + ret.erase (ret.end () - 1);

Are we always sure ARGV wasn't empty here?

> +
> + return ret;
> +}
> diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
> index b4ded31..172a2ec 100644
> --- a/gdb/gdbserver/utils.h
> +++ b/gdb/gdbserver/utils.h
> @@ -19,7 +19,18 @@
> #ifndef UTILS_H
> #define UTILS_H
>
> +#include <vector>
> +
> char *paddress (CORE_ADDR addr);
> char *pfildes (gdb_fildes_t fd);
>
> +/* Assumes that V is an argv for a program, and iterates through
> + freeing all the elements. */
> +extern void free_vector_argv (std::vector<char *> &v);
> +
> +/* Given a vector of arguments ARGV, return a string equivalent to
> + joining all the arguments (starting from ARGV + 1) with a
> + whitespace separating them. */

Stale?

> +extern std::string stringify_argv (const std::vector<char *> &argv);
> +
> #endif /* UTILS_H */
> diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c
> index d3ddbf5..b86ae2b 100644
> --- a/gdb/gdbserver/win32-low.c
> +++ b/gdb/gdbserver/win32-low.c
> @@ -608,13 +608,11 @@ create_process (const char *program, char *args,
> }
>
> /* Start a new process.
> - PROGRAM is a path to the program to execute.
> - ARGS is a standard NULL-terminated array of arguments,
> - to be passed to the inferior as ``argv''.
> + PROGRAM_ARGV is the vector containing the inferior's argv.

Stale? I also wonder about the variable renamings that were preserved
(throughout) in the patch. Maybe calling the vector of arguments
some other than "argv", like in the preexisting "args" would result in
clearer code for avoiding a confusion with "argv" generaly understood
as including the argv[0]==program name.

> Returns the new PID on success, -1 on failure. Registers the new
> process with the process list. */
> static int
> -win32_create_inferior (char *program, char **program_args)
> +win32_create_inferior (char *program, const std::vector<char *> &program_argv)
> {
> #ifndef USE_WIN32API
> char real_path[PATH_MAX];
> @@ -627,6 +625,8 @@ win32_create_inferior (char *program, char **program_args)
> int argc;
> PROCESS_INFORMATION pi;
> DWORD err;
> + std::string program_args = stringify_argv (program_argv);
> + char *args = (char *) program_args.c_str ();

Why do we need the cast?

>
> /* win32_wait needs to know we're not attaching. */
> attaching = 0;
> @@ -652,18 +652,6 @@ win32_create_inferior (char *program, char **program_args)
> program = real_path;
> #endif
>
> - argslen = 1;
> - for (argc = 1; program_args[argc]; argc++)
> - argslen += strlen (program_args[argc]) + 1;
> - args = (char *) alloca (argslen);
> - args[0] = '\0';
> - for (argc = 1; program_args[argc]; argc++)
> - {
> - /* FIXME: Can we do better about quoting? How does Cygwin
> - handle this? */
> - strcat (args, " ");
> - strcat (args, program_args[argc]);
> - }
> OUTMSG2 (("Command line is \"%s\"\n", args));
>
> #ifdef CREATE_NEW_PROCESS_GROUP
> diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
> index 7efb3c1..21445f5 100644
> --- a/gdb/gnu-nat.c
> +++ b/gdb/gnu-nat.c
> @@ -2136,6 +2136,7 @@ gnu_create_inferior (struct target_ops *ops,
> {
> struct inf *inf = cur_inf ();
> int pid;
> + ptid_t ptid;
>
> inf_debug (inf, "creating inferior");
>
> @@ -2161,7 +2162,10 @@ gnu_create_inferior (struct target_ops *ops,
> thread_change_ptid (inferior_ptid,
> ptid_build (inf->pid, inf_pick_first_thread (), 0));
>
> - startup_inferior (START_INFERIOR_TRAPS_EXPECTED);
> + startup_inferior (START_INFERIOR_TRAPS_EXPECTED, NULL, NULL);
> + /* Mark all threads non-executing. */
> + set_executing (ptid, 0);
> +
> inf->pending_execs = 0;
> /* Get rid of the old shell threads. */
> prune_threads ();
> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> index 21578742..e3ddaa8 100644
> --- a/gdb/inf-ptrace.c
> +++ b/gdb/inf-ptrace.c
> @@ -94,7 +94,7 @@ inf_ptrace_create_inferior (struct target_ops *ops,
> int from_tty)
> {
> int pid;
> -
> + ptid_t ptid;
> /* Do not change either targets above or the same target if already present.
> The reason is the target stack is shared across multiple inferiors. */
> int ops_already_pushed = target_is_pushed (ops);
> @@ -112,7 +112,10 @@ inf_ptrace_create_inferior (struct target_ops *ops,
>
> discard_cleanups (back_to);
>
> - startup_inferior (START_INFERIOR_TRAPS_EXPECTED);
> + ptid = startup_inferior (START_INFERIOR_TRAPS_EXPECTED, NULL, NULL);
> +
> + /* Mark all threads non-executing. */
> + set_executing (ptid, 0);

Since all callers on the gdb side need to do this, wouldn't it
be better to add a gdb_startup_inferior function that calls
the common startup_inferior and calls set_executing ?
(or call the common one something else)

Thanks,
Pedro Alves