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

[Pedro Alves <pedro@palves.net>]

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.
>  #
>