[RFA,2/4] Implement | (pipe) command.

Message ID 20190420212153.30934-3-philippe.waroquiers@skynet.be
State New, archived
Headers

Commit Message

Philippe Waroquiers April 20, 2019, 9:21 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
  Executes COMMAND and sends its output to 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-20  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   | 91 ++++++++++++++++++++++++++++++++++++++++++++
 gdb/cli/cli-decode.c |  4 +-
 2 files changed, 93 insertions(+), 2 deletions(-)
  

Comments

Eli Zaretskii April 21, 2019, 5:19 a.m. UTC | #1
> From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Date: Sat, 20 Apr 2019 23:21:51 +0200
> 
> +  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));

These macros will need to be ported to MinGW, since the current
definitions in gdb_wait.h are for Posix systems (and never used on
anything other than that).  Gnulib has replacements, but they are less
functional on Windows than I'd like them to be, in particular when the
shell program exited due to a signal.  I can provide better
replacements if you want.
  
Philippe Waroquiers April 21, 2019, 9:40 a.m. UTC | #2
On Sun, 2019-04-21 at 08:19 +0300, Eli Zaretskii wrote:
> > From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> > Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> > Date: Sat, 20 Apr 2019 23:21:51 +0200
> > 
> > +  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));
> 
> These macros will need to be ported to MinGW, since the current
> definitions in gdb_wait.h are for Posix systems (and never used on
> anything other than that).  Gnulib has replacements, but they are less
> functional on Windows than I'd like them to be, in particular when the
> shell program exited due to a signal.  I can provide better
> replacements if you want.
It would be nice to have better replacement for Windows,
as I do not have the knowledge and needed setup to test
for this platform.

Thanks for the review.

Philippe
  
Eli Zaretskii April 21, 2019, 10:18 a.m. UTC | #3
> From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Cc: gdb-patches@sourceware.org
> Date: Sun, 21 Apr 2019 11:40:22 +0200
> 
> > > +  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));
> > 
> > These macros will need to be ported to MinGW, since the current
> > definitions in gdb_wait.h are for Posix systems (and never used on
> > anything other than that).  Gnulib has replacements, but they are less
> > functional on Windows than I'd like them to be, in particular when the
> > shell program exited due to a signal.  I can provide better
> > replacements if you want.
> It would be nice to have better replacement for Windows,
> as I do not have the knowledge and needed setup to test
> for this platform.

Here's my proposal:

#define WIFEXITED(stat_val)   (((stat_val) & 0xC0000000) == 0)
#define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) == 0xC0000000)
#define WEXITSTATUS(stat_val) ((stat_val) & 255)
#define WTERMSIG(stat_val)    windows_status_to_termsig (stat_val)

