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

Message ID 20190426201108.7489-5-philippe.waroquiers@skynet.be
State New, archived
Headers

Commit Message

Philippe Waroquiers April 26, 2019, 8:11 p.m. UTC
  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.

gdb/ChangeLog
2019-04-26  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.
	cli/cli-decode.c (find_command_name_length): Recognize | as
	a single character command.
---
 gdb/cli/cli-cmds.c   | 110 +++++++++++++++++++++++++++++++++++++++++++
 gdb/cli/cli-decode.c |   4 +-
 2 files changed, 112 insertions(+), 2 deletions(-)
  

Comments

Pedro Alves May 3, 2019, 6:59 p.m. UTC | #1
On 4/26/19 9:11 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)

I think that making "-dX" a single character is a mistake.  I'd rather
make that "-d STRING", so you can use use a string that is much less
likely to conflict.  Like bash herestrings.  So a user would be able
to do:

pipe -d XXXYYYXXX $command | $shell_command

I also think "-dX" without a space in between is very non-gdb-ish.
In gdb, you kind of either have "-OPT X", or "/OX", which is kind of
double "--" vs "-" with getopt, except with different leading
tokens.

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

I get that "it makes sense", but do you see yourself using the "|" command
in preference to "pipe"?  "pipe" can be typed as "pi", which is two
key strokes as well.  Kind of wondering whether the character could be
saved for something else in the future.  I don't have a use in mind,
just thinking out loud.

> If no GDB COMMAND is given, then the previous command is relaunched,
> and its output is sent to the given 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++; /* skip the separator.  */

Uppercase "Skip".

> +  shell_command = skip_spaces (shell_command);
> +  if (*shell_command == 0)

  if (*shell_command == '\0')


> +    {
> +      if (separator == '|')

I don't think this check is 100% right, considering that you could do:

(gdb) pipe -d| SHELL_COMMAND

I.e., what you want to check is whether a separate was explicitly specified.

> +	error (_("Missing SHELL_COMMAND in \"| [COMMAND] | SHELL_COMMAND\""));
> +      else
> +	error (_("Missing SHELL_COMMAND in "
> +		 "\"| -d%c COMMAND %c SHELL_COMMAND\""),
> +	       separator, separator);

Or instead of making this dynamic, just print something like:

	error (_("Missing SHELL_COMMAND.\n"
                 "Usage: \"| [-c SEP] [COMMAND] | SHELL_COMMAND\""));


> +    }
> +
> +  FILE *to_shell_command = popen (shell_command, "w");
> +
> +  if (to_shell_command == NULL)
> +    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 shell_command_status = pclose (to_shell_command);
> +
> +  if (shell_command_status < 0)
> +    error (_("shell command  \"%s\" errno %s"), shell_command,
> +           safe_strerror (errno));
> +  if (shell_command_status > 0)
> +    {
> +      if (WIFEXITED (shell_command_status))
> +	warning (_("shell command \"%s\" exit status %d"), shell_command,
> +		 WEXITSTATUS (shell_command_status));
> +      else if (WIFSIGNALED (shell_command_status))
> +	warning (_("shell command \"%s\" exit with signal %d"), shell_command,
> +		 WTERMSIG (shell_command_status));
> +      else
> +	warning (_("shell command \"%s\" status %d"), shell_command,
> +		 shell_command_status);
> +    }

Is there a reason this isn't using the pex routines?

Thanks,
Pedro Alves
  
Philippe Waroquiers May 4, 2019, 6:24 a.m. UTC | #2
On Fri, 2019-05-03 at 19:59 +0100, Pedro Alves wrote:
> On 4/26/19 9:11 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)
> 
> I think that making "-dX" a single character is a mistake.  I'd rather
> make that "-d STRING", so you can use use a string that is much less
> likely to conflict.  Like bash herestrings.  So a user would be able
> to do:
> 
> pipe -d XXXYYYXXX $command | $shell_command
> 
> I also think "-dX" without a space in between is very non-gdb-ish.
> In gdb, you kind of either have "-OPT X", or "/OX", which is kind of
> double "--" vs "-" with getopt, except with different leading
> tokens.

