From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 42172 invoked by alias); 4 May 2016 15:07:56 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 42160 invoked by uid 89); 4 May 2016 15:07:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.0 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Though, iow, 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; Wed, 04 May 2016 15:07:53 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (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 20F7A8E37D; Wed, 4 May 2016 15:07:52 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u44F7oiB013571; Wed, 4 May 2016 11:07:51 -0400 Subject: Re: [PATCH] Add mi-threads-interrupt.exp test (PR 20039) To: Simon Marchi , gdb-patches@sourceware.org References: <1462305612-16493-1-git-send-email-simon.marchi@ericsson.com> <5983d4d2-016a-8020-c109-cb7ea2cfd179@redhat.com> <572A0853.3020408@ericsson.com> From: Pedro Alves Message-ID: Date: Wed, 04 May 2016 15:07:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <572A0853.3020408@ericsson.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-05/txt/msg00059.txt.bz2 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