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

[Pedro Alves]

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