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

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

[Thiago Jung Bauermann]

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 v2:
- Put "^main" in the correct re in "$step_out == 1" case (pointed out by Bruno).

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 | 28 +++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
index 729218d4cb8c..4b78a8f8fb75 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,19 @@ 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.*" {
+	-re -wrap "^main.*NEXT OVER THIS RECURSION.*" {
 	    set step_out 0
+	    pass "$gdb_test_name"
 	}
-	-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
 	}
@@ -276,9 +285,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 +302,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
     }

base-commit: 3c5e824b9cee93a987a77906240c509add260a0d

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

[Bruno Larsen]

On 06/06/2023 01:28, 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>
> ---

Thanks for fixing all the nits, looks good to me now. Reviewed-By: Bruno 
Larsen <blarsen@redhat.com>

(Just making sure you know, the rb tag is not enough to push, you need 
an approval)

-- 
Cheers,
Bruno

>
> Changes since v2:
> - Put "^main" in the correct re in "$step_out == 1" case (pointed out by Bruno).
>
> 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 | 28 +++++++++++++++++++---
>   1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.reverse/step-reverse.exp b/gdb/testsuite/gdb.reverse/step-reverse.exp
> index 729218d4cb8c..4b78a8f8fb75 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,19 @@ 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.*" {
> +	-re -wrap "^main.*NEXT OVER THIS RECURSION.*" {
>   	    set step_out 0
> +	    pass "$gdb_test_name"
>   	}
> -	-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
>   	}
> @@ -276,9 +285,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 +302,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
>       }
>
> base-commit: 3c5e824b9cee93a987a77906240c509add260a0d
>

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

[Thiago Jung Bauermann]

Bruno Larsen <blarsen@redhat.com> writes:

> On 06/06/2023 01:28, 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>
>> ---
>
> Thanks for fixing all the nits, looks good to me now. Reviewed-By: Bruno Larsen
> <blarsen@redhat.com>
>
> (Just making sure you know, the rb tag is not enough to push, you need an approval)

Ping.

-- 
Thiago

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

[Tom de Vries]

On 6/23/23 00:52, Thiago Jung Bauermann wrote:
> 
> Bruno Larsen <blarsen@redhat.com> writes:
> 
>> On 06/06/2023 01:28, 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>
>>> ---
>>
>> Thanks for fixing all the nits, looks good to me now. Reviewed-By: Bruno Larsen
>> <blarsen@redhat.com>
>>
>> (Just making sure you know, the rb tag is not enough to push, you need an approval)
> 

Hi,

LGTM, thanks.

Approved-By: Tom de Vries <tdevries@suse.de>

- Tom

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

[Thiago Jung Bauermann]

Hello,

Tom de Vries <tdevries@suse.de> writes:

> On 6/23/23 00:52, Thiago Jung Bauermann wrote:
>> Bruno Larsen <blarsen@redhat.com> writes:
>> 
>>> On 06/06/2023 01:28, 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>
>>>> ---
>>>
>>> Thanks for fixing all the nits, looks good to me now. Reviewed-By: Bruno Larsen
>>> <blarsen@redhat.com>
>>>
>>> (Just making sure you know, the rb tag is not enough to push, you need an approval)
>> 
>
> Hi,
>
> LGTM, thanks.
>
> Approved-By: Tom de Vries <tdevries@suse.de>

Thank you! I have just pushed it.

-- 
Thiago