Re: [RFAv3 4/6] Implement | (pipe) command.

Pedro Alves
2019-05-27T17:48:44Z

On 5/4/19 5:17 PM, Philippe Waroquiers wrote:
> The pipe command allows to run a GDB command, and pipe its output
> to a shell command:
> (gdb) help pipe
> Send the output of a gdb command to a shell command.
> Usage: pipe [COMMAND] | SHELL_COMMAND
> Usage: | [COMMAND] | SHELL_COMMAND
> Usage: pipe -dX COMMAND X SHELL_COMMAND
> Usage: | -dX COMMAND X SHELL_COMMAND
> Executes COMMAND and sends its output to SHELL_COMMAND.
> If COMMAND contains a | character, the option -dX indicates
> to use the character X to separate COMMAND from SHELL_COMMAND.
> With no COMMAND, repeat the last executed command
> and send its output to SHELL_COMMAND.
> (gdb)

This help output is stale now. The current output is:

(gdb) help pipe
Send the output of a gdb command to a shell command.
Usage: | [COMMAND] | SHELL_COMMAND
Usage: | -d SEP COMMAND SEP SHELL_COMMAND
Usage: pipe [COMMAND] | SHELL_COMMAND
Usage: pipe -d SEP COMMAND SEP SHELL_COMMAND
Executes COMMAND and sends its output to SHELL_COMMAND.
If COMMAND contains a | character, the option -d SEP indicates
to use the string SEP to separate COMMAND from SHELL_COMMAND.
With no COMMAND, repeat the last executed command
and send its output to SHELL_COMMAND.
(gdb)

I think the "If COMMAND contains a | character," could be
improved with a bit of copy/editing. Something like this:

(gdb) help pipe
Send the output of a gdb command to a shell command.
Usage: | [COMMAND] | SHELL_COMMAND
Usage: | -d SEP COMMAND SEP SHELL_COMMAND
Usage: pipe [COMMAND] | SHELL_COMMAND
Usage: pipe -d SEP COMMAND SEP SHELL_COMMAND

Executes COMMAND and sends its output to SHELL_COMMAND.

The -d option indicates to use the string SEP to separate COMMAND
from SHELL_COMMAND, in alternative to |. This is useful in
case COMMAND contains a | character.

With no COMMAND, repeat the last executed command
and send its output to SHELL_COMMAND.

(Or see the suggestion in my reply to the manual patch).

Also, is there a reason you picked "-d" for the option
letter? Maybe you were thinking of "delimiter"? In
that case, maybe consider describing it with
"-d DEL" or "-d DELIM" instead of "-d SEP", but better
mnemonics. Just a suggestion.

