Message ID | 20230601214239.209822-1-thiago.bauermann@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id CD1473856DF1 for <patchwork@sourceware.org>; Thu, 1 Jun 2023 21:43:25 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org CD1473856DF1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1685655805; bh=alMYR/55StAdtqsYjJQ+z/ZygS4VrWXlev9HUxDnRBo=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=lZFDPUn+tg8dTClhl9sshb1GzNzPtIpcePPIdE88CayqXZ6ifrSUEiSpEGiaTELgG QNrDxruMRNAtsganZlxg6GZC6eM4wcKfoeQG6tUeSlN8WOLY170N3BMA3cgE5w5L/5 jHD4VMYGr/wmjpLtY4GU3CuKteTTfjHRjkLmrdpc= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-ed1-x535.google.com (mail-ed1-x535.google.com [IPv6:2a00:1450:4864:20::535]) by sourceware.org (Postfix) with ESMTPS id 5E2C53858288 for <gdb-patches@sourceware.org>; Thu, 1 Jun 2023 21:43:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5E2C53858288 Received: by mail-ed1-x535.google.com with SMTP id 4fb4d7f45d1cf-5147f4bbfdaso1975971a12.0 for <gdb-patches@sourceware.org>; Thu, 01 Jun 2023 14:43:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685655780; x=1688247780; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=alMYR/55StAdtqsYjJQ+z/ZygS4VrWXlev9HUxDnRBo=; b=Aqg5P4stBA0XwlGAjN2/JGOcVM8lG0HU6UrHx9CO2TbdU9bkIGUznK/UFisnk1aivH oYs8Gg32Z9aNX4dE1cKGuDWC3kPbcgp4+MyCQxmRtVFdfx8QaPsZd7GvdRznuJd1uYzW pjb/XIZIG2mXeEaBSf5BczKYPd68mPthOGA4QNWDDGLTdMO+9DJGdke9q9lPo+mMx1SL yy53lt7UKdwJO7XudE/EExsqPiLgPnmFgMsJCE3XIwAisziAVQJsXrCbbCybplcP9r6s L/mHZi3jPIdiLusWmCJlD4o4j6uTDw/GhXlbmjvw5HO+if7+UsDj5059+3AM+hMnXEIN Q7lw== X-Gm-Message-State: AC+VfDxzJUTUFdj1Ldpgshhr1AgvWWnP/SXdf+Qn+KtXXv1ZVv15asyh xlLMirT8U/PEKeq+FwdNzjfP8FJhRc9esNJ7K60= X-Google-Smtp-Source: ACHHUZ64dV4Myk497d84UVSxbOlNYQPj6iorcspJvFztPmoZd6wEWShqdpX0guKQWt5z4mJ4qeORSA== X-Received: by 2002:a17:907:268c:b0:94e:cf72:8147 with SMTP id bn12-20020a170907268c00b0094ecf728147mr9207502ejc.48.1685655779965; Thu, 01 Jun 2023 14:42:59 -0700 (PDT) Received: from localhost (p200300eaff33e08878ebf65188eb3c13.dip0.t-ipconnect.de. [2003:ea:ff33:e088:78eb:f651:88eb:3c13]) by smtp.gmail.com with ESMTPSA id q9-20020a170906940900b009545230e682sm11129475ejx.91.2023.06.01.14.42.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Jun 2023 14:42:59 -0700 (PDT) To: gdb-patches@sourceware.org Cc: Bruno Larsen <blarsen@redhat.com>, Andrew Burgess <aburgess@redhat.com>, Tom de Vries <tdevries@suse.de>, Thiago Jung Bauermann <thiago.bauermann@linaro.org> Subject: [PATCH v2] gdb/testsuite: Avoid infinite loop in gdb.reverse/step-reverse.exp Date: Thu, 1 Jun 2023 23:42:39 +0200 Message-Id: <20230601214239.209822-1-thiago.bauermann@linaro.org> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> From: Thiago Jung Bauermann via Gdb-patches <gdb-patches@sourceware.org> Reply-To: Thiago Jung Bauermann <thiago.bauermann@linaro.org> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
[v2] gdb/testsuite: Avoid infinite loop in gdb.reverse/step-reverse.exp
|
|
Commit Message
Thiago Jung Bauermann
June 1, 2023, 9:42 p.m. UTC
This testcase sometimes gets stuck in a loop for hours when running in our CI. The problem is that due to an issue unrelated to reverse debugging the inferior exits early, and because of the overly generic ".*" pattern the testcase keeps sending the "next" command without noticing that the inferior is gone. gdb_test_multiple has a pattern to detect that "The program is not being run.", but since it is placed after the patterns from the caller it won't be triggered. It also has a timeout pattern but because it is triggered between successful matches, each time the test matches the '-re -wrap ".*"' this counts as a successful match and the timeout is reset. Since the test binary is compiled with debug information, fix by changing one of the generic patterns to match entering the main function and the other one to match the source code line number that is shown by GDB right after the "step" command. Also, as a precaution add a maximum number of times the "next" command will be sent. Co-Authored-By: Tom de Vries <tdevries@suse.de> --- Changes since v1: - Added maximum number of iterations (code provided by Tom). - Changed one of the patterns to match the main function (suggested by Bruno). - Clarified why the timeout pattern isn't effective (explanation from Andrew). Tom, Most of the patch now is the iteration-counting code. Since I copied it from your email, I thought it made sense to record you as co-author. I hope it's ok. gdb/testsuite/gdb.reverse/step-reverse.exp | 25 ++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) base-commit: d987df5c95340a8b41b23d68ad6a8f9f33485835
Comments
On 01/06/2023 23:42, Thiago Jung Bauermann wrote: > This testcase sometimes gets stuck in a loop for hours when running in our > CI. The problem is that due to an issue unrelated to reverse debugging the > inferior exits early, and because of the overly generic ".*" pattern the > testcase keeps sending the "next" command without noticing that the > inferior is gone. > > gdb_test_multiple has a pattern to detect that "The program is not being > run.", but since it is placed after the patterns from the caller it won't > be triggered. It also has a timeout pattern but because it is triggered > between successful matches, each time the test matches the '-re -wrap ".*"' > this counts as a successful match and the timeout is reset. > > Since the test binary is compiled with debug information, fix by changing > one of the generic patterns to match entering the main function and the > other one to match the source code line number that is shown by GDB right > after the "step" command. > > Also, as a precaution add a maximum number of times the "next" command will > be sent. > > Co-Authored-By: Tom de Vries <tdevries@suse.de> > --- > > Changes since v1: > - Added maximum number of iterations (code provided by Tom). > - Changed one of the patterns to match the main function (suggested by Bruno). > - Clarified why the timeout pattern isn't effective (explanation from Andrew). > > Tom, > > Most of the patch now is the iteration-counting code. Since I copied it > from your email, I thought it made sense to record you as co-author. > I hope it's ok. > > gdb/testsuite/gdb.reverse/step-reverse.exp | 25 ++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp > index 729218d4cb8c..c70d2fcb5166 100644 > --- a/gdb/testsuite/gdb.reverse/step-reverse.exp > +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp > @@ -247,6 +247,7 @@ gdb_test_multiple "step" "$test_message" { > gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call" > > set step_out 0 > +set max_iterations 1000 > gdb_test_multiple "next" "reverse next over recursion" { > -re -wrap ".*NEXT OVER THIS RECURSION.*" { > pass "$gdb_test_name" > @@ -257,11 +258,18 @@ gdb_test_multiple "next" "reverse next over recursion" { > } > } > if { "$step_out" == 1 } { > + set iterations 0 > gdb_test_multiple "next" "stepping out of recursion" { > -re -wrap "NEXT OVER THIS RECURSION.*" { > set step_out 0 > } > - -re -wrap ".*" { > + -re -wrap "^main .*" { > + incr iterations > + if { $iterations == $max_iterations } { > + fail "$gdb_test_name (reached $max_iterations iterations)" > + return > + } > + I'm fine with the idea of a maximum iteration count (even if I find it unnecessary), but your use of it here is incorrect. What your code is doing is doing is _only_ exiting once you see GDB returning to the main function $max_iterations times. if { "$step_out" == 1 } { gdb_test_multiple "next" "stepping out of recursion" { -re -wrap "NEXT OVER THIS RECURSION.*" { set step_out 0 + pass "$gdb_test_name" } - -re -wrap ".*" { + -re -wrap "^main.*" { + pass "$gdb_test_name" + } + -re -wrap "^\[0-9\].*" { + incr iterations + if { $iterations == $max_iterations } { + fail "$gdb_test_name (reached $max_iterations iterations)" + return + } send_gdb "next\n" exp_continue } } } And I wonder if we'd like to add a pass if step_out was never set to 1, so we don't get fewer passes when everything works? Not sure, I'd defer to Andrew or Tom on this one.
Bruno Larsen <blarsen@redhat.com> writes: > On 01/06/2023 23:42, Thiago Jung Bauermann wrote: >> @@ -257,11 +258,18 @@ gdb_test_multiple "next" "reverse next over recursion" { >> } >> } >> if { "$step_out" == 1 } { >> + set iterations 0 >> gdb_test_multiple "next" "stepping out of recursion" { >> -re -wrap "NEXT OVER THIS RECURSION.*" { >> set step_out 0 >> } >> - -re -wrap ".*" { >> + -re -wrap "^main .*" { >> + incr iterations >> + if { $iterations == $max_iterations } { >> + fail "$gdb_test_name (reached $max_iterations iterations)" >> + return >> + } >> + > > I'm fine with the idea of a maximum iteration count (even if I find it unnecessary), but > your use of it here is incorrect. What your code is doing is doing is _only_ exiting once > you see GDB returning to the main function $max_iterations times. > > if { "$step_out" == 1 } { > gdb_test_multiple "next" "stepping out of recursion" { > -re -wrap "NEXT OVER THIS RECURSION.*" { > set step_out 0 > + pass "$gdb_test_name" > } > - -re -wrap ".*" { > + -re -wrap "^main.*" { > + pass "$gdb_test_name" > + } > + -re -wrap "^\[0-9\].*" { > + incr iterations > + if { $iterations == $max_iterations } { > + fail "$gdb_test_name (reached $max_iterations iterations)" > + return > + } > send_gdb "next\n" > exp_continue > } > } > } Ouch, sorry about that. The "$step_out == 1" case doesn't trigger for me, so I'm not testing that part of the patch and I didn't notice the problem. I sent a v3 with a slightly different change from what you posted above. I put the "^main.*" part in the beginning of the "NEXT OVER THIS RECURSION.*" regex. I thought that it makes sense because both strings should appear in the same command output according to the excerpt you pasted in the thread discussing v1 of this patch. Please let me know if you would prefer me to use the alternative above. > And I wonder if we'd like to add a pass if step_out was never set to 1, so we don't get > fewer passes when everything works? Not sure, I'd defer to Andrew or Tom on this one. IMHO it's not necessary for the number of PASS to be the same if the testcase passes or fails, but I'll wait for their input.
On 06/06/2023 01:35, Thiago Jung Bauermann wrote: > Bruno Larsen <blarsen@redhat.com> writes: > >> On 01/06/2023 23:42, Thiago Jung Bauermann wrote: >>> @@ -257,11 +258,18 @@ gdb_test_multiple "next" "reverse next over recursion" { >>> } >>> } >>> if { "$step_out" == 1 } { >>> + set iterations 0 >>> gdb_test_multiple "next" "stepping out of recursion" { >>> -re -wrap "NEXT OVER THIS RECURSION.*" { >>> set step_out 0 >>> } >>> - -re -wrap ".*" { >>> + -re -wrap "^main .*" { >>> + incr iterations >>> + if { $iterations == $max_iterations } { >>> + fail "$gdb_test_name (reached $max_iterations iterations)" >>> + return >>> + } >>> + >> I'm fine with the idea of a maximum iteration count (even if I find it unnecessary), but >> your use of it here is incorrect. What your code is doing is doing is _only_ exiting once >> you see GDB returning to the main function $max_iterations times. >> >> if { "$step_out" == 1 } { >> gdb_test_multiple "next" "stepping out of recursion" { >> -re -wrap "NEXT OVER THIS RECURSION.*" { >> set step_out 0 >> + pass "$gdb_test_name" >> } >> - -re -wrap ".*" { >> + -re -wrap "^main.*" { >> + pass "$gdb_test_name" >> + } >> + -re -wrap "^\[0-9\].*" { >> + incr iterations >> + if { $iterations == $max_iterations } { >> + fail "$gdb_test_name (reached $max_iterations iterations)" >> + return >> + } >> send_gdb "next\n" >> exp_continue >> } >> } >> } > Ouch, sorry about that. The "$step_out == 1" case doesn't trigger for > me, so I'm not testing that part of the patch and I didn't notice the > problem. > > I sent a v3 with a slightly different change from what you posted above. > I put the "^main.*" part in the beginning of the "NEXT OVER THIS > RECURSION.*" regex. I thought that it makes sense because both strings > should appear in the same command output according to the excerpt you > pasted in the thread discussing v1 of this patch. Please let me know if > you would prefer me to use the alternative above. oops, my bad! I forgot the fact that "NEXT OVER THIS RECURSION" comes at the same time, and didn't think to check that. Since both of those outputs are together, there is no point in having 2 cases. You can choose to keep either one, and given my blunder here I would suggest keeping the "^main.*" regexp, but that is up to you. Sorry for the run-around with this one.
Bruno Larsen <blarsen@redhat.com> writes: > On 06/06/2023 01:35, Thiago Jung Bauermann wrote: >> Bruno Larsen <blarsen@redhat.com> writes: >> >>> On 01/06/2023 23:42, Thiago Jung Bauermann wrote: >>>> @@ -257,11 +258,18 @@ gdb_test_multiple "next" "reverse next over recursion" { >>>> } >>>> } >>>> if { "$step_out" == 1 } { >>>> + set iterations 0 >>>> gdb_test_multiple "next" "stepping out of recursion" { >>>> -re -wrap "NEXT OVER THIS RECURSION.*" { >>>> set step_out 0 >>>> } >>>> - -re -wrap ".*" { >>>> + -re -wrap "^main .*" { >>>> + incr iterations >>>> + if { $iterations == $max_iterations } { >>>> + fail "$gdb_test_name (reached $max_iterations iterations)" >>>> + return >>>> + } >>>> + >>> I'm fine with the idea of a maximum iteration count (even if I find it unnecessary), >>> but >>> your use of it here is incorrect. What your code is doing is doing is _only_ exiting >>> once >>> you see GDB returning to the main function $max_iterations times. >>> >>> if { "$step_out" == 1 } { >>> gdb_test_multiple "next" "stepping out of recursion" { >>> -re -wrap "NEXT OVER THIS RECURSION.*" { >>> set step_out 0 >>> + pass "$gdb_test_name" >>> } >>> - -re -wrap ".*" { >>> + -re -wrap "^main.*" { >>> + pass "$gdb_test_name" >>> + } >>> + -re -wrap "^\[0-9\].*" { >>> + incr iterations >>> + if { $iterations == $max_iterations } { >>> + fail "$gdb_test_name (reached $max_iterations iterations)" >>> + return >>> + } >>> send_gdb "next\n" >>> exp_continue >>> } >>> } >>> } >> Ouch, sorry about that. The "$step_out == 1" case doesn't trigger for >> me, so I'm not testing that part of the patch and I didn't notice the >> problem. >> >> I sent a v3 with a slightly different change from what you posted above. >> I put the "^main.*" part in the beginning of the "NEXT OVER THIS >> RECURSION.*" regex. I thought that it makes sense because both strings >> should appear in the same command output according to the excerpt you >> pasted in the thread discussing v1 of this patch. Please let me know if >> you would prefer me to use the alternative above. > > oops, my bad! I forgot the fact that "NEXT OVER THIS RECURSION" comes at the same time, > and didn't think to check that. Since both of those outputs are together, there is no > point in having 2 cases. You can choose to keep either one, and given my blunder here I > would suggest keeping the "^main.*" regexp, but that is up to you. To be honest I like the way it is in v3: -re -wrap "^main.*NEXT OVER THIS RECURSION.*" { set step_out 0 pass "$gdb_test_name" } with the "^main" part anchoring the RE and the "NEXT OVER THIS RECURSION" part making it clear in which source code line the inferior is supposed to be. Do you prefer keeping only the "^main.*" part? > Sorry for the run-around with this one. No problem at all. Thank you for your reviews and suggestions.
diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp index 729218d4cb8c..c70d2fcb5166 100644 --- a/gdb/testsuite/gdb.reverse/step-reverse.exp +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp @@ -247,6 +247,7 @@ gdb_test_multiple "step" "$test_message" { gdb_test "next" ".*NEXT OVER THIS CALL.*" "reverse next over call" set step_out 0 +set max_iterations 1000 gdb_test_multiple "next" "reverse next over recursion" { -re -wrap ".*NEXT OVER THIS RECURSION.*" { pass "$gdb_test_name" @@ -257,11 +258,18 @@ gdb_test_multiple "next" "reverse next over recursion" { } } if { "$step_out" == 1 } { + set iterations 0 gdb_test_multiple "next" "stepping out of recursion" { -re -wrap "NEXT OVER THIS RECURSION.*" { set step_out 0 } - -re -wrap ".*" { + -re -wrap "^main .*" { + incr iterations + if { $iterations == $max_iterations } { + fail "$gdb_test_name (reached $max_iterations iterations)" + return + } + send_gdb "next\n" exp_continue } @@ -276,9 +284,16 @@ gdb_test_no_output "set exec-dir reverse" "reverse again to test recursion" gdb_test "step" ".*EXIT RECURSIVE FUNCTION.*" "enter recursive function" set seen_recursive_call 0 +set iterations 0 gdb_test_multiple "next" "step over recursion inside the recursion" { -re -wrap ".*RECURSIVE CALL.*" { incr seen_recursive_call + incr iterations + if { $iterations == $max_iterations } { + fail "$gdb_test_name (reached $max_iterations iterations)" + return + } + send_gdb "next\n" exp_continue } @@ -286,7 +301,13 @@ gdb_test_multiple "next" "step over recursion inside the recursion" { gdb_assert {"$seen_recursive_call" == 1} \ "step over recursion inside the recursion" } - -re -wrap ".*" { + -re -wrap "^\[0-9\].*" { + incr iterations + if { $iterations == $max_iterations } { + fail "$gdb_test_name (reached $max_iterations iterations)" + return + } + send_gdb "next\n" exp_continue }