[patch 0/3] fix some gdb.base/paginate-bg-execution.exp problems

[patch 0/3] fix some gdb.base/paginate-bg-execution.exp problems

[Sandra Loosemore]

This series of patches fixes some problems I ran into testing 
gdb.base/paginate-bg-execution.exp on a remote Windows host via ssh -T 
(that is, no TTY).

Part 1 is the fix previously suggested by Pedro here:
https://sourceware.org/ml/gdb-patches/2015-09/msg00637.html

Part 2 is another fix to the pagination code, and part 3 fixes a think-o 
in the test .exp file itself.

The results for this test with these patches look better, but I'm still 
getting some pattern match failures due to too many end-of-line \r 
characters in the output.  I still need to investigate that; I think we 
have a patch on our local branch that's preventing those extra 
characters from being generated, but I'm seeing different failures there 
instead.  :-S  Anyway, I think that is separate from the pagination/TTY 
issues, so I'd like to get the fixes I have in, at least.

-Sandra

[patch 1/3] make prompt_for_continue call throw_quit directly

[Sandra Loosemore]

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

This is the patch previously suggested by Pedro here:

https://sourceware.org/ml/gdb-patches/2015-09/msg00637.html

to fix a problem I ran into when testing on a remote Windows host via 
SSH without a terminal or job control.  As Pedro said, when typing 'q' 
in response to a pagination prompt, there's no possibility of a SIGINT 
so we can bypass the parts of quit() handling that are intended to 
intercept ^C instead.

I confirmed that this fixes the bogus quit message I observed previously 
on Windows host, and did full regression testing on Linux host for 
nios2-linux-gnu target.

OK to commit?

-Sandra