where windows_status_to_termsig should use the xlate[] array defined
in windows-nat.c, like the (ifdef'ed away) code in
windows_nat_target::resume does.

The underlying idea is that when a Windows program is terminated by a
fatal exception, its exit code is the value of that exception, as
defined by the various STATUS_* symbols in the Windows API headers.

The above is not perfect, because a program could legitimately exit
normally with a status whose value happens to have the high bits set,
but that's extremely rare, to say the least, and I think such a
negligibly small probability of false positives is justified by the
utility of reporting the terminating signal in the "normal" cases.
  
Philippe Waroquiers April 22, 2019, 10:30 a.m. UTC | #4
On Sun, 2019-04-21 at 13:18 +0300, Eli Zaretskii wrote:
> Here's my proposal:
> 
> #define WIFEXITED(stat_val)   (((stat_val) & 0xC0000000) == 0)
> #define WIFSIGNALED(stat_val) (((stat_val) & 0xC0000000) == 0xC0000000)
> #define WEXITSTATUS(stat_val) ((stat_val) & 255)
> #define WTERMSIG(stat_val)    windows_status_to_termsig (stat_val)
> 
> where windows_status_to_termsig should use the xlate[] array defined
> in windows-nat.c, like the (ifdef'ed away) code in
> windows_nat_target::resume does.
> 
> The underlying idea is that when a Windows program is terminated by a
> fatal exception, its exit code is the value of that exception, as
> defined by the various STATUS_* symbols in the Windows API headers.
> 
> The above is not perfect, because a program could legitimately exit
> normally with a status whose value happens to have the high bits set,
> but that's extremely rare, to say the least, and I think such a
> negligibly small probability of false positives is justified by the
> utility of reporting the terminating signal in the "normal" cases.
Thanks for the above.

To confirm what to do, I will try to explain my current understanding:

Currently, on MingW, the above macros (WIFEXITED, WIFSIGNALED, ...)
are not defined.

So, the idea is to have gdb_wait.h defining them on MingW,
with something like:

#ifndef	WIFEXITED
#if defined (__MINGW32__)
#define WIFEXITED(stat_val)   (((stat_val) & 0xC0000000) == 0)
#else
#define WIFEXITED(w)	(((w)&0377) == 0)
#endif
#endif

Then in windows-nat.c, implement windows_status_to_termsig
that searches in xlate for stat_val & ~0xC0000000 to give
the equivalent signal.

Does the above look reasonable ?

Thanks

Philippe
  
Eli Zaretskii April 22, 2019, 11:08 a.m. UTC | #5
> From: Philippe Waroquiers <philippe.waroquiers@skynet.be>
> Cc: gdb-patches@sourceware.org
> Date: Mon, 22 Apr 2019 12:30:00 +0200
> 
> Currently, on MingW, the above macros (WIFEXITED, WIFSIGNALED, ...)
> are not defined.

I think gdb_wait.h does define them for MinGW, it's just that those
definitions are currently unused anywhere that is compiled into the
MinGW build.

> So, the idea is to have gdb_wait.h defining them on MingW,
> with something like:
> 
> #ifndef	WIFEXITED
> #if defined (__MINGW32__)
> #define WIFEXITED(stat_val)   (((stat_val) & 0xC0000000) == 0)
> #else
> #define WIFEXITED(w)	(((w)&0377) == 0)
> #endif
> #endif
> 
> Then in windows-nat.c, implement windows_status_to_termsig
> that searches in xlate for stat_val & ~0xC0000000 to give
> the equivalent signal.
> 
> Does the above look reasonable ?

Yes, thanks.
  
Tom Tromey April 24, 2019, 8:50 p.m. UTC | #6
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> +  while (*shell_command && *shell_command != '|')
Philippe> +    shell_command++;

I think using strchr would be preferable here.

Philippe> +  if (gdb_cmd.length () == 0)

Use .empty instead.

Philippe> +      if (gdb_cmd.length () == 0)

Likewise.

Philippe> +  to = popen (shell_command, "w");

I wonder if it's better to use libiberty's pexecute code?
This may avoid the WIFEXITED problems.

Philippe> +	/* In case GDB_CMD switches of inferior/thread/frame, the below
Philippe> +	   restores the inferior/thread/frame.  */
Philippe> +	scoped_restore_current_thread restore;

What's the rationale for this?

Philippe> +	gdb_cmd_result = execute_command_to_string (gdb_cmd.c_str (),
Philippe> +						    from_tty);
Philippe> +
Philippe> +	if (fwrite (gdb_cmd_result.c_str (), 1, gdb_cmd_result.length (), to)
Philippe> +	    != gdb_cmd_result.length ())
Philippe> +	  {
Philippe> +	    error (_("error writing \"%s\" result to \"%s\", errno %s"),
Philippe> +		   gdb_cmd.c_str (), shell_command, safe_strerror (errno));
Philippe> +	  }

Maybe it would be preferable to redirect gdb's actual output here
instead, using whatever mechanism it is that "set logging" uses.

Tom
  
Eli Zaretskii April 25, 2019, 5:58 a.m. UTC | #7
> From: Tom Tromey <tom@tromey.com>
> Cc: gdb-patches@sourceware.org
> Date: Wed, 24 Apr 2019 14:50:47 -0600
> 
> Philippe> +  to = popen (shell_command, "w");
> 
> I wonder if it's better to use libiberty's pexecute code?
> This may avoid the WIFEXITED problems.

Libiberty's pexecute doesn't solve the WIFEXITED problem, AFAICT, it
just returns the exit status to the caller, and I see no facilities
there to help interpret that status with platform-independent code.
Did I miss something?
  

Patch

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index 5f3b973f06..6964f9e1ab 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -24,6 +24,8 @@ 
 #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 "gdbthread.h"
 #include "gdb_regex.h"	/* Used by apropos_command.  */
 #include "gdb_vfork.h"
 #include "linespec.h"
@@ -854,6 +856,86 @@  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)
+{
+  FILE *to;
+  const char *shell_command = arg;
+
+  if (arg == NULL)
+    error (_("Missing \"[COMMAND] | SHELL_COMMAND\""));
+
+  while (*shell_command && *shell_command != '|')
+    shell_command++;
+
+  if (*shell_command != '|')
+    error (_("Missing | SHELL_COMMAND in \"| [COMMAND] | SHELL_COMMAND\""));
+
+  arg = skip_spaces (arg);
+  std::string gdb_cmd (arg, shell_command - arg);
+
+  if (gdb_cmd.length () == 0)
+    {
+      repeat_previous ();
+      gdb_cmd = skip_spaces (saved_command_line);
+      if (gdb_cmd.length () == 0)
+	error (_("No previous command to relaunch"));
+    }
+
+  shell_command++; /* skip the |.  */
+  shell_command = skip_spaces (shell_command);
+  if (*shell_command == 0)
+    error (_("Missing SHELL_COMMAND in \"| [COMMAND] | SHELL_COMMAND\""));
+
+  to = popen (shell_command, "w");
+  if (to == NULL)
+    error (_("Error launching \"%s\""), shell_command);
+
+  try
+    {
+      std::string gdb_cmd_result;
+      {
+	/* In case GDB_CMD switches of inferior/thread/frame, the below
+	   restores the inferior/thread/frame.  */
+	scoped_restore_current_thread restore;
+
+	gdb_cmd_result = execute_command_to_string (gdb_cmd.c_str (),
+						    from_tty);
+
+	if (fwrite (gdb_cmd_result.c_str (), 1, gdb_cmd_result.length (), to)
+	    != gdb_cmd_result.length ())
+	  {
+	    error (_("error writing \"%s\" result to \"%s\", errno %s"),
+		   gdb_cmd.c_str (), shell_command, safe_strerror (errno));
+	  }
+      }
+    }
+  catch (const gdb_exception_error &ex)
+    {
+      pclose (to);
+      throw;
+    }
+
+  int shell_command_status = pclose (to);
+  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 +1927,15 @@  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\
+Executes COMMAND and sends its output to 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 == '_'