[6/8] gdb/testsuite: introduce is_target_non_stop helper proc

  I noticed that several test included copy & pasted code to run the
'maint show target-non-stop' command, and then switch based on the
result.

In this commit I factor this code out into a helper proc in
lib/gdb.exp, and update all the places I could find that used this
pattern to make use of the helper proc.

There should be no change in what is tested after this commit.
---
 gdb/testsuite/gdb.base/access-mem-running.exp | 10 +++-------
 gdb/testsuite/gdb.base/fork-running-state.exp | 11 +++--------
 .../access-mem-running-thread-exit.exp        | 10 +++-------
 .../gdb.threads/clone-attach-detach.exp       | 11 +++--------
 .../gdb.threads/detach-step-over.exp          |  8 ++------
 gdb/testsuite/lib/gdb.exp                     | 19 +++++++++++++++++++
 6 files changed, 33 insertions(+), 36 deletions(-)
  

On 2023-02-20 2:13 p.m., Andrew Burgess via Gdb-patches wrote:
> I noticed that several test included copy & pasted code to run the

"several test" -> "several tests"

> 'maint show target-non-stop' command, and then switch based on the
> result.
> 
> In this commit I factor this code out into a helper proc in
> lib/gdb.exp, and update all the places I could find that used this
> pattern to make use of the helper proc.
> 
> There should be no change in what is tested after this commit.
> ---
>  gdb/testsuite/gdb.base/access-mem-running.exp | 10 +++-------
>  gdb/testsuite/gdb.base/fork-running-state.exp | 11 +++--------
>  .../access-mem-running-thread-exit.exp        | 10 +++-------
>  .../gdb.threads/clone-attach-detach.exp       | 11 +++--------
>  .../gdb.threads/detach-step-over.exp          |  8 ++------
>  gdb/testsuite/lib/gdb.exp                     | 19 +++++++++++++++++++
>  6 files changed, 33 insertions(+), 36 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.base/access-mem-running.exp b/gdb/testsuite/gdb.base/access-mem-running.exp
> index 90ddedc470d..d0f7871fc0f 100644
> --- a/gdb/testsuite/gdb.base/access-mem-running.exp
> +++ b/gdb/testsuite/gdb.base/access-mem-running.exp
> @@ -46,13 +46,9 @@ proc test { non_stop } {
>  	&& ([target_info gdb_protocol] == "remote"
>  	    || [target_info gdb_protocol] == "extended-remote")} {
>  
> -	gdb_test_multiple "maint show target-non-stop" "" {
> -	    -wrap -re "(is|currently) on.*" {
> -	    }
> -	    -wrap -re "(is|currently) off.*" {
> -		unsupported "can't issue commands while target is running"
> -		return 0
> -	    }
> +	if {![is_target_non_stop]} {
> +	    unsupported "can't issue commands while target is running"
> +	    return 0
>  	}
>      }
>  
> diff --git a/gdb/testsuite/gdb.base/fork-running-state.exp b/gdb/testsuite/gdb.base/fork-running-state.exp
> index b8045ad490f..9a18193c4dc 100644
> --- a/gdb/testsuite/gdb.base/fork-running-state.exp
> +++ b/gdb/testsuite/gdb.base/fork-running-state.exp
> @@ -46,14 +46,9 @@ proc do_test { detach_on_fork follow_fork non_stop schedule_multiple } {
>  	 && ([target_info gdb_protocol] == "remote"
>  	     || [target_info gdb_protocol] == "extended-remote")} {
>  
> -	set test "maint show target-non-stop"
> -	gdb_test_multiple "maint show target-non-stop" $test {
> -	    -re "(is|currently) on.*$gdb_prompt $" {
> -	    }
> -	    -re "(is|currently) off.*$gdb_prompt $" {
> -		unsupported "can't issue info threads while target is running"
> -		return 0
> -	    }
> +	if {![is_target_non_stop]} {
> +	    unsupported "can't issue info threads while target is running"
> +	    return 0
>  	}
>      }
>  
> diff --git a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
> index 09b05480c04..2d3ace44ccd 100644
> --- a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
> +++ b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
> @@ -65,13 +65,9 @@ proc test { non_stop } {
>  	&& ([target_info gdb_protocol] == "remote"
>  	    || [target_info gdb_protocol] == "extended-remote")} {
>  
> -	gdb_test_multiple "maint show target-non-stop" "" {
> -	    -wrap -re "(is|currently) on.*" {
> -	    }
> -	    -wrap -re "(is|currently) off.*" {
> -		unsupported "can't issue commands while target is running"
> -		return 0
> -	    }
> +	if {![is_target_non_stop]} {
> +	    unsupported "can't issue commands while target is running"
> +	    return 0
>  	}
>      }
>  
> diff --git a/gdb/testsuite/gdb.threads/clone-attach-detach.exp b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
> index ac9b92d5f57..a71713ea8ba 100644
> --- a/gdb/testsuite/gdb.threads/clone-attach-detach.exp
> +++ b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
> @@ -64,14 +64,9 @@ if {[target_info exists gdb_protocol]
>      && ([target_info gdb_protocol] == "remote"
>  	|| [target_info gdb_protocol] == "extended-remote")} {
>  
> -    set test "maint show target-non-stop"
> -    gdb_test_multiple "maint show target-non-stop" $test {
> -	-re "(is|currently) on.*$gdb_prompt $" {
> -	}
> -	-re "(is|currently) off.*$gdb_prompt $" {
> -	    unsupported "bg attach: can't issue info threads while target is running"
> -	    return 0
> -	}
> +    if {![is_target_non_stop]} {
> +	unsupported "bg attach: can't issue info threads while target is running"
> +	return 0
>      }
>  }
>  
> diff --git a/gdb/testsuite/gdb.threads/detach-step-over.exp b/gdb/testsuite/gdb.threads/detach-step-over.exp
> index ed9dc1aab88..bf5ef6b06a1 100644
> --- a/gdb/testsuite/gdb.threads/detach-step-over.exp
> +++ b/gdb/testsuite/gdb.threads/detach-step-over.exp
> @@ -314,12 +314,8 @@ proc_with_prefix test_detach_quit {condition_eval target_non_stop \
>  	start_gdb_for_test $condition_eval $target_non_stop \
>  	    $non_stop $displaced
>  
> -	gdb_test_multiple "maint show target-non-stop" "" {
> -	    -wrap -re "(is|currently) on.*" {
> -	    }
> -	    -wrap -re "(is|currently) off.*" {
> -		return
> -	    }
> +	if {![is_target_non_stop]} {
> +	    return
>  	}
>      }
>  
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index e48228ed4f6..ef62174301e 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -9303,6 +9303,25 @@ proc gdb_step_until { regexp {test_name ""} {max_steps 10} } {
>      }
>  }
>  
> +# Return true if the current target is operating in non-stop mode,
> +# otherwise, return false.
> +#
> +# The inferior will need to have started running in order to get the
> +# correct result.
> +
> +proc is_target_non_stop { {testname ""} } {
> +    set is_non_stop false
> +    gdb_test_multiple "maint show target-non-stop" $testname {
> +	-wrap -re "(is|currently) on.*" {
> +	    set is_non_stop true
> +	}
> +	-wrap -re "(is|currently) off.*" {
> +	    set is_non_stop false
> +	}
> +    }
> +    return $is_non_stop
> +}

The original code defaulted to "true", while this defaults to "false".

I.e., a plain refactor that doesn't change behavior would look like this:

proc is_target_non_stop { {testname ""} } {
    gdb_test_multiple "maint show target-non-stop" $testname {
	-wrap -re "(is|currently) on.*" {
	}
	-wrap -re "(is|currently) off.*" {
	    return false
	}
    }
    return true
}

The original code was written that way so that we'd only skip
the remainder of the tests if we're absolutely sure they
wouldn't work.  If the maint command failed for some reason, we
don't know that, so we continue with the testcase.

I think that for a refactoring patch, that detail shouldn't be
changed.

Pedro Alves

> +
>  # Check if the compiler emits epilogue information associated
>  # with the closing brace or with the last statement line.
>  #
>
  

Pedro Alves <pedro@palves.net> writes:

> On 2023-02-20 2:13 p.m., Andrew Burgess via Gdb-patches wrote:
>> I noticed that several test included copy & pasted code to run the
>
> "several test" -> "several tests"
>

Thanks, fixed.

>> 'maint show target-non-stop' command, and then switch based on the
>> result.
>> 
>> In this commit I factor this code out into a helper proc in
>> lib/gdb.exp, and update all the places I could find that used this
>> pattern to make use of the helper proc.
>> 
>> There should be no change in what is tested after this commit.
>> ---
>>  gdb/testsuite/gdb.base/access-mem-running.exp | 10 +++-------
>>  gdb/testsuite/gdb.base/fork-running-state.exp | 11 +++--------
>>  .../access-mem-running-thread-exit.exp        | 10 +++-------
>>  .../gdb.threads/clone-attach-detach.exp       | 11 +++--------
>>  .../gdb.threads/detach-step-over.exp          |  8 ++------
>>  gdb/testsuite/lib/gdb.exp                     | 19 +++++++++++++++++++
>>  6 files changed, 33 insertions(+), 36 deletions(-)
>> 
>> diff --git a/gdb/testsuite/gdb.base/access-mem-running.exp b/gdb/testsuite/gdb.base/access-mem-running.exp
>> index 90ddedc470d..d0f7871fc0f 100644
>> --- a/gdb/testsuite/gdb.base/access-mem-running.exp
>> +++ b/gdb/testsuite/gdb.base/access-mem-running.exp
>> @@ -46,13 +46,9 @@ proc test { non_stop } {
>>  	&& ([target_info gdb_protocol] == "remote"
>>  	    || [target_info gdb_protocol] == "extended-remote")} {
>>  
>> -	gdb_test_multiple "maint show target-non-stop" "" {
>> -	    -wrap -re "(is|currently) on.*" {
>> -	    }
>> -	    -wrap -re "(is|currently) off.*" {
>> -		unsupported "can't issue commands while target is running"
>> -		return 0
>> -	    }
>> +	if {![is_target_non_stop]} {
>> +	    unsupported "can't issue commands while target is running"
>> +	    return 0
>>  	}
>>      }
>>  
>> diff --git a/gdb/testsuite/gdb.base/fork-running-state.exp b/gdb/testsuite/gdb.base/fork-running-state.exp
>> index b8045ad490f..9a18193c4dc 100644
>> --- a/gdb/testsuite/gdb.base/fork-running-state.exp
>> +++ b/gdb/testsuite/gdb.base/fork-running-state.exp
>> @@ -46,14 +46,9 @@ proc do_test { detach_on_fork follow_fork non_stop schedule_multiple } {
>>  	 && ([target_info gdb_protocol] == "remote"
>>  	     || [target_info gdb_protocol] == "extended-remote")} {
>>  
>> -	set test "maint show target-non-stop"
>> -	gdb_test_multiple "maint show target-non-stop" $test {
>> -	    -re "(is|currently) on.*$gdb_prompt $" {
>> -	    }
>> -	    -re "(is|currently) off.*$gdb_prompt $" {
>> -		unsupported "can't issue info threads while target is running"
>> -		return 0
>> -	    }
>> +	if {![is_target_non_stop]} {
>> +	    unsupported "can't issue info threads while target is running"
>> +	    return 0
>>  	}
>>      }
>>  
>> diff --git a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
>> index 09b05480c04..2d3ace44ccd 100644
>> --- a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
>> +++ b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
>> @@ -65,13 +65,9 @@ proc test { non_stop } {
>>  	&& ([target_info gdb_protocol] == "remote"
>>  	    || [target_info gdb_protocol] == "extended-remote")} {
>>  
>> -	gdb_test_multiple "maint show target-non-stop" "" {
>> -	    -wrap -re "(is|currently) on.*" {
>> -	    }
>> -	    -wrap -re "(is|currently) off.*" {
>> -		unsupported "can't issue commands while target is running"
>> -		return 0
>> -	    }
>> +	if {![is_target_non_stop]} {
>> +	    unsupported "can't issue commands while target is running"
>> +	    return 0
>>  	}
>>      }
>>  
>> diff --git a/gdb/testsuite/gdb.threads/clone-attach-detach.exp b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
>> index ac9b92d5f57..a71713ea8ba 100644
>> --- a/gdb/testsuite/gdb.threads/clone-attach-detach.exp
>> +++ b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
>> @@ -64,14 +64,9 @@ if {[target_info exists gdb_protocol]
>>      && ([target_info gdb_protocol] == "remote"
>>  	|| [target_info gdb_protocol] == "extended-remote")} {
>>  
>> -    set test "maint show target-non-stop"
>> -    gdb_test_multiple "maint show target-non-stop" $test {
>> -	-re "(is|currently) on.*$gdb_prompt $" {
>> -	}
>> -	-re "(is|currently) off.*$gdb_prompt $" {
>> -	    unsupported "bg attach: can't issue info threads while target is running"
>> -	    return 0
>> -	}
>> +    if {![is_target_non_stop]} {
>> +	unsupported "bg attach: can't issue info threads while target is running"
>> +	return 0
>>      }
>>  }
>>  
>> diff --git a/gdb/testsuite/gdb.threads/detach-step-over.exp b/gdb/testsuite/gdb.threads/detach-step-over.exp
>> index ed9dc1aab88..bf5ef6b06a1 100644
>> --- a/gdb/testsuite/gdb.threads/detach-step-over.exp
>> +++ b/gdb/testsuite/gdb.threads/detach-step-over.exp
>> @@ -314,12 +314,8 @@ proc_with_prefix test_detach_quit {condition_eval target_non_stop \
>>  	start_gdb_for_test $condition_eval $target_non_stop \
>>  	    $non_stop $displaced
>>  
>> -	gdb_test_multiple "maint show target-non-stop" "" {
>> -	    -wrap -re "(is|currently) on.*" {
>> -	    }
>> -	    -wrap -re "(is|currently) off.*" {
>> -		return
>> -	    }
>> +	if {![is_target_non_stop]} {
>> +	    return
>>  	}
>>      }
>>  
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index e48228ed4f6..ef62174301e 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -9303,6 +9303,25 @@ proc gdb_step_until { regexp {test_name ""} {max_steps 10} } {
>>      }
>>  }
>>  
>> +# Return true if the current target is operating in non-stop mode,
>> +# otherwise, return false.
>> +#
>> +# The inferior will need to have started running in order to get the
>> +# correct result.
>> +
>> +proc is_target_non_stop { {testname ""} } {
>> +    set is_non_stop false
>> +    gdb_test_multiple "maint show target-non-stop" $testname {
>> +	-wrap -re "(is|currently) on.*" {
>> +	    set is_non_stop true
>> +	}
>> +	-wrap -re "(is|currently) off.*" {
>> +	    set is_non_stop false
>> +	}
>> +    }
>> +    return $is_non_stop
>> +}
>
> The original code defaulted to "true", while this defaults to "false".
>
> I.e., a plain refactor that doesn't change behavior would look like this:
>
> proc is_target_non_stop { {testname ""} } {
>     gdb_test_multiple "maint show target-non-stop" $testname {
> 	-wrap -re "(is|currently) on.*" {
> 	}
> 	-wrap -re "(is|currently) off.*" {
> 	    return false
> 	}
>     }
>     return true
> }
>
> The original code was written that way so that we'd only skip
> the remainder of the tests if we're absolutely sure they
> wouldn't work.  If the maint command failed for some reason, we
> don't know that, so we continue with the testcase.
>
> I think that for a refactoring patch, that detail shouldn't be
> changed.

Thanks for the feedback.  How's the patch below?  The only change is in
testsuite/lib/gdb.exp, everything else is the same as before.

Thanks,
Andrew

---

commit 40b895a3386c8472e54bc8bb5c3eae01d7123b9a
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sat Feb 18 20:52:40 2023 +0000

    gdb/testsuite: introduce is_target_non_stop helper proc
    
    I noticed that several tests included copy & pasted code to run the
    'maint show target-non-stop' command, and then switch based on the
    result.
    
    In this commit I factor this code out into a helper proc in
    lib/gdb.exp, and update all the places I could find that used this
    pattern to make use of the helper proc.
    
    There should be no change in what is tested after this commit.

diff --git a/gdb/testsuite/gdb.base/access-mem-running.exp b/gdb/testsuite/gdb.base/access-mem-running.exp
index 90ddedc470d..d0f7871fc0f 100644
--- a/gdb/testsuite/gdb.base/access-mem-running.exp
+++ b/gdb/testsuite/gdb.base/access-mem-running.exp
@@ -46,13 +46,9 @@ proc test { non_stop } {
 	&& ([target_info gdb_protocol] == "remote"
 	    || [target_info gdb_protocol] == "extended-remote")} {
 
-	gdb_test_multiple "maint show target-non-stop" "" {
-	    -wrap -re "(is|currently) on.*" {
-	    }
-	    -wrap -re "(is|currently) off.*" {
-		unsupported "can't issue commands while target is running"
-		return 0
-	    }
+	if {![is_target_non_stop]} {
+	    unsupported "can't issue commands while target is running"
+	    return 0
 	}
     }
 
diff --git a/gdb/testsuite/gdb.base/fork-running-state.exp b/gdb/testsuite/gdb.base/fork-running-state.exp
index b8045ad490f..9a18193c4dc 100644
--- a/gdb/testsuite/gdb.base/fork-running-state.exp
+++ b/gdb/testsuite/gdb.base/fork-running-state.exp
@@ -46,14 +46,9 @@ proc do_test { detach_on_fork follow_fork non_stop schedule_multiple } {
 	 && ([target_info gdb_protocol] == "remote"
 	     || [target_info gdb_protocol] == "extended-remote")} {
 
-	set test "maint show target-non-stop"
-	gdb_test_multiple "maint show target-non-stop" $test {
-	    -re "(is|currently) on.*$gdb_prompt $" {
-	    }
-	    -re "(is|currently) off.*$gdb_prompt $" {
-		unsupported "can't issue info threads while target is running"
-		return 0
-	    }
+	if {![is_target_non_stop]} {
+	    unsupported "can't issue info threads while target is running"
+	    return 0
 	}
     }
 
diff --git a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
index 09b05480c04..2d3ace44ccd 100644
--- a/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
+++ b/gdb/testsuite/gdb.threads/access-mem-running-thread-exit.exp
@@ -65,13 +65,9 @@ proc test { non_stop } {
 	&& ([target_info gdb_protocol] == "remote"
 	    || [target_info gdb_protocol] == "extended-remote")} {
 
-	gdb_test_multiple "maint show target-non-stop" "" {
-	    -wrap -re "(is|currently) on.*" {
-	    }
-	    -wrap -re "(is|currently) off.*" {
-		unsupported "can't issue commands while target is running"
-		return 0
-	    }
+	if {![is_target_non_stop]} {
+	    unsupported "can't issue commands while target is running"
+	    return 0
 	}
     }
 
diff --git a/gdb/testsuite/gdb.threads/clone-attach-detach.exp b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
index ac9b92d5f57..a71713ea8ba 100644
--- a/gdb/testsuite/gdb.threads/clone-attach-detach.exp
+++ b/gdb/testsuite/gdb.threads/clone-attach-detach.exp
@@ -64,14 +64,9 @@ if {[target_info exists gdb_protocol]
     && ([target_info gdb_protocol] == "remote"
 	|| [target_info gdb_protocol] == "extended-remote")} {
 
-    set test "maint show target-non-stop"
-    gdb_test_multiple "maint show target-non-stop" $test {
-	-re "(is|currently) on.*$gdb_prompt $" {
-	}
-	-re "(is|currently) off.*$gdb_prompt $" {
-	    unsupported "bg attach: can't issue info threads while target is running"
-	    return 0
-	}
+    if {![is_target_non_stop]} {
+	unsupported "bg attach: can't issue info threads while target is running"
+	return 0
     }
 }
 
diff --git a/gdb/testsuite/gdb.threads/detach-step-over.exp b/gdb/testsuite/gdb.threads/detach-step-over.exp
index ed9dc1aab88..bf5ef6b06a1 100644
--- a/gdb/testsuite/gdb.threads/detach-step-over.exp
+++ b/gdb/testsuite/gdb.threads/detach-step-over.exp
@@ -314,12 +314,8 @@ proc_with_prefix test_detach_quit {condition_eval target_non_stop \
 	start_gdb_for_test $condition_eval $target_non_stop \
 	    $non_stop $displaced
 
-	gdb_test_multiple "maint show target-non-stop" "" {
-	    -wrap -re "(is|currently) on.*" {
-	    }
-	    -wrap -re "(is|currently) off.*" {
-		return
-	    }
+	if {![is_target_non_stop]} {
+	    return
 	}
     }
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 669a5f606d6..19c782bea46 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -9298,6 +9298,28 @@ proc gdb_step_until { regexp {test_name ""} {max_steps 10} } {
     }
 }
 
+# Return false if the current target is not operating in non-stop
+# mode, otherwise, return true.
+#
+# The inferior will need to have started running in order to get the
+# correct result.
+
+proc is_target_non_stop { {testname ""} } {
+    # For historical reasons we assume non-stop mode is on.  If the
+    # maintenance command fails for any reason then we're going to
+    # return true.
+    set is_non_stop true
+    gdb_test_multiple "maint show target-non-stop" $testname {
+	-wrap -re "(is|currently) on.*" {
+	    set is_non_stop true
+	}
+	-wrap -re "(is|currently) off.*" {
+	    set is_non_stop false
+	}
+    }
+    return $is_non_stop
+}
+
 # Check if the compiler emits epilogue information associated
 # with the closing brace or with the last statement line.
 #
  

On 2023-02-27 2:58 p.m., Andrew Burgess wrote:

>> The original code was written that way so that we'd only skip
>> the remainder of the tests if we're absolutely sure they
>> wouldn't work.  If the maint command failed for some reason, we
>> don't know that, so we continue with the testcase.
>>
>> I think that for a refactoring patch, that detail shouldn't be
>> changed.
> 
> Thanks for the feedback.  How's the patch below?  The only change is in
> testsuite/lib/gdb.exp, everything else is the same as before.

LGTM.