Re: [PATCH] gdb: Fix from_tty argument to gdb.execute in Python.

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Gareth Rees <grees@undo.io> [2020-09-22 11:23:43 +0100]:

> Prior to commit 56bcdbea2b, the from_tty keyword argument to the
> Python function gdb.execute controlled whether the command took input
> from the terminal. When from_tty=True, the "starti" command prompted
> the user:
> 
>     (gdb) python gdb.execute("starti", from_tty=True)
>     The program being debugged has been started already.
>     Start it from the beginning? (y or n) y
>     Starting program: /bin/true
> 
>     Program stopped.
> 
> When from_tty=False, it did not prompt the user, and "yes" was assumed:
> 
>     (gdb) python gdb.execute('starti', from_tty=False)
> 
>     Program stopped.
> 
> However, after commit 56bcdbea2b, the from_tty keyword argument no
> longer had this effect. For example, as of commit 7ade7fba75:
> 
>     (gdb) python gdb.execute('starti', from_tty=True)
>     The program being debugged has been started already.
>     Start it from the beginning? (y or n) [answered Y; input not from terminal]
>     Starting program: /bin/true
> 
>     Program stopped.
> 
> Note the "[answered Y; input not from terminal]" in the output even
> though from_tty=True was requested.
> 
> Looking at commit 56bcdbea2b, it seems that the behaviour of the
> from_tty argument was changed accidentally. The commit message said:
> 
>     Let gdb.execute handle multi-line commands
> 
>     This changes the Python API so that gdb.execute can now handle
>     multi-line commands, like "commands" or "define".
> 
> and there was no mention of changing the effect of the from_tty
> argument. It looks as though the code for setting the instream to 0
> was accidentally moved from execute_user_command() to
> execute_control_commands() along with the code for iterating over a
> series of command lines.
> 
> Accordingly, the simplest way to fix this is to partially reverse
> commit 56bcdbea2b by moving the code for setting the instream to 0
> back to execute_user_command() where it was to begin with.
> 
> Additionally, add a test case to reduce the risk of similar breakage
> in future.
> 
> gdb/ChangeLog:
> 
> 	* cli/cli-script.c (execute_control_commands): don't set
>         instream to 0 here as this breaks the from_tty argument to
>         gdb.execute in Python.
>         (execute_user_command): set instream to 0 here instead.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.python/python.exp: add test cases for the from_tty
>         argument to gdb.execute.
> ---
>  gdb/cli/cli-script.c                | 16 ++++++++--------
>  gdb/testsuite/gdb.python/python.exp | 13 +++++++++++++
>  2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index da4a41023a..4adcda85e6 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -392,14 +392,6 @@ execute_cmd_post_hook (struct cmd_list_element *c)
>  void
>  execute_control_commands (struct command_line *cmdlines, int from_tty)
>  {
> -  /* Set the instream to 0, indicating execution of a
> -     user-defined function.  */
> -  scoped_restore restore_instream
> -    = make_scoped_restore (&current_ui->instream, nullptr);
> -  scoped_restore save_async = make_scoped_restore (&current_ui->async, 0);
> -  scoped_restore save_nesting
> -    = make_scoped_restore (&command_nest_depth, command_nest_depth + 1);
> -
>    while (cmdlines)
>      {
>        enum command_control_type ret = execute_control_command (cmdlines,
> @@ -464,6 +456,14 @@ execute_user_command (struct cmd_list_element *c, const char *args)
>    if (user_args_stack.size () > max_user_call_depth)
>      error (_("Max user call depth exceeded -- command aborted."));
>  
> +  /* Set the instream to 0, indicating execution of a
> +     user-defined function.  */
> +  scoped_restore restore_instream
> +    = make_scoped_restore (&current_ui->instream, nullptr);
> +  scoped_restore save_async = make_scoped_restore (&current_ui->async, 0);
> +  scoped_restore save_nesting
> +    = make_scoped_restore (&command_nest_depth, command_nest_depth + 1);

I'm happy with the change to instream being moved, however, I'm not
sure about the other two lines.  command_nest_depth especially I think
should be left where it is now, with current HEAD my gdb session looks
like this:

  ....
  (gdb) set trace-commands on
  (gdb) python gdb.execute ("starti", False)
  +python gdb.execute ("starti", False)
  ++starti

  Program stopped.

And with your patch:

  ...
  (gdb) set trace-commands on
  (gdb) python gdb.execute ("starti", False)
  +python gdb.execute ("starti", False)
  +starti

  Program stopped.

I think the version in current HEAD is better.

I'm tempted to think the async change should also remain in its
current location.  I believe this means that if GDB is in async mode,
a Python gdb.execute of something like next/continue will operate in
synchronous mode before returning.  Though I don't know if that would
mean there's no way to initiate async control from Python....

... I think you might want to see what others have to say on this.

Thanks,
Andrew



> +
>    execute_control_commands (cmdlines, 0);
>  }
>  
> diff --git a/gdb/testsuite/gdb.python/python.exp b/gdb/testsuite/gdb.python/python.exp
> index a031ea5a18..017f33afe5 100644
> --- a/gdb/testsuite/gdb.python/python.exp
> +++ b/gdb/testsuite/gdb.python/python.exp
> @@ -526,3 +526,16 @@ gdb_test "print \$cvar3" "= void" \
>  # Test PR 23669, the following would invoke the "commands" command instead of
>  # "show commands".
>  gdb_test "python gdb.execute(\"show commands\")" "$decimal  print \\\$cvar3.*"
> +
> +# Test that the from_tty argument to gdb.execute is effective. If
> +# False, the user is not prompted for decisions such as restarting the
> +# program, and "yes" is assumed. If True, the user is prompted.
> +gdb_test "python gdb.execute('starti', from_tty=False)" \
> +    "Program stopped.*" \
> +    "starti via gdb.execute, not from tty"
> +gdb_test_multiple "python gdb.execute('starti', from_tty=True)" \
> +    "starti via gdb.execute, from tty" {
> +    -re {The program being debugged has been started already\.\r\nStart it from the beginning\? \(y or n\) $} {
> +	gdb_test "y" "Starting program:.*" "starti via interactive input"
> +    }
> +}
> -- 
> 2.26.0
>