Add mi-threads-interrupt.exp test (PR 20039)

  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):

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

On 05/03/2016 10:57 PM, Pedro Alves wrote:
> 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.

Alternatively, force set mi-async off in the test.

Thanks,
Pedro Alves
  

On 05/03/2016 10:57 PM, Pedro Alves wrote:
> 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.

Oh, BTW, I think this explains why reversing the thread list order exposed
this bug.  Before that patch, we'd issue an -exec-continue with
thread 1 selected, and gdb would resume thread 3, thread 2, and thread 1,
and thus end up with thread 1 selected again, and thus no =thread-selected
event would be output.  So I guess that if you select thread 2 before
the -exec-continue, the test exposes the bug even before the thread list
reordering patch.  Might be good to do that in the test in case that
detail ever changes again.

Thanks,
Pedro Alves
  

On 05/03/2016 10:57 PM, Pedro Alves wrote:
> 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.

Woke up this morning realizing that I hadn't done this for so long
myself that I had forgotten how I used to do it.  We don't really
need a board file -- as described in the TestingGDB wiki page [1], 
we can use GDBFLAGS from the command line for this:

 $ make check RUNTESTFLAGS="GDBFLAGS='-ex set\ mi-async\ on'" TESTS="gdb.mi/mi-threads-interrupt.exp"
 ...
 FAIL: gdb.mi/mi-threads-interrupt.exp: interrupt #1 (unknown output after running)
 FAIL: gdb.mi/mi-threads-interrupt.exp: continue #2

[1] https://sourceware.org/gdb/wiki/TestingGDB

Thanks,
Pedro Alves
  

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
  

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
  

On 16-05-04 05:20 AM, Pedro Alves wrote:
> On 05/03/2016 10:57 PM, Pedro Alves wrote:
>> 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.
> 
> Woke up this morning realizing that I hadn't done this for so long
> myself that I had forgotten how I used to do it.  We don't really
> need a board file -- as described in the TestingGDB wiki page [1], 
> we can use GDBFLAGS from the command line for this:
> 
>  $ make check RUNTESTFLAGS="GDBFLAGS='-ex set\ mi-async\ on'" TESTS="gdb.mi/mi-threads-interrupt.exp"
>  ...
>  FAIL: gdb.mi/mi-threads-interrupt.exp: interrupt #1 (unknown output after running)
>  FAIL: gdb.mi/mi-threads-interrupt.exp: continue #2
> 
> [1] https://sourceware.org/gdb/wiki/TestingGDB
> 
> Thanks,
> Pedro Alves
> 

Why would it make sense to set async on in a board file?  My understanding is
that the board file defines the debug target side of things, but the fact that MI
is sync or async (or that we use MI at all) is not decided by the debug target.

I would think that it's up to the tests to set async however they want it,
depending on what they test.  In this particular case, wouldn't it be better
to run both the test in sync and async modes, as we do often for non-stop?

So, for example:

proc test_continue_interrupt { async } {
  with_test_prefix "async=$async" {
    ...
  }
}

foreach async {on off} {
  test_continue_interrupt $async
}

It hacked the test quickly, and it shouldn't be too hard to do.

Thanks,

Simon
  

On 05/04/2016 07:04 PM, Simon Marchi wrote:

> Why would it make sense to set async on in a board file?  My understanding is
> that the board file defines the debug target side of things, but the fact that MI
> is sync or async (or that we use MI at all) is not decided by the debug target.

Either setting GDBFLAGS on the board file or on the command line, it's
really the same thing: an expedient way to run the whole testsuite in a 
certain mode.  I've found it quite useful over the years, while
working on e.g., "set breakpoints always-inserted on", or when
doing all the work toward making "set target-async on" the default.
In either case, this was not a setting that users really should really
need to be toggling themselves.

> I would think that it's up to the tests to set async however they want it,
> depending on what they test.  In this particular case, wouldn't it be better
> to run both the test in sync and async modes, as we do often for non-stop?

A bit of history here.  We nowadays have two distinct commands:

 #1 - "set mi-async on"

 #2 - "maint set target-async on"

The mi-async command makes all MI execution commands be background
commands.  That is, e.g., "-exec-continue" turns into "continue&" rather
than "continue".  (Plus some other minor differences.)
It doesn't make much sense for MI execution commands to ever be sync,
but, frontends that are not prepared for async would break, thus the need
for the switch.

The "maint set target-async on" command changes the way the
target backend works, but other than _enabling_ accessing to 
background commands, it should have no user-visible behavior.
The only reason for the command to exist, is convenience, for
emulating targets that don't do async yet, on GNU/Linux.

While a few releases back we had a single "set target-async" that
conflicted both #1 and #2.  And at the time, because that command
actually changed how the target_ops behaves, it was sort of
a maintenance command in disguise, it didn't make sense to 
change _every_ test in the testsuite to try
both "set target-async on/off".  So years ago (circa 2007/2008, I believe),
what we did was make it possible to force "set target-async on", and run
the testsuite that way.  And then since MI changed output when tested
that way, we made the testsuite cope.

And then nobody bothered yet to make all _MI_ tests run with
"set mi-async on/off", while leaving all CLI tests alone at the
same time.

> 
> So, for example:
> 
> proc test_continue_interrupt { async } {
>   with_test_prefix "async=$async" {
>     ...
>   }
> }
> 
> foreach async {on off} {
>   test_continue_interrupt $async
> }
> 
> It hacked the test quickly, and it shouldn't be too hard to do.
> 

Yeah.  But in reality, we should really be doing that to
all MI tests.

For the console series, I'm adding another test axis to consider,
which is to run all MI tests with MI forced to a separate tty.
This has been _very_ convenient to catch problems.  For now,
I'm adding a way to force that from the command line, but I could
see us run all MI tests always in both normal MI, and separate
MI modes.  It's easy to explode the number of testing axes though...

This test in particular though, maybe it just doesn't make
sense to run in async mode at all, as sending a ctrl-c to
gdb in that case _should_ result in a Quit.

Thanks,
Pedro Alves