Ok, will change to -d STRING.


> 
> > 
> > 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
> > 
> 
> I get that "it makes sense", but do you see yourself using the "|" command
> in preference to "pipe"?  "pipe" can be typed as "pi", which is two
> key strokes as well.  Kind of wondering whether the character could be
> saved for something else in the future.  I don't have a use in mind,
> just thinking out loud.
Yes, I always use "|" (I even just checked the test to be sure that I
also tested "pipe" and not only "|").
pip must be used, as pi launches an interactive python.

Also, "|" allows to do:
(gdb) some_command
...
(gdb) ||grep something_in_some_command_output
(gdb) ||tee save_some_command_output

which is better than
(gdb) pip|grep something_in_some_command_output

> 
> > If no GDB COMMAND is given, then the previous command is relaunched,
> > and its output is sent to the given 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++; /* skip the separator.  */
> 
> Uppercase "Skip".
> 
> > +  shell_command = skip_spaces (shell_command);
> > +  if (*shell_command == 0)
> 
>   if (*shell_command == '\0')
> 
> 
> > +    {
> > +      if (separator == '|')
> 
> I don't think this check is 100% right, considering that you could do:
That part of the code will change heavily when the separator becomes a string.
> 
> (gdb) pipe -d| SHELL_COMMAND
> 
> I.e., what you want to check is whether a separate was explicitly specified.
> 
> > +	error (_("Missing SHELL_COMMAND in \"| [COMMAND] | SHELL_COMMAND\""));
> > +      else
> > +	error (_("Missing SHELL_COMMAND in "
> > +		 "\"| -d%c COMMAND %c SHELL_COMMAND\""),
> > +	       separator, separator);
> 
> Or instead of making this dynamic, just print something like:
> 
> 	error (_("Missing SHELL_COMMAND.\n"
>                  "Usage: \"| [-c SEP] [COMMAND] | SHELL_COMMAND\""));

> 
> 
> > +    }
> > +
> > +  FILE *to_shell_command = popen (shell_command, "w");
> > +
> > +  if (to_shell_command == NULL)
> > +    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 shell_command_status = pclose (to_shell_command);
> > +
> > +  if (shell_command_status < 0)
> > +    error (_("shell command  \"%s\" errno %s"), shell_command,
> > +           safe_strerror (errno));
> > +  if (shell_command_status > 0)
> > +    {
> > +      if (WIFEXITED (shell_command_status))
> > +	warning (_("shell command \"%s\" exit status %d"), shell_command,
> > +		 WEXITSTATUS (shell_command_status));
> > +      else if (WIFSIGNALED (shell_command_status))
> > +	warning (_("shell command \"%s\" exit with signal %d"), shell_command,
> > +		 WTERMSIG (shell_command_status));
> > +      else
> > +	warning (_("shell command \"%s\" status %d"), shell_command,
> > +		 shell_command_status);
> > +    }
> 
> Is there a reason this isn't using the pex routines?
pex routines do not help to handle the exit status (and are
also not used for "shell").

However your question now makes me believe that
it is not a good idea to systematically report to the user
the exit status of the launched command.
E.g. in a shell, if you do:
   $ echo something | grep somethingelse
the shell does not tell you:
  warning: grep has exited with status 1
which is what the GDB patch does now !
I think it is up to each command to have its way to 
report success or failure.

So, I believe the correct solution is to have a convenience variable
(e.g. $_exit_status) set with the exit status of the last launched
command (either by the GDB shell or pipe command).
This convenience variable can then be looked at by the user if there
is a doubt about how the last command worked.
And more interesting, this convenience variable can be used
in user defined commands.

I will prepare a RFAv3 based on this.

Thanks for the review

Philippe
  
Pedro Alves May 27, 2019, 1:12 p.m. UTC | #3
On 5/4/19 7:24 AM, Philippe Waroquiers wrote:
> On Fri, 2019-05-03 at 19:59 +0100, Pedro Alves wrote:
>> On 4/26/19 9:11 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)
>>
>> I think that making "-dX" a single character is a mistake.  I'd rather
>> make that "-d STRING", so you can use use a string that is much less
>> likely to conflict.  Like bash herestrings.  So a user would be able
>> to do:
>>
>> pipe -d XXXYYYXXX $command | $shell_command
>>
>> I also think "-dX" without a space in between is very non-gdb-ish.
>> In gdb, you kind of either have "-OPT X", or "/OX", which is kind of
>> double "--" vs "-" with getopt, except with different leading
>> tokens.
> 
> Ok, will change to -d STRING.
> 

