[RFC] GDB: inferior standard I/O redirection
Commit Message
While reviewing the proposed "GDBServer stderr redirection" patches,
Pedro Alves pointed out that a better approach would be to actually
implement "set inferior-std{in,out,err}" commands to complement "set
inferior-tty". This would need to be done at both the GDB level and
also at the server level.
This is a functional RFC for GDB only. Being an RFC it lacks
documentation, NEWS file mentions, ChangeLog and (DejaGnu) tests.
Since this a feature to be used in Avocado[1], there's a simple
(Avocado) test at:
https://github.com/clebergnu/avocado/blob/gdb-inferior-io-test/examples/tests/gdb-inferior-io.py
I will surely appreciate any comments to help me shape a v1 of this
feature.
[1] - http://avocado-framework.github.io
---
gdb/fork-child.c | 54 +++++++++++++++++++++
gdb/infcmd.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
gdb/inferior.c | 3 ++
gdb/inferior.h | 11 +++++
4 files changed, 208 insertions(+)
Comments
Hi Cleber,
Sorry for the delay, and thanks for doing this.
On 05/06/2015 08:51 PM, Cleber Rosa wrote:
> While reviewing the proposed "GDBServer stderr redirection" patches,
> Pedro Alves pointed out that a better approach would be to actually
> implement "set inferior-std{in,out,err}" commands to complement "set
> inferior-tty". This would need to be done at both the GDB level and
> also at the server level.
(I'm fine with redirecting gdbserver's output, provided it's
done like this patch is doing, with dup/dup2.)
In general this is looking good. A few comments inline below.
>
> This is a functional RFC for GDB only. Being an RFC it lacks
> documentation, NEWS file mentions, ChangeLog and (DejaGnu) tests.
>
> Since this a feature to be used in Avocado[1], there's a simple
> (Avocado) test at:
>
> https://github.com/clebergnu/avocado/blob/gdb-inferior-io-test/examples/tests/gdb-inferior-io.py
>
> I will surely appreciate any comments to help me shape a v1 of this
> feature.
>
> [1] - http://avocado-framework.github.io
>
>
> +/* Helper function for that attempts to open a file and if successfull
> + redirects another file descriptor to the one just opened. */
s/for that/that. "successful".
> +
> +static int
> +set_std_io_helper (const char *file_name, int std_io_fd, int flags, mode_t mode)
> +{
> + int fd;
> +
> + if (!file_name)
> + return 0;
if (file_name == NULL)
> +
> + fd = open (file_name, flags, mode);
> + if (fd > 0)
> + dup2 (fd, std_io_fd);
Should handle dup2 errors too.
Also, don't we end up with 'fd' open/leaked in the inferior process
on success? That is, shouldn't we close fd after the dup2 ?
> + return fd;
> +}
> +
> +/* Performs the redirection of stdin, stdout and stderr, if requested.
> + There's a single error indication if any of these redirection
> + fail, which seem to be a good enough granularity. */
> +
> +static int
> +set_std_io (void)
> +{
> + int fd;
> + int result = 0;
> +
> + fd = set_std_io_helper (get_inferior_io_stdin(), 0,
> + O_RDONLY, S_IRUSR | S_IWUSR);
Missing space before parens.
> + if (fd < 0)
> + result = 1;
> +
> + fd = set_std_io_helper (get_inferior_io_stdout(), 1,
> + O_CREAT | O_WRONLY | O_TRUNC, S_IRUSR | S_IWUSR);
> + if (fd < 0)
> + result = 1;
> +
> + fd = set_std_io_helper (get_inferior_io_stderr(), 2,
> + O_CREAT | O_WRONLY | O_TRUNC, S_IRUSR | S_IWUSR);
Likewise these two.
> + if (fd < 0)
> + result = 1;
> +
> + return result;
> +}
> +
> /* 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
> @@ -358,6 +403,15 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
> path to find $SHELL. Rich Pixley says so, and I agree. */
> environ = env;
>
> + /* Set the inferior process standard I/O (input, output, error)
> + redirection if any was set with the stdin/stdout/stderr
> + commands. */
> + if (set_std_io ())
> + fprintf_unfiltered (gdb_stderr,
> + "Could not set one of stdin, stdout or stderr for "
> + "%s\n", exec_file);
> +
I think you should gdb_flush (gdb_stdout/gdb_err) before dup2'ing.
On failure to redirect, shouldn't this call _exit instead of proceeding
to exec anyway?
Also, if stderr manages to be redirected, but stdout not, then
that error ends up in the file. Same if redirection is successful
but then exec fails. Maybe we claim that that's what's intended.
> +
> if (exec_fun != NULL)
> (*exec_fun) (argv[0], argv, env);
> else
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 7e2484b..a1a9a7b 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -118,6 +118,11 @@ static char *inferior_args_scratch;
>
> static char *inferior_io_terminal_scratch;
>
> +/* Scratch are where 'set inferior-{stdin,stdout,stderr}' will store
> + user provided value. */
"Scratch are where" doesn't parse.
> +
> +static char *inferior_io_std_scratch[3] = { NULL, NULL, NULL };
> +
> /* Pid of our debugged inferior, or 0 if no inferior now.
> Since various parts of infrun.c test this to see whether there is a program
> being debugged it should be nonzero (currently 3 is used) for remote
> @@ -185,6 +190,111 @@ show_inferior_tty_command (struct ui_file *file, int from_tty,
> "is \"%s\".\n"), inferior_io_terminal);
> }
>
> +void
> +set_inferior_io_stdin (const char *file_name)
> +{
> + xfree (current_inferior ()->standard_io[0]);
> + current_inferior ()->standard_io[0] = file_name ? xstrdup (file_name) : 0;
Write NULL instead of 0. Also, no implicit boolean coertion. So write
file_name != NULL.
current_inferior ()->standard_io[0]
= file_name != NULL ? xstrdup (file_name) : NULL;
Repeat for the other files. Maybe some of this duplication could be
eliminated with helpers like:
static void
set_inferior_io_stdfd (const char *file_name, int stdfd)
{
gdb_assert (0 <= std_file && std_file <= 2);
xfree (current_inferior ()->standard_io[stdfd]);
current_inferior ()->standard_io[stdfd]
= file_name != NULL ? xstrdup (file_name) : NULL;
}
void
set_inferior_io_stdin (const char *file_name)
{
set_inferior_io_stdfd (file_name, 0);
}
void
set_inferior_io_stdout (const char *file_name)
{
set_inferior_io_stdfd (file_name, 1);
}
etc.
Thanks,
Pedro Alves
@@ -108,6 +108,51 @@ escape_bang_in_quoted_argument (const char *shell_file)
return 0;
}
+/* Helper function for that attempts to open a file and if successfull
+ redirects another file descriptor to the one just opened. */
+
+static int
+set_std_io_helper (const char *file_name, int std_io_fd, int flags, mode_t mode)
+{
+ int fd;
+
+ if (!file_name)
+ return 0;
+
+ fd = open (file_name, flags, mode);
+ if (fd > 0)
+ dup2 (fd, std_io_fd);
+ return fd;
+}
+
+/* Performs the redirection of stdin, stdout and stderr, if requested.
+ There's a single error indication if any of these redirection
+ fail, which seem to be a good enough granularity. */
+
+static int
+set_std_io (void)
+{
+ int fd;
+ int result = 0;
+
+ fd = set_std_io_helper (get_inferior_io_stdin(), 0,
+ O_RDONLY, S_IRUSR | S_IWUSR);
+ if (fd < 0)
+ result = 1;
+
+ fd = set_std_io_helper (get_inferior_io_stdout(), 1,
+ O_CREAT | O_WRONLY | O_TRUNC, S_IRUSR | S_IWUSR);
+ if (fd < 0)
+ result = 1;
+
+ fd = set_std_io_helper (get_inferior_io_stderr(), 2,
+ O_CREAT | O_WRONLY | O_TRUNC, S_IRUSR | S_IWUSR);
+ if (fd < 0)
+ result = 1;
+
+ return result;
+}
+
/* 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
@@ -358,6 +403,15 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env,
path to find $SHELL. Rich Pixley says so, and I agree. */
environ = env;
+ /* Set the inferior process standard I/O (input, output, error)
+ redirection if any was set with the stdin/stdout/stderr
+ commands. */
+ if (set_std_io ())
+ fprintf_unfiltered (gdb_stderr,
+ "Could not set one of stdin, stdout or stderr for "
+ "%s\n", exec_file);
+
+
if (exec_fun != NULL)
(*exec_fun) (argv[0], argv, env);
else
@@ -118,6 +118,11 @@ static char *inferior_args_scratch;
static char *inferior_io_terminal_scratch;
+/* Scratch are where 'set inferior-{stdin,stdout,stderr}' will store
+ user provided value. */
+
+static char *inferior_io_std_scratch[3] = { NULL, NULL, NULL };
+
/* Pid of our debugged inferior, or 0 if no inferior now.
Since various parts of infrun.c test this to see whether there is a program
being debugged it should be nonzero (currently 3 is used) for remote
@@ -185,6 +190,111 @@ show_inferior_tty_command (struct ui_file *file, int from_tty,
"is \"%s\".\n"), inferior_io_terminal);
}
+void
+set_inferior_io_stdin (const char *file_name)
+{
+ xfree (current_inferior ()->standard_io[0]);
+ current_inferior ()->standard_io[0] = file_name ? xstrdup (file_name) : 0;
+}
+
+const char *
+get_inferior_io_stdin (void)
+{
+ return current_inferior ()->standard_io[0];
+}
+
+static void
+set_inferior_stdin_command (char *args, int from_tty,
+ struct cmd_list_element *c)
+{
+ set_inferior_io_stdin (inferior_io_std_scratch[0]);
+}
+
+static void
+show_inferior_stdin_command (struct ui_file *file, int from_tty,
+ struct cmd_list_element *c, const char *value)
+{
+ /* Note that we ignore the passed-in value in favor of computing it
+ directly. */
+ const char *inferior_io_stdin = get_inferior_io_stdin ();
+
+ if (inferior_io_stdin == NULL)
+ inferior_io_stdin = "";
+ fprintf_filtered (gdb_stdout,
+ _("Standard input for future runs of program being "
+ "debugged is \"%s\".\n"), inferior_io_stdin);
+}
+
+void
+set_inferior_io_stdout (const char *file_name)
+{
+ xfree (current_inferior ()->standard_io[1]);
+ current_inferior ()->standard_io[1] = file_name ? xstrdup (file_name) : 0;
+}
+
+const char *
+get_inferior_io_stdout (void)
+{
+ return current_inferior ()->standard_io[1];
+}
+
+static void
+set_inferior_stdout_command (char *args, int from_tty,
+ struct cmd_list_element *c)
+{
+ set_inferior_io_stdout (inferior_io_std_scratch[1]);
+}
+
+static void
+show_inferior_stdout_command (struct ui_file *file, int from_tty,
+ struct cmd_list_element *c, const char *value)
+{
+ /* Note that we ignore the passed-in value in favor of computing it
+ directly. */
+ const char *inferior_io_stdout = get_inferior_io_stdout ();
+
+ if (inferior_io_stdout == NULL)
+ inferior_io_stdout = "";
+ fprintf_filtered (gdb_stdout,
+ _("Standard output for future runs of program being "
+ "debugged is \"%s\".\n"), inferior_io_stdout);
+}
+
+void
+set_inferior_io_stderr (const char *file_name)
+{
+ xfree (current_inferior ()->standard_io[2]);
+ current_inferior ()->standard_io[2] = file_name ? xstrdup (file_name) : 0;
+}
+
+const char *
+get_inferior_io_stderr (void)
+{
+ return current_inferior ()->standard_io[2];
+}
+
+static void
+set_inferior_stderr_command (char *args, int from_tty,
+ struct cmd_list_element *c)
+{
+ set_inferior_io_stderr (inferior_io_std_scratch[2]);
+}
+
+static void
+show_inferior_stderr_command (struct ui_file *file, int from_tty,
+ struct cmd_list_element *c, const char *value)
+{
+ /* Note that we ignore the passed-in value in favor of computing it
+ directly. */
+ const char *inferior_io_stderr = get_inferior_io_stderr ();
+
+ if (inferior_io_stderr == NULL)
+ inferior_io_stderr = "";
+ fprintf_filtered (gdb_stderr,
+ _("Standard error for future runs of program being "
+ "debugged is \"%s\".\n"), inferior_io_stderr);
+}
+
char *
get_inferior_args (void)
{
@@ -2986,6 +3096,36 @@ Usage: set inferior-tty /dev/pts/1"),
&setlist, &showlist);
add_com_alias ("tty", "set inferior-tty", class_alias, 0);
+ add_setshow_filename_cmd ("inferior-stdin", class_run,
+ &inferior_io_std_scratch[0], _("\
+Set standard input for future runs of program being debugged."), _("\
+Show standard input for future runs of program being debugged."), _("\
+Usage: set inferior-input /tmp/redirected.stdin"),
+ set_inferior_stdin_command,
+ show_inferior_stdin_command,
+ &setlist, &showlist);
+ add_com_alias ("stdin", "set inferior-stdin", class_alias, 0);
+
+ add_setshow_filename_cmd ("inferior-stdout", class_run,
+ &inferior_io_std_scratch[1], _("\
+Set standard output for future runs of program being debugged."), _("\
+Show standard output for future runs of program being debugged."), _("\
+Usage: set inferior-stdout /tmp/redirected.stdout"),
+ set_inferior_stdout_command,
+ show_inferior_stdout_command,
+ &setlist, &showlist);
+ add_com_alias ("stdout", "set inferior-stdout", class_alias, 0);
+
+ add_setshow_filename_cmd ("inferior-stderr", class_run,
+ &inferior_io_std_scratch[2], _("\
+Set standard error for future runs of program being debugged."), _("\
+Show standard error for future runs of program being debugged."), _("\
+Usage: set inferior-stderr /tmp/redirected.stderr"),
+ set_inferior_stderr_command,
+ show_inferior_stderr_command,
+ &setlist, &showlist);
+ add_com_alias ("stderr", "set inferior-stderr", class_alias, 0);
+
cmd_name = "args";
add_setshow_string_noescape_cmd (cmd_name, class_run,
&inferior_args_scratch, _("\
@@ -99,6 +99,9 @@ free_inferior (struct inferior *inf)
inferior_free_data (inf);
xfree (inf->args);
xfree (inf->terminal);
+ xfree (inf->standard_io[0]);
+ xfree (inf->standard_io[1]);
+ xfree (inf->standard_io[2]);
free_environ (inf->environment);
target_desc_info_free (inf->tdesc_info);
xfree (inf->priv);
@@ -77,6 +77,14 @@ extern void clear_sigint_trap (void);
extern void set_inferior_io_terminal (const char *terminal_name);
extern const char *get_inferior_io_terminal (void);
+/* Set/get file name for standard input/output/error. */
+extern void set_inferior_io_stdin (const char *file_name);
+extern const char *get_inferior_io_stdin (void);
+extern void set_inferior_io_stdout (const char *file_name);
+extern const char *get_inferior_io_stdout (void);
+extern void set_inferior_io_stderr (const char *file_name);
+extern const char *get_inferior_io_stderr (void);
+
/* Collected pid, tid, etc. of the debugged inferior. When there's
no inferior, ptid_get_pid (inferior_ptid) will be 0. */
@@ -345,6 +353,9 @@ struct inferior
/* The name of terminal device to use for I/O. */
char *terminal;
+ /* The names of files to use as standard input/output/error */
+ char *standard_io[3];
+
/* Environment to use for running inferior,
in format described in environ.h. */
struct gdb_environ *environment;