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

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 16-05-03 05:57 PM, Pedro Alves wrote:
> 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):

I had the feeling it was something like that, but couldn't put the finger on it.
Thanks for the explanation.

>> +  # 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.

Because, as you said, handling of internal events calls target_terminal_inferior?
Where is that?

Actually, because of that, I would only need to test a single pair of continue/interrupt, not
two like I do now.  I guess in my manual testing I didn't remove the breakpoint, and so I needed
one more continue/interrupt to trigger the bug, as it was masked by the step over.  Indeed, without
the fix, the test hangs at the first interrupt.  Do you see a reason to keep the two continue/interrupt
pairs, instead of just one?

>> +  # 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.

Hmm no, I left it out because it appears after the gdb prompt, so I couldn't include it in the mi_gdb_test
(since the mi_gdb_test ends with the prompt).

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

I didn't think about matching/waiting for the =thread-selected event, but since it's that event that leaves
the terminal in the wrong state, it's true that we want to make sure it's output before ctrl-Cing.  Ideally
I'd like to avoid sleeping if it's not necessary, and instead match things more explicitly.  The test runs
in about 0.5 second without it, so about 1.5 seconds.  By itself it's not significant, but if we use sleeps
liberally in the tests in general, it will make the testsuite unnecessary longer to run (it's already long
enough as it is!).

Would you be ok with adding a gdb_expect call, such as

  # The bug was caused by the =thread-select emitting code not giving back the
  # terminal to the inferior, so make sure we see the event before doing the ctrl-C.
  gdb_expect "=thread-selected,id=\"3\""

(possibly with ${decimal} instead of 3)

The downside of that, as you said, is that it's tied to not so significant implementation detail.  If it
changes in the future, the test will need to be updated.  Given that you're the one who happens to do
this kind of changes more frequently, I'd understand if you preferred to avoid that.

While we're at it, there is something I don't understand about test output matching.  How come
=thread-selected (and a lot of other MI output, actually) is never matched and consumed, and yet the test
passes.  I thought that all the output needed to be matched somewhere for it to get out of the expect buffer?
What exactly allows some of it to be skipped?

>> +  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.

Ok, but as I mentioned earlier I might remove it.

>> +  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.

Ok, I'll look at that (and your other replies).

Thanks!

Simon