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

  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
  

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
  

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

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