[RFA,1/2] PR gdb/20604 - fix "quit" when an invalid expression is used
Commit Message
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
Comments
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
@@ -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.
@@ -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
@@ -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>
new file mode 100644
@@ -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
@@ -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. */
@@ -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);