> For example:
> (gdb) pipe print some_data_structure | grep -B3 -A3 something
>
> The pipe character is defined as an alias for pipe command, so that
> the above can be typed as:
> (gdb) | print some_data_structure | grep -B3 -A3 something
>
> If no GDB COMMAND is given, then the previous command is relaunched,
> and its output is sent to the given SHELL_COMMAND.
>
> This also defines convenience vars $_shell_exitcode and $_shell_exitsignal
> to record the exit code and exit signal of the last shell command
> launched by GDB e.g. by "shell", "pipe", ...
>
> gdb/ChangeLog
> 2019-05-04 Philippe Waroquiers <philippe.waroquiers@skynet.be>
>
> * cli/cli-cmds.c (pipe_command): New function.
> (_initialize_cli_cmds): Call add_com for pipe_command.
> Define | as an alias for pipe.
> (exit_status_set_internal_vars): New function.
> (shell_escape): Call exit_status_set_internal_vars.
> cli/cli-decode.c (find_command_name_length): Recognize | as
> a single character command.
> ---
> gdb/cli/cli-cmds.c | 105 +++++++++++++++++++++++++++++++++++++++++++
> gdb/cli/cli-decode.c | 4 +-
> 2 files changed, 107 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index 5f3b973f06..55fb5a9a7f 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -24,6 +24,7 @@
> #include "completer.h"
> #include "target.h" /* For baud_rate, remote_debug and remote_timeout. */
> #include "common/gdb_wait.h" /* For shell escape implementation. */
> +#include "gdbcmd.h"
> #include "gdb_regex.h" /* Used by apropos_command. */
> #include "gdb_vfork.h"
> #include "linespec.h"
> @@ -41,6 +42,7 @@
> #include "block.h"
>
> #include "ui-out.h"
> +#include "interps.h"
>
> #include "top.h"
> #include "cli/cli-decode.h"
> @@ -695,6 +697,25 @@ echo_command (const char *text, int from_tty)
> gdb_flush (gdb_stdout);
> }
>
> +/* Sets the last launched shell command convenience variables based on
> + EXIT_STATUS. */
> +
> +static void
> +exit_status_set_internal_vars (int exit_status)
> +{
> + struct internalvar *var_code = lookup_internalvar ("_shell_exitcode");
> + static internalvar *var_signal = lookup_internalvar ("_shell_exitsignal");
^^^^^^

Did you really mean static ? At least, did you mean it just for one of them?

> +
> + clear_internalvar (var_code);
> + clear_internalvar (var_signal);
> + if (WIFEXITED (exit_status))
> + set_internalvar_integer (var_code, WEXITSTATUS (exit_status));
> + else if (WIFSIGNALED (exit_status))
> + set_internalvar_integer (var_signal, WTERMSIG (exit_status));
> + else
> + warning (_("unexpected shell command exit_status %d\n"), exit_status);

That "exit_status" in the warning is leaking out the internal variable
name in a user-facing message.

> +}
> +
> static void
> shell_escape (const char *arg, int from_tty)
> {
> @@ -716,6 +737,7 @@ shell_escape (const char *arg, int from_tty)
> /* Make sure to return to the directory GDB thinks it is, in case
> the shell command we just ran changed it. */
> chdir (current_directory);
> + exit_status_set_internal_vars (rc);
> #endif
> #else /* Can fork. */
> int status, pid;
> @@ -743,6 +765,7 @@ shell_escape (const char *arg, int from_tty)
> waitpid (pid, &status, 0);
> else
> error (_("Fork failed"));
> + exit_status_set_internal_vars (status);
> #endif /* Can fork. */
> }
>
> @@ -854,6 +877,75 @@ edit_command (const char *arg, int from_tty)
> xfree (p);
> }
>
> +/* Implementation of the "pipe" command. */
> +
> +static void
> +pipe_command (const char *arg, int from_tty)
> +{
> + const char *command = arg;
> + const char *shell_command = arg;
> + std::string separator ("|");
> +
> + if (arg == NULL)
> + error (_("Missing COMMAND"));
> +
> + shell_command = skip_spaces (shell_command);

This isn't necessary.

> +
> + if (*shell_command == '-' && *(shell_command + 1) == 'd')
> + {
> + shell_command += 2; /* Skip '-d'. */

Use check_for_argument.

> + separator = extract_arg (&shell_command);
> + if (separator.empty ())
> + error (_("Missing separator SEP after -d"));
> + command = shell_command;
> + }
> +
> + shell_command = strstr (shell_command, separator.c_str ());
> +
> + if (shell_command == nullptr)

Let's be consistent with NULL vs nullptr in new code.

> + error (_("Missing separator before SHELL_COMMAND"));

BTW, I suspect the section above would look clearer if it
referred to arg instead of shell_command, and declared
command/shell_command where they're found. Like:

if (check_for_argument (arg, "-d", 2))
{
separator = extract_arg (&arg);
if (separator.empty ())
error (_("Missing separator SEP after -d"));
}

arg = skip_spaces (arg);

const char *command = arg;

const char *shell_command = strstr (arg, separator.c_str ());

if (shell_command == nullptr)
error (_("Missing separator before SHELL_COMMAND"));

> +
> + command = skip_spaces (command);
> + std::string gdb_cmd (command, shell_command - command);
> +
> + if (gdb_cmd.empty ())
> + {
> + repeat_previous ();
> + gdb_cmd = skip_spaces (get_saved_command_line ());
> + if (gdb_cmd.empty ())
> + error (_("No previous command to relaunch"));
> + }
> +
> + shell_command += separator.length (); /* Skip the separator. */
> + shell_command = skip_spaces (shell_command);
> + if (*shell_command == '\0')
> + error (_("Missing SHELL_COMMAND"));
> +
> + FILE *to_shell_command = popen (shell_command, "w");
> +
> + if (to_shell_command == nullptr)
> + error (_("Error launching \"%s\""), shell_command);
> +
> + try
> + {
> + stdio_file pipe_file (to_shell_command);

stdio_file's destructor calls fclose unless you tell it otherwise:

/* Create a ui_file from a previously opened FILE. CLOSE_P
indicates whether the underlying file should be closed when the
stdio_file is destroyed. */
explicit stdio_file (FILE *file, bool close_p = false);

> +
> + execute_command_to_ui_file (&pipe_file, gdb_cmd.c_str (), from_tty);

So here this is calling fclose before leaving the scope. But popen
FILE's should be closed with pclose, only. I don't know why it isn't
causing problems, may be the glibc's fclose does nothing in this case.
We shouldn't rely on that.

> + }
> + catch (const gdb_exception_error &ex)

This should catch all kinds of exceptions, not just errors. E.g.,
Ctrl-C results in gdb_exception_quit, which this doesn't catch.

Write, literally:

catch (...)

to be clear that we want to catch everything.

> + {
> + pclose (to_shell_command);
> + throw;
> + }
> +
> + int exit_status = pclose (to_shell_command);
> +
> + if (exit_status < 0)
> + error (_("shell command \"%s\" errno %s"), shell_command,
> + safe_strerror (errno));

Spurious double space in the error message. Note you say "errno",
but this isn't printing the errno (the error number). I'd
suggest printing in perror_with_name style:

error (_("shell command \"%s\" failed: %s"), shell_command,
safe_strerror (errno));

Thanks,
Pedro Alves