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

[Pedro Alves <palves@redhat.com>]

On 05/04/2016 03:33 PM, Simon Marchi wrote:
> 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?

When the step-over finishes, infrun.c decides to keep_going and that ends up
in a call to target_terminal_inferior.  Something like
keep_going -> resume -> do_target_resume -> target_terminal_inferior.
Putting a break on target_terminal_inferior will show it clearly.

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

Indeed, I don't see a reason.  I was actually confused about why you needed two
ctrl-c's in the first place.  :-)

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

I agree, in general.  Though several ctrl-c tests necessarily do
this already.  E.g.,:

        # Wait a bit for GDB to give the terminal to the inferior,
        # otherwise ctrl-c too soon can result in a "Quit".
        sleep 1
        send_gdb "\003"

I think for ctrl-c tests, we just need to live with it.

[ Half kidding, we just need to run tests with a high enough -jN
  and then only the longest test matters.  :-) ]

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

Yeah, I'd prefer to avoid it, because this is likely to differ with
e.g., all-stop-on-top-of-non-stop, and maybe other modes in the future.

I could easily see us getting rid of this event entirely, even,
and then we're left back into thinking what to do with this
test.

So I think we just need to make sure the use case is covered,
and be reasonably sure all potential MI events have been
emitted, and the program is running free.

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

No, expect's -re matches are unanchored.  From man expect:

~~~
  patterns do not have to match the entire string, but can begin 
  and end the match anywhere in the string (as long as everything else matches).
~~~

So expecting with a "foo" regexp is implicitly the same ".*foo".
IOW, the next test consumes the =thread-select.

Thanks,
Pedro Alves