This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Add mi-threads-interrupt.exp test (PR 20039)
- From: Pedro Alves <palves at redhat dot com>
- To: Simon Marchi <simon dot marchi at ericsson dot com>, gdb-patches at sourceware dot org
- Date: Wed, 4 May 2016 16:07:50 +0100
- Subject: Re: [PATCH] Add mi-threads-interrupt.exp test (PR 20039)
- Authentication-results: sourceware.org; auth=none
- References: <1462305612-16493-1-git-send-email-simon dot marchi at ericsson dot com> <5983d4d2-016a-8020-c109-cb7ea2cfd179 at redhat dot com> <572A0853 dot 3020408 at ericsson dot 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