[PATCH] gdb/testsuite/gdb.rocm: Fix incorrect use of continue N in multi-inferior-gpu.exp

[PATCH] gdb/testsuite/gdb.rocm: Fix incorrect use of continue N in multi-inferior-gpu.exp

[Lancelot Six]

The gdb.rocm/multi-inferior-gpu.exp testcase uses a "continue $thread"
command, but this is incorrect.  If "continue" is given an argument, it
sets the ignore count of the breakpoint the thread stopped at.

For this testcase it does not really matter since the breakpoint is not
meant to be hit anymore, so whatever the ignore count is won't influence
the outcome of the test.  It is worth fixing nevertheless.

While at changing it, switch to using "continue&" to consume the GDB
prompt right away.  This makes the following pattern matching more
reliable.

Change-Id: I0eb674d5529cdeb9e808b74870a29b6077265737
---
 gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp b/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
index 18b4172ff09..958b70e6df1 100644
--- a/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
+++ b/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
@@ -68,8 +68,10 @@ proc do_test {} {
 	foreach thread $stopped_gpu_threads {
 	    set infnumber [lindex [split $thread .] 0]
 	    gdb_test "thread $thread" "Switching to thread.*"
-	    gdb_test_multiple "continue $thread" "" {
-		-re "\\\[Inferior $infnumber \[^\n\r\]* exited normally\\]\r\n$::gdb_prompt " {
+	    # Continue this thread, and consime the prompt
+	    gdb_test "continue&" "" "continue GPU thread in $infnumber"
+	    gdb_test_multiple "" "wait for inferior $infnumber" {
+		-re "\\\[Inferior $infnumber \[^\n\r\]* exited normally\\\]\r\n" {
 		    pass $gdb_test_name
 		}
 	    }
-- 
2.34.1

Re: [PATCH] gdb/testsuite/gdb.rocm: Fix incorrect use of continue N in multi-inferior-gpu.exp

[Lancelot SIX]

Hi,


> diff --git a/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp b/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
> index 18b4172ff09..958b70e6df1 100644
> --- a/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
> +++ b/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
> @@ -68,8 +68,10 @@ proc do_test {} {
>   	foreach thread $stopped_gpu_threads {
>   	    set infnumber [lindex [split $thread .] 0]
>   	    gdb_test "thread $thread" "Switching to thread.*"
> -	    gdb_test_multiple "continue $thread" "" {
> -		-re "\\\[Inferior $infnumber \[^\n\r\]* exited normally\\]\r\n$::gdb_prompt " {
> +	    # Continue this thread, and consime the prompt
                                             ^
There is a typo here.  I have fixed it locally.

Best,
Lancelot.

> +	    gdb_test "continue&" "" "continue GPU thread in $infnumber"
> +	    gdb_test_multiple "" "wait for inferior $infnumber" {
> +		-re "\\\[Inferior $infnumber \[^\n\r\]* exited normally\\\]\r\n" {
>   		    pass $gdb_test_name
>   		}
>   	    }

Re: [PATCH] gdb/testsuite/gdb.rocm: Fix incorrect use of continue N in multi-inferior-gpu.exp

[Simon Marchi]

On 10/17/23 07:16, Lancelot Six wrote:
> The gdb.rocm/multi-inferior-gpu.exp testcase uses a "continue $thread"
> command, but this is incorrect.  If "continue" is given an argument, it
> sets the ignore count of the breakpoint the thread stopped at.
> 
> For this testcase it does not really matter since the breakpoint is not
> meant to be hit anymore, so whatever the ignore count is won't influence
> the outcome of the test.  It is worth fixing nevertheless.
> 
> While at changing it, switch to using "continue&" to consume the GDB
> prompt right away.  This makes the following pattern matching more
> reliable.

I don't understand the "more reliable" part, can you explain?

Simon

Re: [PATCH] gdb/testsuite/gdb.rocm: Fix incorrect use of continue N in multi-inferior-gpu.exp

[Lancelot SIX]

> On 10/17/23 07:16, Lancelot Six wrote:
>> The gdb.rocm/multi-inferior-gpu.exp testcase uses a "continue $thread"
>> command, but this is incorrect.  If "continue" is given an argument, it
>> sets the ignore count of the breakpoint the thread stopped at.
>>
>> For this testcase it does not really matter since the breakpoint is not
>> meant to be hit anymore, so whatever the ignore count is won't influence
>> the outcome of the test.  It is worth fixing nevertheless.
>>
>> While at changing it, switch to using "continue&" to consume the GDB
>> prompt right away.  This makes the following pattern matching more
>> reliable.
> 
> I don't understand the "more reliable" part, can you explain?
> 
> Simon

There will be a "(gdb) " prompt appearing at some point after a 
"continue", but in non-stop we don't really control when it appears (if 
I understand correctly).  It seems that other non-stop tests use the "&" 
variant and consume the prompt immediately.

With this, before the next command is issued, we know that 1) we got a 
prompt and 2) we have seen the process we just released finish.

Overall, I am mostly trying to follow patterns I caught in other 
non-stop tests.

Best,
Lancelot.

Re: [PATCH] gdb/testsuite/gdb.rocm: Fix incorrect use of continue N in multi-inferior-gpu.exp

[Simon Marchi]

On 10/17/23 10:41, Lancelot SIX wrote:
> 
>> On 10/17/23 07:16, Lancelot Six wrote:
>>> The gdb.rocm/multi-inferior-gpu.exp testcase uses a "continue $thread"
>>> command, but this is incorrect.  If "continue" is given an argument, it
>>> sets the ignore count of the breakpoint the thread stopped at.
>>>
>>> For this testcase it does not really matter since the breakpoint is not
>>> meant to be hit anymore, so whatever the ignore count is won't influence
>>> the outcome of the test.  It is worth fixing nevertheless.
>>>
>>> While at changing it, switch to using "continue&" to consume the GDB
>>> prompt right away.  This makes the following pattern matching more
>>> reliable.
>>
>> I don't understand the "more reliable" part, can you explain?
>>
>> Simon
> 
> There will be a "(gdb) " prompt appearing at some point after a "continue", but in non-stop we don't really control when it appears (if I understand correctly).  It seems that other non-stop tests use the "&" variant and consume the prompt immediately.
> 
> With this, before the next command is issued, we know that 1) we got a prompt and 2) we have seen the process we just released finish.
> 
> Overall, I am mostly trying to follow patterns I caught in other non-stop tests.

Ah, I missed that we were in non-stop.

If we run a single thread at a time, I think it will be reliable to use
the sync variant of continue.  Using the async variant is useful when
you resume multiple threads and expect multiple events (like breakpoint
hits) which can happen in any order.  At least that's my understanding.

Simon

Re: [PATCH] gdb/testsuite/gdb.rocm: Fix incorrect use of continue N in multi-inferior-gpu.exp

[Lancelot SIX]

On 17/10/2023 15:48, Simon Marchi wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On 10/17/23 10:41, Lancelot SIX wrote:
>>
>>> On 10/17/23 07:16, Lancelot Six wrote:
>>>> The gdb.rocm/multi-inferior-gpu.exp testcase uses a "continue $thread"
>>>> command, but this is incorrect.  If "continue" is given an argument, it
>>>> sets the ignore count of the breakpoint the thread stopped at.
>>>>
>>>> For this testcase it does not really matter since the breakpoint is not
>>>> meant to be hit anymore, so whatever the ignore count is won't influence
>>>> the outcome of the test.  It is worth fixing nevertheless.
>>>>
>>>> While at changing it, switch to using "continue&" to consume the GDB
>>>> prompt right away.  This makes the following pattern matching more
>>>> reliable.
>>>
>>> I don't understand the "more reliable" part, can you explain?
>>>
>>> Simon
>>
>> There will be a "(gdb) " prompt appearing at some point after a "continue", but in non-stop we don't really control when it appears (if I understand correctly).  It seems that other non-stop tests use the "&" variant and consume the prompt immediately.
>>
>> With this, before the next command is issued, we know that 1) we got a prompt and 2) we have seen the process we just released finish.
>>
>> Overall, I am mostly trying to follow patterns I caught in other non-stop tests.
> 
> Ah, I missed that we were in non-stop.
> 
> If we run a single thread at a time, I think it will be reliable to use
> the sync variant of continue.  Using the async variant is useful when
> you resume multiple threads and expect multiple events (like breakpoint
> hits) which can happen in any order.  At least that's my understanding.
> 
> Simon

OK, if you find it is overdoing it, I'll revert this part of the patch 
and submit a V2 shortly.

Thanks,
Lancelot.

[PATCH v2] gdb/testsuite/gdb.rocm: Fix incorrect use of continue N in multi-inferior-gpu.exp

[Lancelot Six]

The gdb.rocm/multi-inferior-gpu.exp testcase uses a "continue $thread"
command, but this is incorrect.  If "continue" is given an argument, it
sets the ignore count of the breakpoint the thread stopped at.

For this testcase it does not really matter since the breakpoint is not
meant to be hit anymore, so whatever the ignore count is won't influence
the outcome of the test.  It is worth fixing nevertheless.

Change-Id: I0eb674d5529cdeb9e808b74870a29b6077265737
---
Changes since V1:
- Following Simon's comments, remove the "continue" -> "continue&"
  change.  Even if this test is in non-stop mode, this change should not
  be required to esure when the GDB prompt is seen.

Best,
Lancelot.

---
 gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp b/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
index 18b4172ff09..5bac50d2ef8 100644
--- a/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
+++ b/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
@@ -68,7 +68,7 @@ proc do_test {} {
 	foreach thread $stopped_gpu_threads {
 	    set infnumber [lindex [split $thread .] 0]
 	    gdb_test "thread $thread" "Switching to thread.*"
-	    gdb_test_multiple "continue $thread" "" {
+	    gdb_test_multiple "continue" "continue inferior $infnumber" {
 		-re "\\\[Inferior $infnumber \[^\n\r\]* exited normally\\]\r\n$::gdb_prompt " {
 		    pass $gdb_test_name
 		}

base-commit: fded0fb898618b5b659762ace776144afa876035
-- 
2.34.1

Re: [PATCH v2] gdb/testsuite/gdb.rocm: Fix incorrect use of continue N in multi-inferior-gpu.exp

[Simon Marchi]

On 10/18/23 06:24, Lancelot Six wrote:
> The gdb.rocm/multi-inferior-gpu.exp testcase uses a "continue $thread"
> command, but this is incorrect.  If "continue" is given an argument, it
> sets the ignore count of the breakpoint the thread stopped at.
> 
> For this testcase it does not really matter since the breakpoint is not
> meant to be hit anymore, so whatever the ignore count is won't influence
> the outcome of the test.  It is worth fixing nevertheless.
> 
> Change-Id: I0eb674d5529cdeb9e808b74870a29b6077265737
> ---
> Changes since V1:
> - Following Simon's comments, remove the "continue" -> "continue&"
>   change.  Even if this test is in non-stop mode, this change should not
>   be required to esure when the GDB prompt is seen.
> 
> Best,
> Lancelot.
> 
> ---
>  gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp b/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
> index 18b4172ff09..5bac50d2ef8 100644
> --- a/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
> +++ b/gdb/testsuite/gdb.rocm/multi-inferior-gpu.exp
> @@ -68,7 +68,7 @@ proc do_test {} {
>  	foreach thread $stopped_gpu_threads {
>  	    set infnumber [lindex [split $thread .] 0]
>  	    gdb_test "thread $thread" "Switching to thread.*"
> -	    gdb_test_multiple "continue $thread" "" {
> +	    gdb_test_multiple "continue" "continue inferior $infnumber" {
>  		-re "\\\[Inferior $infnumber \[^\n\r\]* exited normally\\]\r\n$::gdb_prompt " {
>  		    pass $gdb_test_name
>  		}
> 
> base-commit: fded0fb898618b5b659762ace776144afa876035
> -- 
> 2.34.1
> 

Thanks, LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

Re: [PATCH v2] gdb/testsuite/gdb.rocm: Fix incorrect use of continue N in multi-inferior-gpu.exp

[Lancelot SIX]

> 
> Thanks, LGTM:
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> 
> Simon

Thanks,

I have just pushed this patch.

Best,
Lancelot.