Thanks.

>>> 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
>>>
>>
>> I get that "it makes sense", but do you see yourself using the "|" command
>> in preference to "pipe"?  "pipe" can be typed as "pi", which is two
>> key strokes as well.  Kind of wondering whether the character could be
>> saved for something else in the future.  I don't have a use in mind,
>> just thinking out loud.
> Yes, I always use "|" (I even just checked the test to be sure that I
> also tested "pipe" and not only "|").
> pip must be used, as pi launches an interactive python.
> 
> Also, "|" allows to do:
> (gdb) some_command
> ...
> (gdb) ||grep something_in_some_command_output
> (gdb) ||tee save_some_command_output
> 
> which is better than
> (gdb) pip|grep something_in_some_command_output

Alright, that does sound clever.

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 5f3b973f06..eefdc2cd02 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"
@@ -854,6 +856,101 @@  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;
+  int separator = '|';
+
+  if (arg == NULL)
+    error (_("Missing \"[COMMAND] | SHELL_COMMAND\""));
+
+  shell_command = skip_spaces (shell_command);
+
+  if (*shell_command == '-' && *(shell_command + 1) == 'd')
+    {
+      separator = *(shell_command + 2);
+      if (separator == 0 || separator == ' ')
+	error (_("Missing separator X after -d in "
+		 "\"| -dX COMMAND X SHELL_COMMAND\""));
+      shell_command += 3;
+      command = shell_command;
+    }
+
+  shell_command = strchr (shell_command, separator);
+
+  if (shell_command == nullptr)
+    {
+      if (separator == '|')
+	error (_("Missing | SHELL_COMMAND in \"| [COMMAND] | SHELL_COMMAND\""));
+      else
+	error (_("Missing %c SHELL_COMMAND in "
+		 "\"| -d%c COMMAND %c SHELL_COMMAND\""),
+	       separator, separator, separator);
+    }
+
+  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++; /* skip the separator.  */
+  shell_command = skip_spaces (shell_command);
+  if (*shell_command == 0)
+    {
+      if (separator == '|')
+	error (_("Missing SHELL_COMMAND in \"| [COMMAND] | SHELL_COMMAND\""));
+      else
+	error (_("Missing SHELL_COMMAND in "
+		 "\"| -d%c COMMAND %c SHELL_COMMAND\""),
+	       separator, separator);
+    }
+
+  FILE *to_shell_command = popen (shell_command, "w");
+
+  if (to_shell_command == NULL)
+    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 shell_command_status = pclose (to_shell_command);
+
+  if (shell_command_status < 0)
+    error (_("shell command  \"%s\" errno %s"), shell_command,
+           safe_strerror (errno));
+  if (shell_command_status > 0)
+    {
+      if (WIFEXITED (shell_command_status))
+	warning (_("shell command \"%s\" exit status %d"), shell_command,
+		 WEXITSTATUS (shell_command_status));
+      else if (WIFSIGNALED (shell_command_status))
+	warning (_("shell command \"%s\" exit with signal %d"), shell_command,
+		 WTERMSIG (shell_command_status));
+      else
+	warning (_("shell command \"%s\" status %d"), shell_command,
+		 shell_command_status);
+    }
+}
+
 static void
 list_command (const char *arg, int from_tty)
 {
@@ -1845,6 +1942,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: pipe [COMMAND] | SHELL_COMMAND\n\
+Usage: | [COMMAND] | SHELL_COMMAND\n\
+Usage: pipe -dX COMMAND X SHELL_COMMAND\n\
+Usage: | -dX COMMAND X SHELL_COMMAND\n\
+Executes COMMAND and sends its output to SHELL_COMMAND.\n\
+If COMMAND contains a | character, the option -dX indicates\n\
+to use the character X 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 == '_'