From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from eggs.gnu.org (eggs.gnu.org [IPv6:2001:470:142:3::10]) by sourceware.org (Postfix) with ESMTPS id 642173858D32 for ; Sat, 11 Feb 2023 08:03:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 642173858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gnu.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gnu.org Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pQkqd-0006sW-Ay; Sat, 11 Feb 2023 03:03:04 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=zP+fB425f9FMKfHhO1HgemdHhoMlYxRrb723fU7cz78=; b=m1T8qWAwJASE M17Ry50+j5a3k+fwNcdsEUgsiYxYdA9vHzOkEnBd69DinywapSm+nA+GccxiH2lU5t7tkwqKD+5ip FzIIYvkG+e5QrUBt9vbKTcalj3a/zTsKpB06KbufepV5Df03nHBxpuBLVJMaP5p/LZAb+KouzmVT9 nAWvGIdsziAongLeUn7x+NU6u/CpY6qGMAd7F3EswRNYqWr3TvXYftnhJrTVEgLWoPc2NJGKfLPpp uZoVW+XZzk4UINVAe6rNtGOuKfd7HwqMD2n499PLbuTqfNKavczL+AuMD7TgyFdkknavc5ofDOgiq aiGPpWMZQ41Gv2FfuayiKQ==; Received: from [87.69.77.57] (helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pQkqb-00078g-6u; Sat, 11 Feb 2023 03:03:02 -0500 Date: Sat, 11 Feb 2023 10:02:29 +0200 Message-Id: <83r0uwk4tm.fsf@gnu.org> From: Eli Zaretskii To: Pedro Alves Cc: gdb-patches@sourceware.org In-Reply-To: <20230210233604.2228450-4-pedro@palves.net> (message from Pedro Alves on Fri, 10 Feb 2023 23:36:01 +0000) Subject: Re: [PATCH 3/6] Add new "$_shell(CMD)" internal function References: <20230210233604.2228450-1-pedro@palves.net> <20230210233604.2228450-4-pedro@palves.net> X-Spam-Status: No, score=-2.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: > From: Pedro Alves > Date: Fri, 10 Feb 2023 23:36:01 +0000 > > gdb/NEWS | 10 ++++ > gdb/cli/cli-cmds.c | 89 ++++++++++++++++++++++++++++-- > gdb/doc/gdb.texinfo | 47 ++++++++++++++++ > gdb/testsuite/gdb.base/default.exp | 1 + > gdb/testsuite/gdb.base/shell.exp | 36 ++++++++++++ > 5 files changed, 179 insertions(+), 4 deletions(-) Thanks. > +* New convenience function "$_shell", to execute a shell command and > + return the result. This lets you run shell commands in expressions. > + Some examples: > + > + (gdb) p $_shell("true") > + $1 = 0 > + (gdb) p $_shell("false") > + $2 = 1 > + (gdb) break func if $_shell("some command") == 0 > + > * MI changes This part is OK. > -static void > -shell_escape (const char *arg, int from_tty) > +/* Run ARG under the shell, and return the exit status. If ARG is > + NULL, run an interactive shell. */ > + > +static int > +run_under_shell (const char *arg, int from_tty) > { > #if defined(CANT_FORK) || \ > (!defined(HAVE_WORKING_VFORK) && !defined(HAVE_WORKING_FORK)) > @@ -915,7 +922,7 @@ shell_escape (const char *arg, int from_tty) > the shell command we just ran changed it. */ > chdir (current_directory); > #endif > - exit_status_set_internal_vars (rc); > + return rc; > #else /* Can fork. */ > int status, pid; > > @@ -942,10 +949,21 @@ shell_escape (const char *arg, int from_tty) > waitpid (pid, &status, 0); > else > error (_("Fork failed")); > - exit_status_set_internal_vars (status); > + return status; > #endif /* Can fork. */ > } This part seems to leave in place the error message in case invoking the shell command failed (i.e. 'system' or 'execl' failed). I wonder whether we should still show that error message if this was called via the new built-in function. Perhaps it would be better to instead return a distinct return value? Having these messages emitted during breakpoint commands, for example, disrupts the "normal" display of breakpoint data and output from breakpoint commands, so perhaps it is better to leave the handling of this situation to the caller? And another comment/question: on Posix hosts this calls fork/exec, so if GDB was configured to follow forks, does it mean it might start debugging the program invoked via the shell function? if so, what does that mean for using that function in breakpoint commands? If there is some subtleties to be aware of here, I think at least the manual should mention them. > + add_internal_function ("_shell", _("\ > +$_shell - execute a shell command and return the result.\n\ > +Usage: $_shell (command)\n\ > +Returns the command's exit code: zero on success, non-zero otherwise."), ^^^^^^^ I think our usual style is to say "Return", not "Returns". > +@anchor{$_shell convenience function} > +@item $_shell (@var{command-string}) > +@findex $_shell@r{, convenience function} @fined should be _before_ @item, so that the index records the position of @item, and the Info reader places you before the @item when you use index-search. > +Invoke a standard shell to execute @var{command-string}. Returns the ^^^^^^^^^^^^^^^^ "the standard system shell" is better, I think. > +command's exit status. On Unix systems, a command which exits with a > +zero exit status has succeeded, and non-zero exit status indicates > +failure. When a command terminates on a fatal signal whose number is > +N, @value{GDBN} uses the value 128+N as the exit status, as is > +standard in Unix shells. "N" should be "@var{n}" here. Also, this text could benefit from a cross-reference to where we describe the commands that convert from signal numbers to their mnemonics, since that is system-dependent. Maybe "info signal N" is the right tool here? If so, a cross-reference to "Signals" is what we should have here. (Is there a better way of asking GDB about signal number N?) > +In this scenario, you'll want to make sure that the shell command you > +run in the breakpoint condition takes the least amount of time > +possible. This is important to minimize the time it takes to evaluate > +the condition and re-resume the program if the condition turns out to > +be false. I understand what this says, but not what it means in practice. Perhaps one or two additional sentences to help the reader understand how to "make sure the shell command in a breakpoint condition takes the least amount of time" would be in order? Reviewed-by: Eli Zaretskii