[RFA 1/2] PR gdb/20604 - fix "quit" when an invalid expression is used

[RFA 1/2] PR gdb/20604 - fix "quit" when an invalid expression is used

[Tom Tromey]

This fixes PR gdb/20604.  The bug here is that passing an invalid
expression to "quit" -- e.g., "quit()" -- causes gdb to enter a
non-functioning state.

The immediate problem is that quit_force resets the terminal before
evaluating the expression.  However, it seemed to me that it doesn't
really make sense to pass the quit_force argument to kill_or_detach
(which passes it to to_detach), first because conflating the exit
status for "quit" and the signal to pass when detaching doesn't make
sense, and second because to_detach implementations generally only
accept a constant here, while "quit" accepts an expression.  So, I
removed that.

As an aside, I think the "detach SIGNO" functionality is not
documented.

Built and regtested on x86-64 Fedora 24.

2016-09-13  Tom Tromey  <tom@tromey.com>

	PR gdb/20604:
	* top.h (quit_force): Update.
	* top.c (quit_force): Changed type of first argument.  Don't
	evaluate expression.  Pass NULL to kill_or_detach.
	* cli/cli-cmds.c (quit_command): Evaluate "args".

2016-09-13  Tom Tromey  <tom@tromey.com>

	PR gdb/20604:
	* gdb.base/quit.exp: New file.
---
 gdb/ChangeLog                   |  8 ++++++++
 gdb/cli/cli-cmds.c              | 13 ++++++++++++-
 gdb/testsuite/ChangeLog         |  5 +++++
 gdb/testsuite/gdb.base/quit.exp | 34 ++++++++++++++++++++++++++++++++++
 gdb/top.c                       | 12 ++++--------
 gdb/top.h                       |  2 +-
 6 files changed, 64 insertions(+), 10 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/quit.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b585914..8f8d554 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2016-09-13  Tom Tromey  <tom@tromey.com>
+
+	PR gdb/20604:
+	* top.h (quit_force): Update.
+	* top.c (quit_force): Changed type of first argument.  Don't
+	evaluate expression.  Pass NULL to kill_or_detach.
+	* cli/cli-cmds.c (quit_command): Evaluate "args".
+
 2016-09-09  Andreas Arnez  <arnez@linux.vnet.ibm.com>
 
 	* elfread.c (auxv.h): New include.
diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index c60b1d3..6495477 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -343,12 +343,23 @@ show_configuration (char *args, int from_tty)
 void
 quit_command (char *args, int from_tty)
 {
+  int exit_code = 0;
+
+  /* An optional expression may be used to cause gdb to terminate with
+     the value of that expression.  */
+  if (args)
+    {
+      struct value *val = parse_and_eval (args);
+
+      exit_code = (int) value_as_long (val);
+    }
+
   if (!quit_confirm ())
     error (_("Not confirmed."));
 
   query_if_trace_running (from_tty);
 
-  quit_force (args, from_tty);
+  quit_force (args ? &exit_code : NULL, from_tty);
 }
 
 static void
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index c63ea72..e34ae67 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2016-09-13  Tom Tromey  <tom@tromey.com>
+
+	PR gdb/20604:
+	* gdb.base/quit.exp: New file.
+
 2016-09-11  Sergio Durigan Junior  <sergiodj@redhat.com>
 	    Jan Kratochvil  <jan.kratochvil@redhat.com>
 
diff --git a/gdb/testsuite/gdb.base/quit.exp b/gdb/testsuite/gdb.base/quit.exp
new file mode 100644
index 0000000..c0f5f57
--- /dev/null
+++ b/gdb/testsuite/gdb.base/quit.exp
@@ -0,0 +1,34 @@
+# Copyright (C) 2016 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/>.
+
+# Check the "quit" command.
+
+clean_restart
+
+# Test that a syntax error causes quit to abort.
+# Regression test for PR gdb/20604.
+gdb_test "quit()" "A syntax error in expression, near .*" \
+    "quit with syntax error"
+
+# Test that an expression can be used to set the error code.
+send_gdb "quit 22+1\n"
+set result [wait -i $gdb_spawn_id]
+verbose $result
+if {[lindex $result 2] == 0 && [lindex $result 3] == 23} {
+    pass "quit with expression"
+} else {
+    fail "quit with expression"
+}
+close $gdb_spawn_id
diff --git a/gdb/top.c b/gdb/top.c
index 320c296..3cfa113 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1625,7 +1625,7 @@ undo_terminal_modifications_before_exit (void)
 /* Quit without asking for confirmation.  */
 
 void
