Re: [PATCH 11/12] gdb: add timeouts for inferior function calls

[Eli Zaretskii <eliz@gnu.org>]

> Date: Fri, 21 Oct 2022 09:43:47 +0100
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
> 
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -115,6 +115,22 @@ set debug infcall on|off
>  show debug infcall
>    Print additional debug messages about inferior function calls.
>  
> +set direct-call-timeout SECONDS
> +show direct-call-timeout
> +set indirect-call-timeout SECONDS
> +show indirect-call-timeout
> +  These new settings can be used to limit how long GDB will wait for
> +  an inferior function call to complete.  The direct timeout is used
> +  for inferior function calls from e.g. 'call' and 'print' commands,
> +  while the indirect timeout is used for inferior function calls from
> +  within a conditional breakpoint expression.
> +
> +  The default for the direct timeout is unlimited, while the default
> +  for the indirect timeout is 300 seconds.

IMO, 300 seconds is way too long a wait for a breakpoint condition.
It should be a matter of a few seconds at most, IMO.

> +  These timeouts will only have an effect for targets that are
> +  operating in async mode.

This should tell what happens on targets that don't support async
mode.

> +When calling a function within a program, it is possible that the
> +program could enter a state from which the called function may never
> +return.  If this happens then @value{GDBN} will appear to hang.
> +Should this happen then it is possible to interrupt the running
> +program by typing the interrupt character (often @kbd{Ctrl-c}).
> +
> +On some targets @value{GDBN} can also place a timeout on any function
> +calls made into the program.

Instead of "some targets", which leaves those targets unspecified, we
should say "targets that support async mode", with a cross-reference
to where that is described.  We should give the user a means to
determine whether the particular target does or doesn't need/support
this timeout feature.

>                              If the timeout expires and the function
> +call is still going, then @value{GDBN} will interrupt the program
> +automatically.

And what does this mean for the value returned by the interrupted
call?  This is important for the breakpoint condition use case, for
example.

> +
> +@table @code
> +@item set direct-call-timeout @var{seconds}
> +@kindex set direct-call-timeout
> +@cindex timeout for called functions
> +Set the timeout used when calling functions in the program to
> +@var{seconds}, which should be an integer greater than zero, or the
> +special value @code{unlimited}, which indicates no timeout should be
> +used.  The default for this setting is @code{unlimited}.

Why integer in seconds? don't we want to be able to support shorter
timeouts, like 100 msec?  Most inferior calls should take much less
than a second, so a second resolution is not the best idea, IMO.  It
could, for example, make running a program with such a breakpoint
unbearably slow.

> +This setting only works for targets that support asynchronous
> +execution (@pxref{Background Execution}), for any other target the
> +setting is treated as @code{unlimited}.

This should be moved to the beginning of the description, as mentioned
above.  In addition, saying "treated as 'unlimited'" is not clear
enough in this context, because actually no timeout is applicable at
all, and GDB will wait indefinitely for the call to return.  We should
tell this explicitly.

> +It is also possible to call functions within the program from the
> +condition of a conditional breakpoint (@pxref{Conditions, ,Break
> +Conditions}).  A different setting controls the timeout used for
> +functions made from a breakpoint condition.
   ^^^^^^^^^^^^^^
"function calls made..."

> +@item set indirect-call-timeout @var{seconds}
> +@kindex set indirect-call-timeout
> +@cindex timeout for called functions
> +Set the timeout used when calling functions in the program from a
> +breakpoint condition to @var{seconds}, which should be an integer
> +greater than zero, or the special value @code{unlimited}, which
> +indicates no timeout should be used.  The default for this setting is
> +@code{300} seconds.

Here 300 seconds is definitely too long.

> +  add_setshow_uinteger_cmd ("direct-call-timeout", no_class,
> +			    &direct_call_timeout, _("\
> +Set the timeout, for direct calls to inferior function calls."), _("\
> +Show the timeout, for direct calls to inferior function calls."), _("\
> +If running on a target that supports, and is running in, async mode\n\
> +then this timeout is used for any inferior function calls triggered\n\
> +directly from the prompt, e.g. from a 'call' or 'print' command.  The\n\
> +timeout is specified in seconds."),
> +			    nullptr,
> +			    show_direct_call_timeout,
> +			    &setlist, &showlist);
> +
> +  add_setshow_uinteger_cmd ("indirect-call-timeout", no_class,
> +			    &indirect_call_timeout, _("\
> +Set the timeout, for indirect calls to inferior function calls."), _("\
> +Show the timeout, for indirect calls to inferior function calls."), _("\
> +If running on a target that supports, and is running in, async mode\n\
> +then this timeout is used for any inferior function calls triggered\n\
> +indirectly, e.g. when evaluating a conditional breakpoint expression.\n\
> +The timeout is specified in seconds."),

These doc strings explain what is a "direct" vs "indirect" call by way
of "e.g.".  But that leaves the issue not well-defined, because it
begs the question: what are the other cases that are considered
"direct" or "indirect"?