From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 634DE3857007 for ; Thu, 1 Jun 2023 09:40:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 634DE3857007 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1685612429; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=y7IsCUWUmu75K82mN/Wh3iY7pfywSmg6bMRDvQCuod0=; b=CmuvCp4Lc5Wak3u0QMGBIy5NapWTKnYH1EXLfG/TDNvxE+gSuAhy+B8pkipvPMnN21QI5z RIk/LUmasHF04vlbc/PyiziBg9Gnbl5ydPfMEa5wM1VQQr37rQ38/adjqGiSXrwnMEhUcf 2JNp9R7n5Nh04TcvRrTqwSE8ZOcEjOk= Received: from mail-ej1-f69.google.com (mail-ej1-f69.google.com [209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-373--hOqAiKaOkyGeltfLVXyvg-1; Thu, 01 Jun 2023 05:40:28 -0400 X-MC-Unique: -hOqAiKaOkyGeltfLVXyvg-1 Received: by mail-ej1-f69.google.com with SMTP id a640c23a62f3a-94a348facbbso44817466b.1 for ; Thu, 01 Jun 2023 02:40:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685612427; x=1688204427; h=content-transfer-encoding:in-reply-to:from:references:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=y7IsCUWUmu75K82mN/Wh3iY7pfywSmg6bMRDvQCuod0=; b=dknJPL/yKoIlbXRTEIwqltaCudaWtyaTP/Jw/cuFWQMZvpVaepTtAq98moQ84KGbhM U2gg6QfXeIK+LyYxLsmIfw9239JAZxR8KMLwX1l7INh8CScKLVSrVWfx48g5lE0Ud856 nkzMQjOIzrUY6Jo3jOUfZtlEZ9gU2KgFFT6BSCocUrU1xP5v6M3DG/DBC+xaLWoXNB8w 4MbnQc6x6fGG26e0udhOwMgayDMz8J0OokMOqw5xr8H8ALp5cAbbn5d1KKaUPRosbLuH a66Gn0j/ejweRZoEk56lSHtUojt8UCkgGMSaaEoAXDBGnUdWmkUITp13LLzCh8EZymcc i+RA== X-Gm-Message-State: AC+VfDxdknwsf5vOfFe+PfUhXWa/0UAQ08NrvxWP/Z3VDDnxBdl7HsEG 9YBlCjrrRv7GhsrxQsQLE2qwsY55/UmPGRhqDIo2sEw52qNVu00XdIHeH0zq8ZxG7SrX81cmYGh COmmh3fxwGbplzCw+4pWiFQ== X-Received: by 2002:a17:907:25c8:b0:94a:7979:41f5 with SMTP id ae8-20020a17090725c800b0094a797941f5mr6582997ejc.71.1685612427720; Thu, 01 Jun 2023 02:40:27 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ63bBj3RckHHcCUQZEiOHbGHEqPxJJ946+KbSPFZOF28NmnrbPmiBPOcXWFdovMTYDuoRplgw== X-Received: by 2002:a17:907:25c8:b0:94a:7979:41f5 with SMTP id ae8-20020a17090725c800b0094a797941f5mr6582985ejc.71.1685612427424; Thu, 01 Jun 2023 02:40:27 -0700 (PDT) Received: from [10.43.2.191] (nat-pool-brq-t.redhat.com. [213.175.37.10]) by smtp.gmail.com with ESMTPSA id f19-20020a170906139300b0095fd0462695sm10218411ejc.5.2023.06.01.02.40.26 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 01 Jun 2023 02:40:26 -0700 (PDT) Message-ID: <9717cbae-90c9-e1b0-9344-651e57c80a36@redhat.com> Date: Thu, 1 Jun 2023 11:40:26 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH] gdb/testsuite: Avoid infinite loop in gdb.reverse/step-reverse.exp To: Tom de Vries , Thiago Jung Bauermann , gdb-patches@sourceware.org References: <20230531200602.165033-1-thiago.bauermann@linaro.org> From: Bruno Larsen In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00,BODY_8BITS,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,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 List-Id: On 01/06/2023 11:15, Tom de Vries wrote: > On 5/31/23 22:06, Thiago Jung Bauermann via Gdb-patches 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 for some reason I don't >> understand it often doesn't trigger. >> >> Since the test binary is compiled with debug information, fix by >> changing >> the pattern to match the source code line number that is shown by GDB >> right >> after the "step" command. >> --- >>   gdb/testsuite/gdb.reverse/step-reverse.exp | 4 ++-- >>   1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp >> b/gdb/testsuite/gdb.reverse/step-reverse.exp >> index 729218d4cb8c..766ca02910af 100644 >> --- a/gdb/testsuite/gdb.reverse/step-reverse.exp >> +++ b/gdb/testsuite/gdb.reverse/step-reverse.exp >> @@ -261,7 +261,7 @@ if { "$step_out" == 1 } { >>       -re -wrap "NEXT OVER THIS RECURSION.*" { >>           set step_out 0 >>       } >> -    -re -wrap ".*" { >> +    -re -wrap "^\[0-9\].*"  { >>           send_gdb "next\n" >>           exp_continue >>       } >> @@ -286,7 +286,7 @@ 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\].*" { >>       send_gdb "next\n" >>       exp_continue >>       } > > Hi, > > the fix as is addresses the immediate problem. > > Introducing an iteration bound as a safety net is probably a good idea > as well, given the problem you ran into. > > Also, I prefer a different style of fix.  I think that this > gdb_test_multiple is overly complicated by dealing with the prompt in > two clauses. > > I came up with this as first version which removes the need for any .* > usage (though to be clear, -wrap "" and -wrap ".*" is the same thing): > ... > if { "$step_out" == 1 } { >     gdb_test_multiple "next" "stepping out of recursion" { >         -re "NEXT OVER THIS RECURSION" { >             set step_out 0 >             exp_continue >         } >         -re -wrap "" { >             if { $step_out == 1 } { >                 send_gdb "next\n" >                 exp_continue >             } >             pass $gdb_test_name >         } >     } > } > ... > and this after adding the iteration bound: > ... > if { "$step_out" == 1 } { >     set iterations 0 >     set max_iterations 10 >     gdb_test_multiple "next" "stepping out of recursion" { >         -re "NEXT OVER THIS RECURSION" { >             set step_out 0 >             exp_continue >         } >         -re -wrap "" { >             if { $step_out == 1 } { >                 incr iterations >                 if { $iterations == $max_iterations } { >                     fail $gdb_test_name >                     return >                 } > >                 send_gdb "next\n" >                 exp_continue >             } >             pass $gdb_test_name >         } >     } > } > ... > > [ The gdb_test_multiple can be even further simplified, by wrapping it > in a while that takes care of the iteration. ] > > This code doesn't actually trigger for me, so I added this: > ... > -gdb_test_multiple "next" "reverse next over recursion" { > +gdb_test_multiple "step" "reverse next over recursion" { > ... > > That gave me the default fail: > ... > (gdb) PASS: gdb.reverse/step-reverse.exp: reverse next over call > step^M > recursive_callee (val=17) at > /data/vries/gdb/src/gdb/testsuite/gdb.reverse/step-reverse.c:41^M > 41      } /* EXIT RECURSIVE FUNCTION */^M > (gdb) FAIL: gdb.reverse/step-reverse.exp: reverse next over recursion > ... > which did not set step_out to 1, so still the code didn't get > triggered,so I did: > ... > -    -re -wrap ".*RECURSIVE CALL.*" { > +    -re -wrap ".*(RECURSIVE CALL|EXIT RECURSIVE FUNCTION).*" { > ... > > ISTM that the test-case contains complicated and hard-to-trigger code, > which could be fixed by merging the code for the next and step scenario. > Some of the changes you introduced are not correct with the original idea of the test. As a refresher, the problem that is being tested is that GDB would not be able to reverse-next and skip recursive functions (details: PR 16678). It would skip the first call then stop, which in this test case would still be 4 calls deep into the recursion when we wanted to stop on main. The reason the test works in 2 clauses is because only the first part is actually testing the fix (that we can reverse-next without stopping at a recursive call site), the second case is just resetting the inferior's state into a known position in case the first test fails, so we dont get the whole test being useless if this feature ever regresses. That's why the variable I used is called "step_out", it knows if we have to manually step out of all recursive calls. You could simplify this case into a single test by only emitting a pass/fail when you leave the loop and asserting that step_out is 0, but I thought it would be even harder to figure out what the test was doing in the future. Another problem with adding a maximum iteration count (and the reason I didn't go for it originally) is because you have to have enough iterations to step out of 4 function calls, and I didn't want to hard code that number because changes to the function would not be immediately obvious while the use case continued to work. Another way to know when the inferior is at the expected state is to look for the message that we're back at the main function, so we avoid using ".*" in the match. -- Cheers, Bruno > Anyway, I'm fine with your fix, if an iteration bound is added. > > But I think at least this part of the test-case could do with a rewrite.