[PATCH] gdb/testsuite: Avoid infinite loop in gdb.reverse/step-reverse.exp

[Tom de Vries]

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.

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.

Thanks,
- Tom