From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 39529 invoked by alias); 27 May 2019 17:48:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 39442 invoked by uid 89); 27 May 2019 17:48:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=sk:lookup_, DEL, theyre, launching X-HELO: mail-wr1-f67.google.com Received: from mail-wr1-f67.google.com (HELO mail-wr1-f67.google.com) (209.85.221.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 27 May 2019 17:48:23 +0000 Received: by mail-wr1-f67.google.com with SMTP id d18so17570073wrs.5 for ; Mon, 27 May 2019 10:48:17 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:4eeb:42ff:feef:f164? ([2001:8a0:f913:f700:4eeb:42ff:feef:f164]) by smtp.gmail.com with ESMTPSA id 19sm250085wmi.10.2019.05.27.10.48.14 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Mon, 27 May 2019 10:48:15 -0700 (PDT) Subject: Re: [RFAv3 4/6] Implement | (pipe) command. To: Philippe Waroquiers , gdb-patches@sourceware.org References: <20190504161753.15530-1-philippe.waroquiers@skynet.be> <20190504161753.15530-5-philippe.waroquiers@skynet.be> From: Pedro Alves Message-ID: Date: Mon, 27 May 2019 17:48:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190504161753.15530-5-philippe.waroquiers@skynet.be> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-05/txt/msg00591.txt.bz2 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 > > * 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