[-- Attachment #2: gdb-paginate-1.log --]
[-- Type: text/x-log, Size: 130 bytes --]

2015-12-18  Sandra Loosemore  <sandra@codesourcery.com>

	gdb/
	* utils.c (prompt_for_continue): Call throw_quit directly on 'q'.

[-- Attachment #3: gdb-paginate-1.patch --]
[-- Type: text/x-patch, Size: 309 bytes --]

diff --git a/gdb/utils.c b/gdb/utils.c
index 284fbbb..862a802 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1868,7 +1868,7 @@ prompt_for_continue (void)
       while (*p == ' ' || *p == '\t')
 	++p;
       if (p[0] == 'q')
-	quit ();
+	throw_quit ("Quit");
       xfree (ignore);
     }
   immediate_quit--;

Re: [patch 1/3] make prompt_for_continue call throw_quit directly

[Pedro Alves]

On 12/18/2015 07:30 PM, Sandra Loosemore wrote:
> This is the patch previously suggested by Pedro here:
> 
> https://sourceware.org/ml/gdb-patches/2015-09/msg00637.html
> 
> to fix a problem I ran into when testing on a remote Windows host via 
> SSH without a terminal or job control.  As Pedro said, when typing 'q' 
> in response to a pagination prompt, there's no possibility of a SIGINT 
> so we can bypass the parts of quit() handling that are intended to 
> intercept ^C instead.
> 
> I confirmed that this fixes the bogus quit message I observed previously 
> on Windows host, and did full regression testing on Linux host for 
> nios2-linux-gnu target.
> 
> OK to commit?

OK.  Could you add a little comment to prevent someone from deciding later
that we should using quit() instead?

Thanks,
Pedro Alves

[patch 2/3] reset pagination counts even when stdin is not a tty

[Sandra Loosemore]

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

In testing gdb.base/paginate-bg-execution.exp on a remote Windows host 
via ssh -T, I observed this output:

set height 2
(gdb) PASS: gdb.base/paginate-bg-execution.exp: paginate: set height 2
continue&
---Type <return> to continue, or q <return> to quit---ERROR: Window too 
small.
UNRESOLVED: gdb.base/paginate-bg-execution.exp: paginate: continue&

E.g., it was giving the pagination prompt in a place the testcase wasn't 
expecting.  I tracked this down to the count of lines already printed 
not being reset properly with each command.  The call to 
reinitialize_more_filter is presently conditional on stdin being a tty, 
but the pagination logic itself clearly is not so conditionalized (if it 
were, I'd not be seeing any pagination prompting at all when testing in 
this configuration).  So the obvious fix seems to be to remove that 
extra condition.  Now the output from that snippet is:

set height 2
(gdb) PASS: gdb.base/paginate-bg-execution.exp: paginate: set height 2
continue&
Continuing.
(gdb) PASS: gdb.base/paginate-bg-execution.exp: paginate: continue&

and the pagination prompts do appear in the subsequent tests, same as in 
the Linux-host testing with a TTY.

OK to commit?

-Sandra


[-- Attachment #2: gdb-paginate-2.log --]
[-- Type: text/x-log, Size: 204 bytes --]

2015-12-18  Sandra Loosemore  <sandra@codesourcery.com>

	gdb/
	* event-top.c (command_handler): Don't require stdin to be a tty
	for call to reinitialize_more_filter.
	* top.c (command_loop): Likewise.


[-- Attachment #3: gdb-paginate-2.patch --]
[-- Type: text/x-patch, Size: 1037 bytes --]

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 3f98c05..e5a5ac6 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -469,11 +469,10 @@ async_disable_stdin (void)
 static void
 command_handler (char *command)
 {
-  int stdin_is_tty = ISATTY (stdin);
   struct cleanup *stat_chain;
 
   clear_quit_flag ();
-  if (instream == stdin && stdin_is_tty)
+  if (instream == stdin)
     reinitialize_more_filter ();
 
   /* If readline returned a NULL command, it means that the connection
diff --git a/gdb/top.c b/gdb/top.c
index d1e2271..a45f3cc 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -542,7 +542,6 @@ command_loop (void)
 {
   struct cleanup *old_chain;
   char *command;
-  int stdin_is_tty = ISATTY (stdin);
 
   while (instream && !feof (instream))
     {
@@ -550,7 +549,7 @@ command_loop (void)
 	(*window_hook) (instream, get_prompt ());
 
       clear_quit_flag ();
-      if (instream == stdin && stdin_is_tty)
+      if (instream == stdin)
 	reinitialize_more_filter ();
       old_chain = make_cleanup (null_cleanup, 0);
 

Re: [patch 2/3] reset pagination counts even when stdin is not a tty

[Pedro Alves]

On 12/18/2015 07:43 PM, Sandra Loosemore wrote:

> OK to commit?

OK.

Thanks,
Pedro Alves

[patch 3/3] add missing gdb_test arguments in paginate-bg-execution.exp

[Sandra Loosemore]

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

This patch fixes a think-o in gdb.base/paginate-bg-execution.exp -- 
there are two calls to gdb_test with only a single argument.  Looking at 
the definition of this proc in lib/gdb.exp, the second argument (the 
output pattern) is not supposed to be optional.  For whatever reason, I 
was only seeing failures on remote Windows host testing, but it must 
have been an accident that it appeared to be working elsewhere.

I copied the breakpoint output pattern used elsewhere in the testsuite, 
and confirmed this passes now.  OK to commit?

-Sandra


[-- Attachment #2: gdb-paginate-3.log --]
[-- Type: text/x-log, Size: 244 bytes --]

2015-12-18  Sandra Loosemore  <sandra@codesourcery.com>

	gdb/testsuite/
	* gdb.base/paginate-bg-execution.exp
	(test_bg_execution_pagination_return): Add missing pattern argument
	to gdb_test.
	(test_bg_execution_pagination_cancel): Likewise.

[-- Attachment #3: gdb-paginate-3.patch --]
[-- Type: text/x-patch, Size: 660 bytes --]

diff --git a/gdb/testsuite/gdb.base/paginate-bg-execution.exp b/gdb/testsuite/gdb.base/paginate-bg-execution.exp
index f2a4d73..12d5250 100644
--- a/gdb/testsuite/gdb.base/paginate-bg-execution.exp
+++ b/gdb/testsuite/gdb.base/paginate-bg-execution.exp
@@ -37,7 +37,7 @@ proc test_bg_execution_pagination_return {} {
 	    return 0
 	}
 
-	gdb_test "b after_sleep"
+	gdb_test "b after_sleep" "Breakpoint .* at .*"
 
 	gdb_test_no_output "set height 2"
 
@@ -80,7 +80,7 @@ proc test_bg_execution_pagination_cancel { how } {
 	    return 0
 	}
 
-	gdb_test "b after_sleep"
+	gdb_test "b after_sleep" "Breakpoint .* at .*"
 
 	gdb_test_no_output "set height 2"
 

Re: [patch 3/3] add missing gdb_test arguments in paginate-bg-execution.exp

[Pedro Alves]

On 12/18/2015 07:50 PM, Sandra Loosemore wrote:
> This patch fixes a think-o in gdb.base/paginate-bg-execution.exp -- 
> there are two calls to gdb_test with only a single argument.  Looking at 
> the definition of this proc in lib/gdb.exp, the second argument (the 
> output pattern) is not supposed to be optional.  For whatever reason, I 
> was only seeing failures on remote Windows host testing, but it must 
> have been an accident that it appeared to be working elsewhere.
> 
> I copied the breakpoint output pattern used elsewhere in the testsuite, 
> and confirmed this passes now.  OK to commit?

Can you show the gdb.log of the failed run?

Thanks,
Pedro Alves

Re: [patch 3/3] add missing gdb_test arguments in paginate-bg-execution.exp

[Sandra Loosemore]

On 12/18/2015 01:40 PM, Pedro Alves wrote:
> On 12/18/2015 07:50 PM, Sandra Loosemore wrote:
>> This patch fixes a think-o in gdb.base/paginate-bg-execution.exp --
>> there are two calls to gdb_test with only a single argument.  Looking at
>> the definition of this proc in lib/gdb.exp, the second argument (the
>> output pattern) is not supposed to be optional.  For whatever reason, I
>> was only seeing failures on remote Windows host testing, but it must
>> have been an accident that it appeared to be working elsewhere.
>>
>> I copied the breakpoint output pattern used elsewhere in the testsuite,
>> and confirmed this passes now.  OK to commit?
>
> Can you show the gdb.log of the failed run?

Hmmmm.  I lost the original log, and now I cannot reproduce the failure. 
  I must be losing my marbles.  :-(

Is it supposed to be correct to call gdb_test with only one argument? 
If yes, I'll withdraw this patch and submit another one to tweak the 
comments in lib/gdb.exp to explicitly say the pattern arg is optional.

-Sandra

Re: [patch 3/3] add missing gdb_test arguments in paginate-bg-execution.exp

[Pedro Alves]

On 12/18/2015 11:49 PM, Sandra Loosemore wrote:
> On 12/18/2015 01:40 PM, Pedro Alves wrote:
>> On 12/18/2015 07:50 PM, Sandra Loosemore wrote:
>>> This patch fixes a think-o in gdb.base/paginate-bg-execution.exp --
>>> there are two calls to gdb_test with only a single argument.  Looking at
>>> the definition of this proc in lib/gdb.exp, the second argument (the
>>> output pattern) is not supposed to be optional.  For whatever reason, I
>>> was only seeing failures on remote Windows host testing, but it must
>>> have been an accident that it appeared to be working elsewhere.
>>>
>>> I copied the breakpoint output pattern used elsewhere in the testsuite,
>>> and confirmed this passes now.  OK to commit?
>>
>> Can you show the gdb.log of the failed run?
> 
> Hmmmm.  I lost the original log, and now I cannot reproduce the failure. 
>   I must be losing my marbles.  :-(
> 
> Is it supposed to be correct to call gdb_test with only one argument? 

If it wasn't supposed to be correct, then it'd be good to add
an "error" call in gdb_test, to make it a hard error.

But I think it is supposed to work.  At least

 $ grep -rn "gdb_test "  | grep -v "\".*\".*\"" | grep -v "\\\\"

shows many (hundreds) of instances.  With no explicit pattern,
we end up just matching the prompt, ignoring whatever output
precedes it.

> If yes, I'll withdraw this patch and submit another one to tweak the 
> comments in lib/gdb.exp to explicitly say the pattern arg is optional.

I should we should do that indeed.

Thanks,
Pedro Alves

Re: [patch 3/3] add missing gdb_test arguments in paginate-bg-execution.exp

[Joel Brobecker]

> If it wasn't supposed to be correct, then it'd be good to add
> an "error" call in gdb_test, to make it a hard error.
> 
> But I think it is supposed to work.  At least
> 
>  $ grep -rn "gdb_test "  | grep -v "\".*\".*\"" | grep -v "\\\\"
> 
> shows many (hundreds) of instances.  With no explicit pattern,
> we end up just matching the prompt, ignoring whatever output
> precedes it.

Yes, I confirm it is supposed to work, or at least it's been
acknowledged to be that way for quite a while. The last time
we discussed it is when we created gdb_test_no_output.

-- 
Joel

Re: [patch 3/3] add missing gdb_test arguments in paginate-bg-execution.exp

[Sandra Loosemore]

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

On 12/21/2015 05:27 AM, Joel Brobecker wrote:
>> If it wasn't supposed to be correct, then it'd be good to add
>> an "error" call in gdb_test, to make it a hard error.
>>
>> But I think it is supposed to work.  At least
>>
>>   $ grep -rn "gdb_test "  | grep -v "\".*\".*\"" | grep -v "\\\\"
>>
>> shows many (hundreds) of instances.  With no explicit pattern,
>> we end up just matching the prompt, ignoring whatever output
>> precedes it.
>
> Yes, I confirm it is supposed to work, or at least it's been
> acknowledged to be that way for quite a while. The last time
> we discussed it is when we created gdb_test_no_output.

OK.  I think this tweak to the comments is obvious enough that I've gone 
ahead and checked it in.

-Sandra


[-- Attachment #2: gdb.log --]
[-- Type: text/x-log, Size: 167 bytes --]

2015-12-25  Sandra Loosemore  <sandra@codesourcery.com>

	gdb/testsuite/
	* lib/gdb.exp (gdb_test): Update comments to clarify that the
	PATTERN argument is optional.

[-- Attachment #3: gdb.patch --]
[-- Type: text/x-patch, Size: 845 bytes --]

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 765ac83..5488c29 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -959,7 +959,9 @@ proc gdb_test_multiple { command message user_code } {
 # COMMAND is the command to execute, send to GDB with send_gdb.  If
 #   this is the null string no command is sent.
 # PATTERN is the pattern to match for a PASS, and must NOT include
-#   the \r\n sequence immediately before the gdb prompt.
+#   the \r\n sequence immediately before the gdb prompt.  This argument
+#   may be omitted to just match the prompt, ignoring whatever output 
+#   precedes it.
 # MESSAGE is an optional message to be printed.  If this is
 #   omitted, then the pass/fail messages use the command string as the
 #   message.  (If this is the empty string, then sometimes we don't