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

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

[Gareth Rees]

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);
+
   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

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

[Andrew Burgess]

* 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
> 

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

[Gareth Rees]

Andrew Burgess wrote:

> I'm happy with the change to instream being moved, however, I'm not
> sure about the other two lines.


I moved the entire block of settings changes from
execute_control_commands() back to execute_user_command(), because none of
the changes was mentioned in the commit message for 56bcdbea2b, so that all
these changes seemed to be accidental consequences of supporting multi-line
statements.

I'd be happy to resubmit the patch with just the instream change — that's
the only one of the settings that I need to get from_tty working again.

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

[Gareth Rees]

[-- Attachment #1: Type: text/plain, Size: 56 bytes --]

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

[-- Attachment #2: 0001-gdb-Fix-from_tty-argument-to-gdb.execute-in-Python.patch --]
[-- Type: text/x-patch, Size: 4843 bytes --]

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);
+
   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

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

[Andrew Burgess]

* 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
> 

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

[Gareth Rees]

[-- Attachment #1: Type: text/plain, Size: 90 bytes --]

Here's a patch with a comment and commit message updated to use "nullptr"
instead of "0".

[-- Attachment #2: 0001-gdb-Fix-from_tty-argument-to-gdb.execute-in-Python.patch --]
[-- Type: text/x-patch, Size: 4873 bytes --]

From 7efaa3657c8b72c128675b27a2cb2f21e2e05b14 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
nullptr 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
nullptr 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 nullptr here as this breaks the from_tty argument
        to gdb.execute in Python.
        (execute_user_command): set instream to nullptr 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..f8ac610d4d 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 nullptr, indicating execution of a
+     user-defined function.  */
+  scoped_restore restore_instream
+    = make_scoped_restore (&current_ui->instream, nullptr);
+
   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

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

[Andrew Burgess]

* Gareth Rees <grees@undo.io> [2020-09-24 12:16:32 +0100]:

> Here's a patch with a comment and commit message updated to use "nullptr"
> instead of "0".

LGTM.  Do you have commit access to push this?

Thanks,
Andrew


> From 7efaa3657c8b72c128675b27a2cb2f21e2e05b14 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
> nullptr 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
> nullptr 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 nullptr here as this breaks the from_tty argument
>         to gdb.execute in Python.
>         (execute_user_command): set instream to nullptr 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..f8ac610d4d 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 nullptr, indicating execution of a
> +     user-defined function.  */
> +  scoped_restore restore_instream
> +    = make_scoped_restore (&current_ui->instream, nullptr);
> +
>    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
> 

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

[Gareth Rees]

Andrew Burgess wrote:

> LGTM.  Do you have commit access to push this?
>

Thanks for reviewing. I don't have commit access. Also, I haven't signed
any copyright assignment documents yet.

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

[Joel Brobecker]

> > LGTM.  Do you have commit access to push this?
> >
> 
> Thanks for reviewing. I don't have commit access. Also, I haven't signed
> any copyright assignment documents yet.

Thanks for letting us know about the copyright assignment issue.
For this patch, it's sufficiently small that we can accept it
without the assignment.

There is one small change I made prior to pushing it: I added
a reference to the PR in the ChangeLog and the revision log
(as "PR python/26586" at the start of each ChangeLog entry),
so as to make sure the commit gets linked in the bugzilla PR.

@Andrew, would you agree that this patch would be safe enough
for backporting to the gdb-10-branch?

Thanks,
-- 
Joel

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

[Andrew Burgess]

* Joel Brobecker <brobecker@adacore.com> [2020-09-26 11:07:00 -0700]:

> > > LGTM.  Do you have commit access to push this?
> > >
> > 
> > Thanks for reviewing. I don't have commit access. Also, I haven't signed
> > any copyright assignment documents yet.
> 
> Thanks for letting us know about the copyright assignment issue.
> For this patch, it's sufficiently small that we can accept it
> without the assignment.
> 
> There is one small change I made prior to pushing it: I added
> a reference to the PR in the ChangeLog and the revision log
> (as "PR python/26586" at the start of each ChangeLog entry),
> so as to make sure the commit gets linked in the bugzilla PR.
> 
> @Andrew, would you agree that this patch would be safe enough
> for backporting to the gdb-10-branch?

I think so.

Thanks for pushing this.

Andrew

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

[Joel Brobecker]

> > There is one small change I made prior to pushing it: I added
> > a reference to the PR in the ChangeLog and the revision log
> > (as "PR python/26586" at the start of each ChangeLog entry),
> > so as to make sure the commit gets linked in the bugzilla PR.
> > 
> > @Andrew, would you agree that this patch would be safe enough
> > for backporting to the gdb-10-branch?
> 
> I think so.

Perfect, thank you. Pushed to the gdb-10-branch also.

-- 
Joel