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-23 12:20:06 +0100]:

> Here's the updated patch with just the instream change.

> From 92db8f79ee0dbfaaf689210a4656513a55a82d69 Mon Sep 17 00:00:00 2001
> From: Gareth Rees <grees@undo.io>
> Date: Tue, 8 Sep 2020 15:52:44 +0100
> Subject: [PATCH] gdb: Fix from_tty argument to gdb.execute in Python.
> 
> 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, "starti" and similar commands
> 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, these commands 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 other scoped restores.
> 
> 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                |  9 +++++----
>  gdb/testsuite/gdb.python/python.exp | 13 +++++++++++++
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> index da4a41023a..97541fcca3 100644
> --- a/gdb/cli/cli-script.c
> +++ b/gdb/cli/cli-script.c
> @@ -392,10 +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);
> @@ -464,6 +460,11 @@ 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);

Could you change the comment to read:

  /* Set the instream to nullptr, indicating execution of a
     user-defined function.  */

I'm happy to see this merged with this change.  If anyone else thinks
we should move either of the other two parts back they can do so in a
follow up commit.

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
>