Message ID | 5983d4d2-016a-8020-c109-cb7ea2cfd179@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 55022 invoked by alias); 3 May 2016 21:57:37 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 55004 invoked by uid 89); 3 May 2016 21:57:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=states X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 03 May 2016 21:57:35 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7B24BC05E174; Tue, 3 May 2016 21:57:34 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u43LvXiW011720; Tue, 3 May 2016 17:57:33 -0400 Subject: Re: [PATCH] Add mi-threads-interrupt.exp test (PR 20039) To: Simon Marchi <simon.marchi@ericsson.com>, gdb-patches@sourceware.org References: <1462305612-16493-1-git-send-email-simon.marchi@ericsson.com> From: Pedro Alves <palves@redhat.com> Message-ID: <5983d4d2-016a-8020-c109-cb7ea2cfd179@redhat.com> Date: Tue, 3 May 2016 22:57:32 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <1462305612-16493-1-git-send-email-simon.marchi@ericsson.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit |
Commit Message
Pedro Alves
May 3, 2016, 9:57 p.m. UTC
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
Comments
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
--- 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); }