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

Message ID 20190504161753.15530-5-philippe.waroquiers@skynet.be
State New
Headers show
Series
  • Implement | (pipe) command.
Related show

Commit Message

Philippe Waroquiers May 4, 2019, 4:17 p.m.
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)

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(-)

-- 
2.20.1

Comments

Pedro Alves May 27, 2019, 5:48 p.m. | #1
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
Pedro Alves May 27, 2019, 5:55 p.m. | #2
On 5/27/19 6:48 PM, Pedro Alves wrote:
> 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.

> 


Ahahaha.  The argument defaults to false, not true.  Silly me.

Thanks,
Pedro Alves

Patch

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");
+
+  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);
+}
+
 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);
+
+  if (*shell_command == '-' && *(shell_command + 1) == 'd')
+    {
+      shell_command += 2; /* Skip '-d'.  */
+      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)
+    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);
+
+      execute_command_to_ui_file (&pipe_file, gdb_cmd.c_str (), from_tty);
+    }
+  catch (const gdb_exception_error &ex)
+    {
+      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));
+  exit_status_set_internal_vars (exit_status);
+}
+
 static void
 list_command (const char *arg, int from_tty)
 {
@@ -1845,6 +1937,19 @@  Uses EDITOR environment variable contents as editor (or ex as default)."));
 
   c->completer = location_completer;
 
+  c = add_com ("pipe", class_support, pipe_command, _("\
+Send the output of a gdb command to a shell command.\n\
+Usage: | [COMMAND] | SHELL_COMMAND\n\
+Usage: | -d SEP COMMAND SEP SHELL_COMMAND\n\
+Usage: pipe [COMMAND] | SHELL_COMMAND\n\
+Usage: pipe -d SEP COMMAND SEP SHELL_COMMAND\n\
+Executes COMMAND and sends its output to SHELL_COMMAND.\n\
+If COMMAND contains a | character, the option -d SEP indicates\n\
+to use the string SEP to separate COMMAND from SHELL_COMMAND.\n\
+With no COMMAND, repeat the last executed command\n\
+and send its output to SHELL_COMMAND."));
+  add_com_alias ("|", "pipe", class_support, 0);
+
   add_com ("list", class_files, list_command, _("\
 List specified function or line.\n\
 With no argument, lists ten more lines after or around previous listing.\n\
diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 50430953c7..46051090cd 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -1311,9 +1311,9 @@  find_command_name_length (const char *text)
      Note that this is larger than the character set allowed when
      creating user-defined commands.  */
 
-  /* Recognize '!' as a single character command so that, e.g., "!ls"
+  /* Recognize the single character commands so that, e.g., "!ls"
      works as expected.  */
-  if (*p == '!')
+  if (*p == '!' || *p == '|')
     return 1;
 
   while (isalnum (*p) || *p == '-' || *p == '_'