-quit_force (char *args, int from_tty)
+quit_force (int *exit_arg, int from_tty)
 {
   int exit_code = 0;
   struct qt_args qt;
@@ -1634,16 +1634,12 @@ quit_force (char *args, int from_tty)
 
   /* An optional expression may be used to cause gdb to terminate with the 
      value of that expression.  */
-  if (args)
-    {
-      struct value *val = parse_and_eval (args);
-
-      exit_code = (int) value_as_long (val);
-    }
+  if (exit_arg)
+    exit_code = *exit_arg;
   else if (return_child_result)
     exit_code = return_child_result_value;
 
-  qt.args = args;
+  qt.args = NULL;
   qt.from_tty = from_tty;
 
   /* We want to handle any quit errors and exit regardless.  */
diff --git a/gdb/top.h b/gdb/top.h
index ee664c1..acdb8e9 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -212,7 +212,7 @@ extern void read_command_file (FILE *);
 extern void init_history (void);
 extern void command_loop (void);
 extern int quit_confirm (void);
-extern void quit_force (char *, int);
+extern void quit_force (int *, int);
 extern void quit_command (char *, int);
 extern void quit_cover (void);
 extern void execute_command (char *, int);
-- 
2.7.4

[RFA 2/2] Make some test names invariant

[Tom Tromey]

While working on the previous patch, I noticed that the test names in
process-dies-while-detaching include the PID of some test process,
making it so that the test names change between runs.  This patch
fixes the problem.

2016-09-14  Tom Tromey  <tom@tromey.com>

	* gdb.threads/process-dies-while-detaching.exp
	(test_detach_killed_outside): Pass test name to
	get_integer_valueof.
	* lib/gdb.exp (get_integer_valueof): Add "test" argument.
---
 gdb/testsuite/ChangeLog                                    | 7 +++++++
 gdb/testsuite/gdb.threads/process-dies-while-detaching.exp | 2 +-
 gdb/testsuite/lib/gdb.exp                                  | 6 ++++--
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index e34ae67..b397364 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2016-09-14  Tom Tromey  <tom@tromey.com>
+
+	* gdb.threads/process-dies-while-detaching.exp
+	(test_detach_killed_outside): Pass test name to
+	get_integer_valueof.
+	* lib/gdb.exp (get_integer_valueof): Add "test" argument.
+
 2016-09-13  Tom Tromey  <tom@tromey.com>
 
 	PR gdb/20604:
diff --git a/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp b/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp
index 52dc8dd..734effc 100644
--- a/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp
+++ b/gdb/testsuite/gdb.threads/process-dies-while-detaching.exp
@@ -268,7 +268,7 @@ proc test_detach_killed_outside {multi_process cmd} {
 	# Run to _exit in the child.
 	continue_to_exit_bp
 
-	set childpid [get_integer_valueof "mypid" -1]
+	set childpid [get_integer_valueof "mypid" -1 "get value of mypid"]
 	if { $childpid == -1 } {
 	    untested "failed to extract child pid"
 	    return -1
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e538812..9abe3cd 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5524,10 +5524,12 @@ proc get_valueof { fmt exp default } {
     return ${val}
 }
 
-proc get_integer_valueof { exp default } {
+proc get_integer_valueof { exp default {test ""} } {
     global gdb_prompt
 
-    set test "get integer valueof \"${exp}\""
+    if {$test == ""} {
+	set test "get integer valueof \"${exp}\""
+    }
     set val ${default}
     gdb_test_multiple "print /d ${exp}" "$test" {
 	-re "\\$\[0-9\]* = (\[-\]*\[0-9\]*).*$gdb_prompt $" {
-- 
2.7.4

Re: [RFA 1/2] PR gdb/20604 - fix "quit" when an invalid expression is used

[Pedro Alves]

Hi Tom, the gdb code looks fine.  Some comments on the test.

On 09/14/2016 11:59 PM, Tom Tromey wrote:

> +# Check the "quit" command.
> +
> +clean_restart
> +
> +# Test that a syntax error causes quit to abort.
> +# Regression test for PR gdb/20604.
> +gdb_test "quit()" "A syntax error in expression, near .*" \
> +    "quit with syntax error"
> +
> +# Test that an expression can be used to set the error code.
> +send_gdb "quit 22+1\n"
> +set result [wait -i $gdb_spawn_id]
> +verbose $result
> +if {[lindex $result 2] == 0 && [lindex $result 3] == 23} {

Not sure getting back the exit status works universally with 
remote-host testing.  ISTR we're not guaranteed to get back an
exit status on all connection types (may get back an ssh/rsh status
instead).  Also, the board may need to tear down more
than just gdb.

Could you try the new test with --host_board=local-remote-host.exp?  
That should cover at least remote ssh testing, which is what
most people use nowadays, anyway.  I'd be happy with that.

> +    pass "quit with expression"
> +} else {
> +    fail "quit with expression"
> +}
> +close $gdb_spawn_id

I suspect that this may be breaking non-parallel-mode testing
(e.g., runtest directly instead of make check), since it leaves 
$gdb_spawn_id set.

Maybe just calling clear_gdb_spawn_id at the end is all that's
necessary?

(I mildly wonder whether making this a gdb.gdb self test would be a good
idea.  I.e., the inferior gdb's exit would be caught by the
superior gdb as a normal exit.  OTOH, that'd make the test
heavier.)

Thanks,
Pedro Alves

Re: [RFA 2/2] Make some test names invariant

[Pedro Alves]

On 09/14/2016 11:59 PM, Tom Tromey wrote:
> While working on the previous patch, I noticed that the test names in
> process-dies-while-detaching include the PID of some test process,
> making it so that the test names change between runs.  This patch
> fixes the problem.

LGTM.

However, FYI, with the current "definition" of test names, it 
doesn't actually change, because tools should be ignoring the
trailing " (...)" bit.  See:

 https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

We've always had to consider this, given "$test (timeout)", etc.

Note that the wiki doesn't say it, and I don't think we've ever
discussed it, but but the space before the open parens should matter
when deciding whether to ignore the trailing bit.  If there's no space,
it shouldn't be ignored.  This in order to make it possible to write
tests that call functions without coming up with odd contortions,
like, e.g., "print foo(1)".

Thanks,
Pedro Alves

Re: [RFA 2/2] Make some test names invariant

[Tom Tromey]

Pedro> However, FYI, with the current "definition" of test names, it 
Pedro> doesn't actually change, because tools should be ignoring the
Pedro> trailing " (...)" bit.

Aha, I didn't realize this, and the old test-comparison script I have
been using doesn't do this.

I think I'll just drop this patch and fix my script.

Tom

Re: [RFA 1/2] PR gdb/20604 - fix "quit" when an invalid expression is used

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Hi Tom, the gdb code looks fine.  Some comments on the test.

Pedro> Could you try the new test with --host_board=local-remote-host.exp?  
Pedro> That should cover at least remote ssh testing, which is what
Pedro> most people use nowadays, anyway.  I'd be happy with that.

I tried this today and it worked for me.

>> +close $gdb_spawn_id

Pedro> Maybe just calling clear_gdb_spawn_id at the end is all that's
Pedro> necessary?

Seems like a good idea, I made this change locally.

thanks,
Tom

Re: [RFA 1/2] PR gdb/20604 - fix "quit" when an invalid expression is used

[Pedro Alves]

On 09/20/2016 11:45 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Hi Tom, the gdb code looks fine.  Some comments on the test.
> 
> Pedro> Could you try the new test with --host_board=local-remote-host.exp?  
> Pedro> That should cover at least remote ssh testing, which is what
> Pedro> most people use nowadays, anyway.  I'd be happy with that.
> 
> I tried this today and it worked for me.
> 
>>> +close $gdb_spawn_id
> 
> Pedro> Maybe just calling clear_gdb_spawn_id at the end is all that's
> Pedro> necessary?
> 
> Seems like a good idea, I made this change locally.
> 

OK, thanks.  Fell free to push.  We'll do something about it if it
causes problems on real remote hosts.

Thanks,
Pedro Alves

Re: [RFA 1/2] PR gdb/20604 - fix "quit" when an invalid expression is used

[Ulrich Weigand]

Pedro Alves wrote:
> On 09/20/2016 11:45 PM, Tom Tromey wrote:
> >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> > 
> > Pedro> Hi Tom, the gdb code looks fine.  Some comments on the test.
> > 
> > Pedro> Could you try the new test with --host_board=local-remote-host.exp?  
> > Pedro> That should cover at least remote ssh testing, which is what
> > Pedro> most people use nowadays, anyway.  I'd be happy with that.
> > 
> > I tried this today and it worked for me.
> > 
> >>> +close $gdb_spawn_id
> > 
> > Pedro> Maybe just calling clear_gdb_spawn_id at the end is all that's
> > Pedro> necessary?
> > 
> > Seems like a good idea, I made this change locally.
> > 
> 
> OK, thanks.  Fell free to push.  We'll do something about it if it
> causes problems on real remote hosts.

I just noticed that this test case completely breaks my daily testing.
When running the quit.exp test, GDB will hang in a way that it isn't
even killed by the timeout logic, and will keep blocking further
execution forever.

Attaching to GDB shows it blocked in an ioctl with this backtrace
(which for some reason doesn't even include the quit path ...)

#0  0x0feea474 in tcsetattr () from /lib/libc.so.6
#1  0x102d7004 in _set_tty_settings (tty=0, tiop=0x10641adc) at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:476
#2  0x102d70e0 in set_tty_settings (tty=<value optimized out>, tiop=<value optimized out>)
    at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:490
#3  0x102d7530 in rl_deprep_terminal () at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:688
#4  0x102ea2d4 in rl_callback_read_char () at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/callback.c:215
#5  0x1017defc in gdb_rl_callback_read_char_wrapper (client_data=<value optimized out>)
    at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/gdb/event-top.c:173
#6  0x1017ecc4 in stdin_event_handler (error=<value optimized out>, client_data=0x10650018)
    at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/gdb/event-top.c:505
#7  0x1017d630 in handle_file_event (file_ptr=0x10749238, ready_mask=<value optimized out>)
    at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/gdb/event-loop.c:733
#8  0x1017d84c in gdb_wait_for_event (block=0) at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/gdb/event-loop.c:884
#9  0x1017dc18 in gdb_do_one_event () at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/gdb/event-loop.c:322
#10 0x1017dd20 in start_event_loop () at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/gdb/event-loop.c:371
#11 0x10175968 in captured_command_loop (data=<value optimized out>) at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/gdb/main.c:325
#12 0x101714a8 in catch_errors(catch_errors_ftype *, void *, char *, ._43) (func=0x10175930 <captured_command_loop(void*)>, func_args=0x0, errstring=0x104ccb4c "", 
    mask=RETURN_MASK_ALL) at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/gdb/exceptions.c:236
#13 0x10176f54 in captured_main (args=<value optimized out>) at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/gdb/main.c:1145
#14 gdb_main (args=<value optimized out>) at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/gdb/main.c:1155
#15 0x10006e28 in main (argc=<value optimized out>, argv=<value optimized out>) at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/gdb/gdb.c:32

Note that when running GDB directly, quit works fine.  The problem
occurs only when running GDB under the DejaGNU framework.

Does this ring any bells?  Any thoughts what could cause this?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [RFA 1/2] PR gdb/20604 - fix "quit" when an invalid expression is used

[Pedro Alves]

On 10/26/2016 08:44 PM, Ulrich Weigand wrote:

> I just noticed that this test case completely breaks my daily testing.
> When running the quit.exp test, GDB will hang in a way that it isn't
> even killed by the timeout logic, and will keep blocking further
> execution forever.
> 
> Attaching to GDB shows it blocked in an ioctl with this backtrace
> (which for some reason doesn't even include the quit path ...)
> 
> #0  0x0feea474 in tcsetattr () from /lib/libc.so.6
> #1  0x102d7004 in _set_tty_settings (tty=0, tiop=0x10641adc) at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:476
> #2  0x102d70e0 in set_tty_settings (tty=<value optimized out>, tiop=<value optimized out>)
>     at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:490
> #3  0x102d7530 in rl_deprep_terminal () at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:688
> #4  0x102ea2d4 in rl_callback_read_char () at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/callback.c:215
> #5  0x1017defc in gdb_rl_callback_read_char_wrapper (client_data=<value optimized out>)

> Note that when running GDB directly, quit works fine.  The problem
> occurs only when running GDB under the DejaGNU framework.

In such cases, I suggest inserting a gdb_interact call in
the testcase and debugging that way.

> Does this ring any bells?  Any thoughts what could cause this?

The [wait -i $gdb_spawn_id] in the test does look dangerous
in the sense that it won't be subject to timeout logic.
So if the previous test fails, that'll likely hang forever.

Other than that, no ideas.  Can you tell from the gdb.log how
far the test went?

Thanks,
Pedro Alves

Re: [RFA 1/2] PR gdb/20604 - fix "quit" when an invalid expression is used

[Ulrich Weigand]

Pedro Alves wrote:
> On 10/26/2016 08:44 PM, Ulrich Weigand wrote:
> 
> > I just noticed that this test case completely breaks my daily testing.
> > When running the quit.exp test, GDB will hang in a way that it isn't
> > even killed by the timeout logic, and will keep blocking further
> > execution forever.
> > 
> > Attaching to GDB shows it blocked in an ioctl with this backtrace
> > (which for some reason doesn't even include the quit path ...)
> > 
> > #0  0x0feea474 in tcsetattr () from /lib/libc.so.6
> > #1  0x102d7004 in _set_tty_settings (tty=0, tiop=0x10641adc) at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:476
> > #2  0x102d70e0 in set_tty_settings (tty=<value optimized out>, tiop=<value optimized out>)
> >     at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:490
> > #3  0x102d7530 in rl_deprep_terminal () at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:688
> > #4  0x102ea2d4 in rl_callback_read_char () at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/callback.c:215
> > #5  0x1017defc in gdb_rl_callback_read_char_wrapper (client_data=<value optimized out>)
> 
> > Note that when running GDB directly, quit works fine.  The problem
> > occurs only when running GDB under the DejaGNU framework.
> 
> In such cases, I suggest inserting a gdb_interact call in
> the testcase and debugging that way.

Thanks for the tip, this allowed me to debug this further.

> > Does this ring any bells?  Any thoughts what could cause this?
> 
> The [wait -i $gdb_spawn_id] in the test does look dangerous
> in the sense that it won't be subject to timeout logic.
> So if the previous test fails, that'll likely hang forever.
> 
> Other than that, no ideas.  Can you tell from the gdb.log how
> far the test went?

Apparently the problem has nothing to do with the "quit" command
in itself; GDB never even gets around to attempt to execute the
command.  Any test case along the lines of:

  send_gdb "<...>\n"
  set result [wait -i $gdb_spawn_id]

will result in the same hang on my RHEL 5 system.

What happens is that after the send_gdb command has sent the newline
to the GDB process, readline triggers its end-of-line machinery.
This will call "rl_deprep_terminal", which attempts to reset the
TTY to its default settings.  This uses the tcsetattr routine
with the TCSADRAIN option, which gets translated by glibc into
a TCSETSW ioctl.

Now this ioctl causes the kernel (at least the 2.6.18 kernel in RHEL 5)
to attempt to flush the *write* side of the TTY and wait until that
flush has succeeded.  However, apparently nobody is *reading* on that
side of the TTY (since expect has only performed the send_gdb which
writes on the other side of the TTY, and is now in a wait which does
not read on any side of the TTY), and thus the wait never returns.

Since rl_deprep_terminal also blocks SIGINT while issuing the tcsetattr,
this cannot even be interrupted easily.

Now I don't fully understand why more recent kernels don't appear to
block indefinitely here; maybe something in the TTY buffering changed?
In any case, I'm also not sure if the test is really doing the right
thing here.  Can this not be done using a gdb_expect or any of the
other usual constructs that will actually read GDB's TTY output?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [RFA 1/2] PR gdb/20604 - fix "quit" when an invalid expression is used

[Pedro Alves]

Hi Ulrich, Tom,

On 11/07/2016 03:57 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:
>> On 10/26/2016 08:44 PM, Ulrich Weigand wrote:
>>
>>> I just noticed that this test case completely breaks my daily testing.
>>> When running the quit.exp test, GDB will hang in a way that it isn't
>>> even killed by the timeout logic, and will keep blocking further
>>> execution forever.
>>>
>>> Attaching to GDB shows it blocked in an ioctl with this backtrace
>>> (which for some reason doesn't even include the quit path ...)
>>>
>>> #0  0x0feea474 in tcsetattr () from /lib/libc.so.6
>>> #1  0x102d7004 in _set_tty_settings (tty=0, tiop=0x10641adc) at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:476
>>> #2  0x102d70e0 in set_tty_settings (tty=<value optimized out>, tiop=<value optimized out>)
>>>     at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:490
>>> #3  0x102d7530 in rl_deprep_terminal () at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/rltty.c:688
>>> #4  0x102ea2d4 in rl_callback_read_char () at /home/uweigand/dailybuild/spu-tc-2016-10-25/binutils-gdb-head/binutils-gdb/readline/callback.c:215
>>> #5  0x1017defc in gdb_rl_callback_read_char_wrapper (client_data=<value optimized out>)
>>
>>> Note that when running GDB directly, quit works fine.  The problem
>>> occurs only when running GDB under the DejaGNU framework.
>>
>> In such cases, I suggest inserting a gdb_interact call in
>> the testcase and debugging that way.
> 
> Thanks for the tip, this allowed me to debug this further.
> 
>>> Does this ring any bells?  Any thoughts what could cause this?
>>
>> The [wait -i $gdb_spawn_id] in the test does look dangerous
>> in the sense that it won't be subject to timeout logic.
>> So if the previous test fails, that'll likely hang forever.
>>
>> Other than that, no ideas.  Can you tell from the gdb.log how
>> far the test went?
> 
> Apparently the problem has nothing to do with the "quit" command
> in itself; GDB never even gets around to attempt to execute the
> command.  Any test case along the lines of:
> 
>   send_gdb "<...>\n"
>   set result [wait -i $gdb_spawn_id]
> 
> will result in the same hang on my RHEL 5 system.
> 
> What happens is that after the send_gdb command has sent the newline
> to the GDB process, readline triggers its end-of-line machinery.
> This will call "rl_deprep_terminal", which attempts to reset the
> TTY to its default settings.  This uses the tcsetattr routine
> with the TCSADRAIN option, which gets translated by glibc into
> a TCSETSW ioctl.
> 
> Now this ioctl causes the kernel (at least the 2.6.18 kernel in RHEL 5)
> to attempt to flush the *write* side of the TTY and wait until that
> flush has succeeded.  However, apparently nobody is *reading* on that
> side of the TTY (since expect has only performed the send_gdb which
> writes on the other side of the TTY, and is now in a wait which does
> not read on any side of the TTY), and thus the wait never returns.
> 
> Since rl_deprep_terminal also blocks SIGINT while issuing the tcsetattr,
> this cannot even be interrupted easily.
> 
> Now I don't fully understand why more recent kernels don't appear to
> block indefinitely here; maybe something in the TTY buffering changed?
> In any case, I'm also not sure if the test is really doing the right
> thing here.  Can this not be done using a gdb_expect or any of the
> other usual constructs that will actually read GDB's TTY output?
> 

[Meanwhile, over a year passed somehow...]

Thanks for the analysis.  I'm not sure what to do about the
specifics of the issue you describe above, but since I was
touching "quit" related tests this week, I remembered this
issue.  I've sent a fix now that should at least prevent
hanging the testsuite forever:

  https://sourceware.org/ml/gdb-patches/2017-10/msg00534.html

Let me know what you think.

Thanks,
Pedro Alves

Re: [RFA 1/2] PR gdb/20604 - fix "quit" when an invalid expression is used

[Ulrich Weigand]

Pedro Alves wrote:

> Thanks for the analysis.  I'm not sure what to do about the
> specifics of the issue you describe above, but since I was
> touching "quit" related tests this week, I remembered this
> issue.  I've sent a fix now that should at least prevent
> hanging the testsuite forever:
> 
>   https://sourceware.org/ml/gdb-patches/2017-10/msg00534.html
> 
> Let me know what you think.

That looks good to me.  I think it'll will fix the problem
I've been having on my Cell test system ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com