Re: [PATCH 3/6] Add new "$_shell(CMD)" internal function

[Eli Zaretskii <eliz@gnu.org>]

> From: Pedro Alves <pedro@palves.net>
> 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 <eliz@gnu.org>