Message ID | 87d2b4c07h.fsf@anubis.Home |
---|---|
State | New, archived |
Headers |
Received: (qmail 19683 invoked by alias); 9 Sep 2014 14:55:56 -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 19672 invoked by uid 89); 9 Sep 2014 14:55:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.5 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.2 X-HELO: layla.krisman.be Received: from layla.krisman.be (HELO layla.krisman.be) (176.31.208.35) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Tue, 09 Sep 2014 14:55:53 +0000 Received: from [127.0.0.1] (localhost [127.0.0.1]) with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (envelope-from <gabriel@krisman.be>) id 1XRMmR-0005ij-TJ; Tue, 09 Sep 2014 16:52:28 +0200 From: Gabriel Krisman Bertazi <gabriel@krisman.be> To: Joel Brobecker <brobecker@adacore.com> Cc: gdb-patches@sourceware.org Subject: Re: [PING] [PATCH] Fix gdb.fortran/array-element.exp failures. References: <1404453487-20108-1-git-send-email-gabriel@krisman.be> <m3iondcf2h.fsf@redhat.com> <87zjgobth0.fsf@Argo.krisman.be> <m3pphk9f53.fsf@redhat.com> <8761jbbu6l.fsf@Argo.krisman.be> <m3bnt28g3s.fsf@redhat.com> <87lhrv36pm.fsf@Argo.krisman.be> <87zjf3sqrd.fsf@krisman.be> <87r4048546.fsf_-_@krisman.be> <20140909130901.GG28404@adacore.com> Date: Tue, 09 Sep 2014 11:55:46 -0300 In-Reply-To: <20140909130901.GG28404@adacore.com> (Joel Brobecker's message of "Tue, 9 Sep 2014 06:09:01 -0700") Message-ID: <87d2b4c07h.fsf@anubis.Home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-IsSubscribed: yes |
Commit Message
Gabriel Krisman Bertazi
Sept. 9, 2014, 2:55 p.m. UTC
Joel, > To help us out, it's useful to show what the patch does in practice. > In this case, I'd like to do what's not working before the patch > gets applied, and how things change once your patch is applied. > GDB transcripts before and after usually help with that. Sorry I didn't resend the original message with my pings. Here it goes. This fixes two FAIL results when running this test file due to a misplaced "continue" command, which caused the inferior to end execution prematurely. On trunk, we have: FAIL: gdb.fortran/array-element.exp: continue to breakpoint once again (the program exited) FAIL: gdb.fortran/array-element.exp: print the second element of array a === gdb Summary === # of expected passes 3 # of unexpected failures 2 And now, we get: === gdb Summary === # of expected passes 3 Is this ok? 2014-07-04 Gabriel Krisman Bertazi <gabriel@krisman.be> * gdb.fortran/array-element.exp: Remove wrong "continue" command. Simplify test case.
Comments
> Sorry I didn't resend the original message with my pings. Here it goes. > > This fixes two FAIL results when running this test file due to a > misplaced "continue" command, which caused the inferior to end execution > prematurely. > > On trunk, we have: > > FAIL: gdb.fortran/array-element.exp: continue to breakpoint once again > (the program exited) > FAIL: gdb.fortran/array-element.exp: print the second element of array a > > === gdb Summary === > > # of expected passes 3 > # of unexpected failures 2 > > And now, we get: > === gdb Summary === > # of expected passes 3 > > Is this ok? The problem is that this does not tell me what was wrong. Just that some tests did not pass. Although your change seems OK at first glance, I'd like to do know what it was that did not match, and why it's OK stop trying to match it now.
Joel Brobecker <brobecker@adacore.com> writes: > The problem is that this does not tell me what was wrong. Just that > some tests did not pass. Although your change seems OK at first glance, > I'd like to do know what it was that did not match, and why it's OK > stop trying to match it now. Joel, thanks for your clarification. Ok, so we got this situation: The original testcase sets a breakpoint at the label continue and resumes execution until we reach it. On the Fortran file, this means the inferior has iterated over the whole loop before reaching the breakpoint for the first time. Then, the original testcase issues another continue command, causing the inferior to finish the execution earlier than expected, since we still want to make a final test on whether we print the second element. This causes the two test failures. My guess is that the original author meant to break after each loop iteration, instead of going all the way until the continue label. Nevertheless, stepping over a single iteration or stopping after the entire loop has no impact on the test results. So, what my patch does is simply remove the second "continue" command that would prematurely end inferior's execution, so we can actually test whether both elements are printed correctly after executing the loop. Other than that, when I first submitted this patch, Sergio asked me to simplify the testcase, because it felt unusual. That is what the other modifications are about. I'll make sure to update the commit message to include part of this explanation to better clarify what this patch is really about. Thanks,
> > Joel, thanks for your clarification. > > Ok, so we got this situation: > > The original testcase sets a breakpoint at the label continue and > resumes execution until we reach it. On the Fortran file, this means > the inferior has iterated over the whole loop before reaching the > breakpoint for the first time. Then, the original testcase issues > another continue command, causing the inferior to finish the execution > earlier than expected, since we still want to make a final test on > whether we print the second element. This causes the two test failures. > > My guess is that the original author meant to break after each loop > iteration, instead of going all the way until the continue label. > > Nevertheless, stepping over a single iteration or stopping after the > entire loop has no impact on the test results. So, what my patch does > is simply remove the second "continue" command that would prematurely > end inferior's execution, so we can actually test whether both elements > are printed correctly after executing the loop. > > Other than that, when I first submitted this patch, Sergio asked me to > simplify the testcase, because it felt unusual. That is what the other > modifications are about. OK, thank you for the explanation of the issue. > 2014-08-17 Gabriel Krisman Bertazi <gabriel@krisman.be> > > * gdb.fortran/array-element.exp: Remove wrong "continue" > command. Simplify test case. OK to push. My only comment is that simplications are indeed good, but it is better if you can submit those separately from other changes. It's easier to review the patch series that way, and it also allows us separate the real change from the enhancement which is expected to be a no-op. Thanks :)
On Wednesday, September 10 2014, Joel Brobecker wrote: > My only comment is that simplications are indeed good, but it is > better if you can submit those separately from other changes. > It's easier to review the patch series that way, and it also > allows us separate the real change from the enhancement which > is expected to be a no-op. Mea culpa. I thought it would be nice to simplify the testcase in one shot, because it seemed to me that the error happened *also* because the testcase was not very clear. And since I had applied the patch, investigated and figured out that it did the right thing, I thought it'd be good to simplify it too. Sorry if it made the review harder!
Joel Brobecker <brobecker@adacore.com> writes:
> OK to push.
Just pushed this: <https://sourceware.org/ml/gdb-cvs/2014-09/msg00046.html>.
Thanks :)
diff --git a/gdb/testsuite/gdb.fortran/array-element.exp b/gdb/testsuite/gdb.fortran/array-element.exp index 579db03..1ac3623 100644 --- a/gdb/testsuite/gdb.fortran/array-element.exp +++ b/gdb/testsuite/gdb.fortran/array-element.exp @@ -31,18 +31,9 @@ if ![runto sub_] then { continue } -set bp_location [gdb_get_line_number "continue"] -gdb_test "break $bp_location" \ - "Breakpoint.*at.* file .*$srcfile, line $bp_location\\." \ - "breakpoint at continue" - -gdb_test "continue" \ - "Continuing\\..*Breakpoint.*" \ - "continue to breakpoint" -gdb_test "print a(1)" ".*1 = 1.*" "print the first element of array a" +gdb_breakpoint [gdb_get_line_number "continue"] +gdb_continue_to_breakpoint "continue" -gdb_test "continue" \ - "Continuing\\..*Breakpoint.*" \ - "continue to breakpoint once again" +gdb_test "print a(1)" ".*1 = 1.*" "print the first element of array a" gdb_test "print a(2)" ".*2 = 2.*" "print the second element of array a"