[commit] problem sourcing GDB script in interactive-mode on

[commit] problem sourcing GDB script in interactive-mode on

[Joel Brobecker]

When interactive-mode is not auto, GDB always uses that setting to
determine whether to act as if the input stream is a terminal or not.
However, this setting should only be honored if the input stream is
the standard input stream.  Otherwise, we run into trouble while
source-ing a GDB script, as shown below (on x86_64-linux):

        % cat script
        print 1
        print 2
        % gdb  -q
        (gdb) set interactive-mode on
        (gdb) source script
        (gdb) print 3
        $1 = 3

The lack of output and the fact that the "print 3" command returned
a value saved in $1 (as opposed to $3) indicates that the script was
not even evaluated at all.

gdb/ChangeLog:

        * top.c (input_from_terminal_p): Restrict the use of interactive_mode
        to the case where instream is stdin.

gdb/testsuite/ChangeLog:

        * gdb.base/interact.exp: New testcase.

Tested on x86_64-linux.  Checked in.


---
 gdb/ChangeLog                       |    5 +++
 gdb/testsuite/ChangeLog             |    4 +++
 gdb/testsuite/gdb.base/interact.exp |   48 +++++++++++++++++++++++++++++++++++
 gdb/top.c                           |    2 +-
 4 files changed, 58 insertions(+), 1 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/interact.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a23aab9..e8ba178 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2011-01-13  Joel Brobecker  <brobecker@adacore.com>
 
+	* top.c (input_from_terminal_p): Restrict the use of interactive_mode
+	to the case where instream is stdin.
+
+2011-01-13  Joel Brobecker  <brobecker@adacore.com>
+
 	* ia64-tdep.h (struct regcache): Forward declare.
 	(struct ia64_infcall_ops): New struct type.
 	(struct gdbarch_tdep): New fields "find_global_pointer_from_solib"
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index f6577c7..8926527 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2011-01-13  Joel Brobecker  <brobecker@adacore.com>
+
+	* gdb.base/interact.exp: New testcase.
+
 2011-01-12  Tom Tromey  <tromey@redhat.com>
 
 	* gdb.mi/gdb2549.exp: Update for error message changes.
