Re: [PATCH] Add mi-threads-interrupt.exp test (PR 20039)

[Pedro Alves <palves at redhat dot com>]

On 05/03/2016 09:00 PM, Simon Marchi wrote:
> Add a new test for PR 20039.  The test spawns new threads, then tries to
> interrupt, continue, and interrupt again.  This use case was fixed by
> commit 5fe966540d6b748f825774868463003700f0c878 in master, but gdb 7.11
> is affected (so if you try it on the gdb-7.11-branch right now, the test
> will fail).

Thanks for the test!

I debugged this a little, and I see that the bug is that -exec-continue ends
up with thread 3 selected, which generates a =thread-selected event.

-exec-continue
^running
*running,thread-id="all"
(gdb) 
=thread-selected,id="3"


And then the code that emits that =thread-selected calls target_terminal_ours,
and doesn't restore the terminal with target_terminal_inferior:

	  if (report_change)
	    {
	      struct thread_info *ti = inferior_thread ();

	      target_terminal_ours ();
	      fprintf_unfiltered (mi->event_channel,
				  "thread-selected,id=\"%d\"",
				  ti->global_num);
	      gdb_flush (mi->event_channel);
	    }

We'll end up calling target_terminal_inferior when we next
re-resume the inferior after some internal event, but if no
such event ever happens, the target will remain running
free with target_terminal_ours in effect...

So two bugs: calling target_terminal_ours instead of
target_terminal_ours_for_output, and not restoring the target terminal,
both addressed in 5fe966540d6b (Use target_terminal_ours_for_output in MI):

--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2171,12 +2171,17 @@ mi_execute_command (const char *cmd, int from_tty)
 	  if (report_change)
 	    {
 	      struct thread_info *ti = inferior_thread ();
+	      struct cleanup *old_chain;
+
+	      old_chain = make_cleanup_restore_target_terminal ();
+	      target_terminal_ours_for_output ();
 
-	      target_terminal_ours ();
 	      fprintf_unfiltered (mi->event_channel,
 				  "thread-selected,id=\"%d\"",
 				  ti->global_num);
 	      gdb_flush (mi->event_channel);
+
+	      do_cleanups (old_chain);
 	    }


> +  # Load the binary in gdb and run it.
> +  mi_gdb_load $binfile
> +  mi_runto "all_threads_created"

I think we could add a comment saying:

# Note this test relies on mi_runto deleting the breakpoint.
# A step-over here would mask the bug.

> +
> +  # Consistency check.
> +  mi_check_thread_states {"stopped" "stopped" "stopped"} "check thread states"
> +
> +  # Continue.
> +  mi_gdb_test "565-exec-continue" "565\\^running\r\n\\*running,thread-id=\"all\"" "continue #1"
> +
> +  # Send ctrl-C
> +  send_gdb "\003"

I guess you didn't add a match for =thread-selected because that
detail may change in future.  I agree with that.
However, I think it'll good to wait a second or some such before
sending the ctrl-C, to make sure all such events were indeed
output.  Otherwise, if the machine is fast enough, we may
end up sending the ctrl-C before the =thread-selected event
is reached.

> +  mi_expect_interrupt "interrupt #1"
> +
> +  # Continue.
> +  mi_gdb_test "565-exec-continue" "565\\^running\r\n\\*running,thread-id=\"all\"" "continue #2"
> +
> +  # Send ctrl-C again.
> +  send_gdb "\003"

Ditto.

> +  mi_expect_interrupt "interrupt #2"
> +}

AFAICS, the test relies on "set mi-async off".  Could you make sure that
if you run it against a board file that forces that on, the test either
passes (probably with -exec-interrupt in async mode) or is skipped?
See mi_detect_async and the async global.

Thanks,
Pedro Alves