diff --git a/gdb/testsuite/gdb.base/interact.exp b/gdb/testsuite/gdb.base/interact.exp
new file mode 100644
index 0000000..1f15fd8
--- /dev/null
+++ b/gdb/testsuite/gdb.base/interact.exp
@@ -0,0 +1,48 @@
+# Copyright 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Create a GDB script that we can source.  The script needs to generate
+# some output, to allow us to verify that it is executed properly.
+set fd [open "zzz-gdbscript" "w"]
+puts $fd "print 1"
+puts $fd "print 2"
+close $fd
+
+# The expected output from the script...
+set script_output "\\$\[0-9\]+ = 1\[\r\n\]+\\$\[0-9\]+ = 2.*"
+
+# Start a fresh GDB.  We don't need an executable for this test, so
+# nothing else to do in terms of testcase setup.
+gdb_exit
+gdb_start
+
+# Test sourcing of the script with interactive mode `auto'
+gdb_test_no_output "set interactive-mode auto"
+gdb_test "source zzz-gdbscript" "$script_output" \
+         "source script with interactive-mode auto"
+gdb_test "print 3" "= 3" "sanity check with interactive-mode auto"
+
+# Test sourcing of the script with interactive mode `on'
+gdb_test_no_output "set interactive-mode on"
+gdb_test "source zzz-gdbscript" "$script_output" \
+         "source script with interactive-mode on"
+gdb_test "print 4" "= 4" "sanity check with interactive-mode on"
+
+# Test sourcing of the script with interactive mode `of'
+gdb_test_no_output "set interactive-mode off"
+gdb_test "source zzz-gdbscript" "$script_output" \
+         "source script with interactive-mode off"
+gdb_test "print 5" "= 5" "sanity check with interactive-mode off"
+
diff --git a/gdb/top.c b/gdb/top.c
index d974897..bba1a2d 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1292,7 +1292,7 @@ show_interactive_mode (struct ui_file *file, int from_tty,
 int
 input_from_terminal_p (void)
 {
-  if (interactive_mode != AUTO_BOOLEAN_AUTO)
+  if (interactive_mode != AUTO_BOOLEAN_AUTO && instream == stdin)
     return interactive_mode == AUTO_BOOLEAN_TRUE;
 
   if (batch_flag)
-- 
1.7.1

Re: [commit] problem sourcing GDB script in interactive-mode on

[Doug Evans]

On Thu, Jan 13, 2011 at 3:05 PM, Joel Brobecker <brobecker@adacore.com> wrote:
> When interactive-mode is not auto, GDB always uses that setting to
> determine whether to act as if the input stream is a terminal or not.
> However, this setting should only be honored if the input stream is
> the standard input stream.  Otherwise, we run into trouble while
> source-ing a GDB script, as shown below (on x86_64-linux):
>
>        % cat script
>        print 1
>        print 2
>        % gdb  -q
>        (gdb) set interactive-mode on
>        (gdb) source script
>        (gdb) print 3
>        $1 = 3
>
> The lack of output and the fact that the "print 3" command returned
> a value saved in $1 (as opposed to $3) indicates that the script was
> not even evaluated at all.

For completeness sake, I think(!) it's a bit more subtle than this.
A backtrace after the "source script" command shows gdb is reading
commands from the keyboard while inside source_command.
Yikes!

Some uses of input_from_terminal_p also test from_tty.
E.g. if (from_tty && input_from_terminal_p ())
But top.c:command_line_input doesn't do this.
It does test instream == stdin in a few places, but it doesn't do this
test when calling input_from_terminal_p.
Instead it has:

    if (deprecated_readline_hook && input_from_terminal_p ())
      ...
    else if (command_editing_p && input_from_terminal_p ())
      ...

I'd like to be consistent.
Either we require callers of input_from_terminal_p to also test
from_tty (or instream == stdin), or we do the test in
input_from_terminal_p and remove the from_tty test in callers that do
that.

An alternative is doing this in command_line_input:

    if (deprecated_readline_hook && instream == stdin &&
input_from_terminal_p ())
      ...
    else if (command_editing_p && instream == stdin && input_from_terminal_p ())
      ...

There's also the calls to input_from_terminal_p in utils.c to consider.
What should happen to paging, for example, if interactive-mode = on
and we're sourcing a script?
Before your patch, set interactive-mode = on means paging is on even
when sourcing a script.
After your patch, paging is off regardless.
I'm kinda leaning towards patching command_line_input instead of
input_from_terminal_p, but I don't actually know when one would set
interactive-mode, so I don't have a strong opinion other than the
desire for consistency (i.e. if we keep your patch, callers shouldn't
have to test from_tty - I think(!)).

Comments?

P.S. Another comment towards the end of the patch.

> gdb/ChangeLog:
>
>        * top.c (input_from_terminal_p): Restrict the use of interactive_mode
>        to the case where instream is stdin.
>
> gdb/testsuite/ChangeLog:
>
>        * gdb.base/interact.exp: New testcase.
>
> Tested on x86_64-linux.  Checked in.
>
>
> ---
>  gdb/ChangeLog                       |    5 +++
>  gdb/testsuite/ChangeLog             |    4 +++
>  gdb/testsuite/gdb.base/interact.exp |   48 +++++++++++++++++++++++++++++++++++
>  gdb/top.c                           |    2 +-
>  4 files changed, 58 insertions(+), 1 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/interact.exp
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index a23aab9..e8ba178 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,5 +1,10 @@
>  2011-01-13  Joel Brobecker  <brobecker@adacore.com>
>
> +       * top.c (input_from_terminal_p): Restrict the use of interactive_mode
> +       to the case where instream is stdin.
> +
> +2011-01-13  Joel Brobecker  <brobecker@adacore.com>
> +
>        * ia64-tdep.h (struct regcache): Forward declare.
>        (struct ia64_infcall_ops): New struct type.
>        (struct gdbarch_tdep): New fields "find_global_pointer_from_solib"
> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
> index f6577c7..8926527 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,3 +1,7 @@
> +2011-01-13  Joel Brobecker  <brobecker@adacore.com>
> +
> +       * gdb.base/interact.exp: New testcase.
> +
>  2011-01-12  Tom Tromey  <tromey@redhat.com>
>
>        * gdb.mi/gdb2549.exp: Update for error message changes.
> diff --git a/gdb/testsuite/gdb.base/interact.exp b/gdb/testsuite/gdb.base/interact.exp
> new file mode 100644
> index 0000000..1f15fd8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/interact.exp
> @@ -0,0 +1,48 @@
> +# Copyright 2011 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Create a GDB script that we can source.  The script needs to generate
> +# some output, to allow us to verify that it is executed properly.
> +set fd [open "zzz-gdbscript" "w"]
> +puts $fd "print 1"
> +puts $fd "print 2"
> +close $fd
> +
> +# The expected output from the script...
> +set script_output "\\$\[0-9\]+ = 1\[\r\n\]+\\$\[0-9\]+ = 2.*"
> +
> +# Start a fresh GDB.  We don't need an executable for this test, so
> +# nothing else to do in terms of testcase setup.
> +gdb_exit
> +gdb_start
> +
> +# Test sourcing of the script with interactive mode `auto'
> +gdb_test_no_output "set interactive-mode auto"
> +gdb_test "source zzz-gdbscript" "$script_output" \
> +         "source script with interactive-mode auto"
> +gdb_test "print 3" "= 3" "sanity check with interactive-mode auto"
> +
> +# Test sourcing of the script with interactive mode `on'
> +gdb_test_no_output "set interactive-mode on"
> +gdb_test "source zzz-gdbscript" "$script_output" \
> +         "source script with interactive-mode on"
> +gdb_test "print 4" "= 4" "sanity check with interactive-mode on"
> +
> +# Test sourcing of the script with interactive mode `of'
> +gdb_test_no_output "set interactive-mode off"
> +gdb_test "source zzz-gdbscript" "$script_output" \
> +         "source script with interactive-mode off"
> +gdb_test "print 5" "= 5" "sanity check with interactive-mode off"
> +
> diff --git a/gdb/top.c b/gdb/top.c
> index d974897..bba1a2d 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -1292,7 +1292,7 @@ show_interactive_mode (struct ui_file *file, int from_tty,
>  int
>  input_from_terminal_p (void)
>  {
> -  if (interactive_mode != AUTO_BOOLEAN_AUTO)
> +  if (interactive_mode != AUTO_BOOLEAN_AUTO && instream == stdin)
>     return interactive_mode == AUTO_BOOLEAN_TRUE;

It's kinda weird to always fall through to the AUTO case when instream
!= stdin, but maybe it's ok.

>   if (batch_flag)
> --
> 1.7.1
>
>

Re: [commit] problem sourcing GDB script in interactive-mode on

[Joel Brobecker]

Thanks for the thorough review!

> There's also the calls to input_from_terminal_p in utils.c to consider.
> What should happen to paging, for example, if interactive-mode = on
> and we're sourcing a script?

I think we should be doing exactly the same as what is happening when
interactive mode is auto.

To answer one of your questions, the purpose of this setting is to
force GDB to think that we're running on a terminal.  This is meant
to be used on Windows, when GDB is configured as a MinGW debugger,
but running inside a cygwin window.  Cygwin implements its "terminal"
as a pair of pipes, and so the MinGW debugger doesn't realize that,
from the user's perspective, we are inside a terminal.

This should explain the patch as it is: I was only being consistent
with what we already do in the rest of the function:

  if (gdb_has_a_terminal () && instream == stdin)
    return 1;

Note what the description of input_from_terminal_p says:

/* Returns whether GDB is running on a terminal and input is
   currently coming from that terminal.  */

But I realize, now, that the option might be poorly named.  The name
I chose does seem to suggest that an interactive mode "on" means
interactive behavior at all times, including while evaluating scripts.

I think that it doesn't really make much sense to force interactive
mode and paging while evaluating a script.  And given also the
description of input_from_terminal_p ("input is currently comming
from that terminal"), I'm actually leaning towards keeping the
"instream == stdin" check in there.

I'm not really surea about the from_tty check, but I think it's
othogonal?

-- 
Joel