[10/12] gdb_target_is_remote -> gdb_protocol_is_remote
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
This is similar to the previous patch, but for gdb_protocol_is_remote.
gdb_is_target_remote and its MI cousin mi_is_target_remote, use "maint
print target-stack", which is unnecessary when checking whether
gdb_protocol is "remote" or "extended-remote" would do. Checking
gdb_protocol is more efficient, and can be done before starting GDB
and running to main, unlike gdb_is_target_remote/mi_is_target_remote.
This adds a new gdb_protocol_is_remote procedure, and uses it in place
of gdb_is_target_remote/mi_is_target_remote throughout.
There are no uses of gdb_is_target_remote/mi_is_target_remote left
after this. Those will be eliminated in a following patch.
In some spots, we no longer need to defer the check until after
starting GDB, so the patch adjusts accordingly.
Change-Id: I90267c132f942f63426f46dbca0b77dbfdf9d2ef
---
.../gdb.arch/aarch64-sme-core.exp.tcl | 12 ++++-----
.../aarch64-sme-regs-available.exp.tcl | 25 ++++++++++---------
.../aarch64-sme-regs-sigframe.exp.tcl | 25 ++++++++++---------
.../aarch64-sme-regs-unavailable.exp.tcl | 12 ++++-----
gdb/testsuite/gdb.arch/aarch64-sme-sanity.exp | 16 ++++++------
gdb/testsuite/gdb.base/cond-eval-mode.exp | 2 +-
gdb/testsuite/gdb.base/dprintf.exp | 2 +-
.../gdb.base/hbreak-in-shr-unsupported.exp | 6 ++---
gdb/testsuite/gdb.mi/mi-nonstop.exp | 2 +-
gdb/testsuite/gdb.python/py-evsignal.exp | 3 +--
.../gdb.reverse/finish-reverse-next.exp | 1 -
.../gdb.threads/break-while-running.exp | 2 +-
.../main-thread-exit-during-detach.exp | 2 +-
.../process-dies-while-handling-bp.exp | 2 +-
gdb/testsuite/gdb.trace/change-loc.exp | 10 ++++----
gdb/testsuite/gdb.trace/ftrace.exp | 2 +-
gdb/testsuite/gdb.trace/qtro.exp | 11 ++++----
gdb/testsuite/lib/gdb.exp | 11 +++++++-
18 files changed, 76 insertions(+), 70 deletions(-)
Comments
>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
Pedro> This is similar to the previous patch, but for gdb_protocol_is_remote.
Pedro> gdb_is_target_remote and its MI cousin mi_is_target_remote, use "maint
Pedro> print target-stack", which is unnecessary when checking whether
Pedro> gdb_protocol is "remote" or "extended-remote" would do. Checking
Pedro> gdb_protocol is more efficient, and can be done before starting GDB
Pedro> and running to main, unlike gdb_is_target_remote/mi_is_target_remote.
Pedro> This adds a new gdb_protocol_is_remote procedure, and uses it in place
Pedro> of gdb_is_target_remote/mi_is_target_remote throughout.
Pedro> There are no uses of gdb_is_target_remote/mi_is_target_remote left
Pedro> after this. Those will be eliminated in a following patch.
Pedro> In some spots, we no longer need to defer the check until after
Pedro> starting GDB, so the patch adjusts accordingly.
Makes sense to me, I have a question though.
Pedro> +# Check if we are talking to a remote target. If so, bail out, as
Pedro> +# right now remote targets can't communicate vector length (vl or svl)
Pedro> +# changes to gdb via the RSP. When this restriction is lifted, we can
Pedro> +# remove this guard.
Pedro> +if {[gdb_protocol_is_remote]} {
Pedro> + unsupported "aarch64 sve/sme tests not supported for remote targets"
Pedro> + return -1
Pedro> +}
Seems like this spot could just use "require !gdb_protocol_is_remote" --
any reason why you didn't do this?
Tom
On Friday, April 19, 2024 5:14 PM, Pedro Alves wrote:
> Subject: [PATCH 10/12] gdb_target_is_remote -> gdb_protocol_is_remote
Similar to the previous patch, "gdb_target_is_remote" shall be spelled
"gdb_is_target_remote".
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index c072a4502b4..f37d54b16be 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -4700,7 +4700,8 @@ proc gdb_is_target_remote_prompt { prompt_regexp } {
> #
> # This is meant to be used on testcases that connect to targets
> # different from the default board protocol. For most tests, you can
> -# check whether gdb_protocol is "remote" or "extended-remote" instead.
> +# check whether gdb_protocol is "remote" or "extended-remote" instead
> +# (or call gdb_protocol_is_remote for either).
> #
> # NOTE: GDB must be running BEFORE this procedure is called!
>
> @@ -4731,6 +4732,14 @@ proc gdb_protocol_is_native { } {
> return [expr {[target_info gdb_protocol] == ""}]
> }
>
> +# Returns true if gdb_protocol is either "remote" or
> +# "extended-remote".
> +
> +proc gdb_protocol_is_remote { } {
> + return [expr {[target_info gdb_protocol] == "remote"
> + || [target_info gdb_protocol] == "extended-remote"}]
> +}
> +
How about using `eq`, since this is string comparison? Also in the previous
patch in the comparison against "".
Regards
-Baris
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Hi!
On 2024-04-19 19:56, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
>
> Pedro> This is similar to the previous patch, but for gdb_protocol_is_remote.
> Pedro> gdb_is_target_remote and its MI cousin mi_is_target_remote, use "maint
> Pedro> print target-stack", which is unnecessary when checking whether
> Pedro> gdb_protocol is "remote" or "extended-remote" would do. Checking
> Pedro> gdb_protocol is more efficient, and can be done before starting GDB
> Pedro> and running to main, unlike gdb_is_target_remote/mi_is_target_remote.
>
> Pedro> This adds a new gdb_protocol_is_remote procedure, and uses it in place
> Pedro> of gdb_is_target_remote/mi_is_target_remote throughout.
>
> Pedro> There are no uses of gdb_is_target_remote/mi_is_target_remote left
> Pedro> after this. Those will be eliminated in a following patch.
>
> Pedro> In some spots, we no longer need to defer the check until after
> Pedro> starting GDB, so the patch adjusts accordingly.
>
> Makes sense to me, I have a question though.
>
> Pedro> +# Check if we are talking to a remote target. If so, bail out, as
> Pedro> +# right now remote targets can't communicate vector length (vl or svl)
> Pedro> +# changes to gdb via the RSP. When this restriction is lifted, we can
> Pedro> +# remove this guard.
> Pedro> +if {[gdb_protocol_is_remote]} {
> Pedro> + unsupported "aarch64 sve/sme tests not supported for remote targets"
> Pedro> + return -1
> Pedro> +}
>
> Seems like this spot could just use "require !gdb_protocol_is_remote" --
> any reason why you didn't do this?
Really no reason. I've done that now locally, in the three spots I could do it.
I also simplified the comment, as most of it is obvious/redundant:
+# Remote targets can't communicate vector length (vl or svl) changes
+# to GDB via the RSP.
+require !gdb_protocol_is_remote
On 2024-04-22 09:30, Aktemur, Tankut Baris wrote:
> On Friday, April 19, 2024 5:14 PM, Pedro Alves wrote:
>> Subject: [PATCH 10/12] gdb_target_is_remote -> gdb_protocol_is_remote
>
> Similar to the previous patch, "gdb_target_is_remote" shall be spelled
> "gdb_is_target_remote".
Thanks. I fixed that locally.
>
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index c072a4502b4..f37d54b16be 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -4700,7 +4700,8 @@ proc gdb_is_target_remote_prompt { prompt_regexp } {
>> #
>> # This is meant to be used on testcases that connect to targets
>> # different from the default board protocol. For most tests, you can
>> -# check whether gdb_protocol is "remote" or "extended-remote" instead.
>> +# check whether gdb_protocol is "remote" or "extended-remote" instead
>> +# (or call gdb_protocol_is_remote for either).
>> #
>> # NOTE: GDB must be running BEFORE this procedure is called!
>>
>> @@ -4731,6 +4732,14 @@ proc gdb_protocol_is_native { } {
>> return [expr {[target_info gdb_protocol] == ""}]
>> }
>>
>> +# Returns true if gdb_protocol is either "remote" or
>> +# "extended-remote".
>> +
>> +proc gdb_protocol_is_remote { } {
>> + return [expr {[target_info gdb_protocol] == "remote"
>> + || [target_info gdb_protocol] == "extended-remote"}]
>> +}
>> +
>
> How about using `eq`, since this is string comparison? Also in the previous
> patch in the comparison against "".
I just find that == reads a little bit better. There is no risk
that this ends up doing a numerical comparison, which would be the reason to
use string eq. Note that we use == when comparing with gdb_protocol pretty
much everywhere throughout the testsuite.
On Tuesday, April 23, 2024 2:47 PM, Pedro Alves wrote:
> >> +# Returns true if gdb_protocol is either "remote" or
> >> +# "extended-remote".
> >> +
> >> +proc gdb_protocol_is_remote { } {
> >> + return [expr {[target_info gdb_protocol] == "remote"
> >> + || [target_info gdb_protocol] == "extended-remote"}]
> >> +}
> >> +
> >
> > How about using `eq`, since this is string comparison? Also in the previous
> > patch in the comparison against "".
>
> I just find that == reads a little bit better. There is no risk
> that this ends up doing a numerical comparison, which would be the reason to
> use string eq. Note that we use == when comparing with gdb_protocol pretty
> much everywhere throughout the testsuite.
I also think `==` is more readable than `eq`. As a background motivation of my
question, I was actually seeking if there is a coding rule about the use of ==
vs eq in the testsuite. Thanks for clarifying!
-Baris
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
@@ -158,20 +158,20 @@ proc test_sme_core_file { id_start id_end } {
continue
}
- if ![runto_main] {
- untested "could not run to main"
- return -1
- }
-
# Check if we are talking to a remote target. If so, bail out,
# as right now remote targets can't communicate vector length (vl
# or svl) changes to gdb via the RSP. When this restriction is
# lifted, we can remove this guard.
- if {[gdb_is_target_remote]} {
+ if {[gdb_protocol_is_remote]} {
unsupported "aarch64 sve/sme tests not supported for remote targets"
return -1
}
+ if ![runto_main] {
+ untested "could not run to main"
+ return -1
+ }
+
generate_sme_core_files ${executable} ${binfile} $id $state $vl $svl
}
}
@@ -18,6 +18,19 @@
load_lib aarch64-scalable.exp
+require is_aarch64_target
+require allow_aarch64_sve_tests
+require allow_aarch64_sme_tests
+
+# Check if we are talking to a remote target. If so, bail out, as
+# right now remote targets can't communicate vector length (vl or svl)
+# changes to gdb via the RSP. When this restriction is lifted, we can
+# remove this guard.
+if {[gdb_protocol_is_remote]} {
+ unsupported "aarch64 sve/sme tests not supported for remote targets"
+ return -1
+}
+
#
# Cycle through all ZA registers and pseudo-registers and validate that their
# contents are available for vector length SVL.
@@ -160,14 +173,6 @@ proc test_sme_registers_available { id_start id_end } {
return -1
}
- # Check if we are talking to a remote target. If so, bail out, as right now
- # remote targets can't communicate vector length (vl or svl) changes to gdb
- # via the RSP. When this restriction is lifted, we can remove this guard.
- if {[gdb_is_target_remote]} {
- unsupported "aarch64 sve/sme tests not supported for remote targets"
- return -1
- }
-
gdb_test_no_output "set print repeats 1"
set prctl_breakpoint "stop 1"
@@ -255,8 +260,4 @@ proc test_sme_registers_available { id_start id_end } {
}
}
-require is_aarch64_target
-require allow_aarch64_sve_tests
-require allow_aarch64_sme_tests
-
test_sme_registers_available $id_start $id_end
@@ -17,6 +17,19 @@
load_lib aarch64-scalable.exp
+require is_aarch64_target
+require allow_aarch64_sve_tests
+require allow_aarch64_sme_tests
+
+# Check if we are talking to a remote target. If so, bail out, as
+# right now remote targets can't communicate vector length (vl or svl)
+# changes to gdb via the RSP. When this restriction is lifted, we can
+# remove this guard.
+if {[gdb_protocol_is_remote]} {
+ unsupported "aarch64 sve/sme tests not supported for remote targets"
+ return -1
+}
+
#
# Validate the state of registers in the signal frame for various states.
#
@@ -39,14 +52,6 @@ proc test_sme_registers_sigframe { id_start id_end } {
return -1
}
- # Check if we are talking to a remote target. If so, bail out, as right now
- # remote targets can't communicate vector length (vl or svl) changes to gdb
- # via the RSP. When this restriction is lifted, we can remove this guard.
- if {[gdb_is_target_remote]} {
- unsupported "aarch64 sve/sme tests not supported for remote targets"
- return -1
- }
-
set sigill_breakpoint "stop before SIGILL"
set handler_breakpoint "handler"
gdb_breakpoint [gdb_get_line_number $sigill_breakpoint]
@@ -183,8 +188,4 @@ proc test_sme_registers_sigframe { id_start id_end } {
}
}
-require is_aarch64_target
-require allow_aarch64_sve_tests
-require allow_aarch64_sme_tests
-
test_sme_registers_sigframe $id_start $id_end
@@ -120,19 +120,19 @@ proc test_sme_registers_unavailable { id_start id_end } {
}
set binfile [standard_output_file ${executable}]
- if ![runto_main] {
- untested "could not run to main"
- return -1
- }
-
# Check if we are talking to a remote target. If so, bail out, as right now
# remote targets can't communicate vector length (vl or svl) changes to gdb
# via the RSP. When this restriction is lifted, we can remove this guard.
- if {[gdb_is_target_remote]} {
+ if {[gdb_protocol_is_remote]} {
unsupported "aarch64 sve/sme tests not supported for remote targets"
return -1
}
+ if ![runto_main] {
+ untested "could not run to main"
+ return -1
+ }
+
gdb_test_no_output "set print repeats 1"
set prctl_breakpoint "stop 1"
@@ -40,6 +40,14 @@ require is_aarch64_target
require allow_aarch64_sve_tests
require allow_aarch64_sme_tests
+# Check if we are talking to a remote target. If so, bail out, as right now
+# remote targets can't communicate vector length (vl or svl) changes to gdb
+# via the RSP. When this restriction is lifted, we can remove this guard.
+if {[gdb_protocol_is_remote]} {
+ unsupported "aarch64 sve/sme tests not supported for remote targets"
+ return -1
+}
+
set compile_flags {"debug" "macros" "additional_flags=-march=armv8.5-a+sve"}
standard_testfile
if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile} ${compile_flags}]} {
@@ -50,14 +58,6 @@ if {![runto_main]} {
return -1
}
-# Check if we are talking to a remote target. If so, bail out, as right now
-# remote targets can't communicate vector length (vl or svl) changes to gdb
-# via the RSP. When this restriction is lifted, we can remove this guard.
-if {[gdb_is_target_remote]} {
- unsupported "aarch64 sve/sme tests not supported for remote targets"
- return -1
-}
-
# Adjust the repeat count for the test.
gdb_test_no_output "set print repeats 1" "adjust repeat count"
@@ -58,7 +58,7 @@ gdb_test_multiple $test_target $test_target {
# We now know that the target supports target-side conditional
# evaluation. Now make sure we can force-disable the
# ConditionalBreakpoints RSP feature.
-if [gdb_is_target_remote] {
+if [gdb_protocol_is_remote] {
gdb_test \
"set remote conditional-breakpoints-packet off" \
"Support for the 'ConditionalBreakpoints' packet on the current remote target is set to \"off\"."
@@ -217,7 +217,7 @@ gdb_test "set dprintf-style foobar" "Undefined item: \"foobar\"." \
# Test that force-disabling the BreakpointCommands RSP feature works
# as expected. dprintf relies on support for target-side breakpoint
# commands --- use it as proxy.
-if [gdb_is_target_remote] {
+if [gdb_protocol_is_remote] {
gdb_test \
"set remote breakpoint-commands-packet off" \
"Support for the 'BreakpointCommands' packet on the current remote target is set to \"off\"."
@@ -41,8 +41,6 @@ if {![runto_main]} {
return -1
}
-set is_target_remote [gdb_is_target_remote]
-
# Get main breakpoint out of the way.
delete_breakpoints
@@ -51,7 +49,7 @@ gdb_test_no_output "set breakpoint always-inserted on"
# Force-disable Z1 packets, in case the target actually supports
# these.
-if {$is_target_remote} {
+if {[gdb_protocol_is_remote]} {
gdb_test \
"set remote Z-packet off" \
"Use of Z packets on the current remote target is set to \"off\"."
@@ -79,7 +77,7 @@ gdb_test_multiple "hbreak -q main" $test {
}
-re "Hardware assisted breakpoint.*at.* file .*$srcfile, line.*$gdb_prompt $" {
set supports_hbreak 1
- if {$is_target_remote} {
+ if {[gdb_protocol_is_remote]} {
# Z-packets have been force-disabled, so this shouldn't
# happen.
fail $test
@@ -126,7 +126,7 @@ mi_gdb_test "-thread-select 2" "\\^done.*" "select first worker thread"
mi_gdb_test "-gdb-set --thread 3 variable exit_first_thread=1" ".*\\^done" "ask the second thread to exit"
set test "wait for thread exit"
-if { [mi_is_target_remote] } {
+if { [gdb_protocol_is_remote] } {
# The remote protocol doesn't have support for thread exit
# notifications.
unsupported $test
@@ -13,8 +13,7 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
-if {[target_info gdb_protocol] == "remote"
- || [target_info gdb_protocol] == "extended-remote"} {
+if {[gdb_protocol_is_remote]} {
# RuntimeError: Could not find event thread
kfail "python/12966" "Signal Thread 3"
return -1
@@ -52,7 +52,6 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
}
runto_main
-set target_remote [gdb_is_target_remote]
if [supports_process_record] {
# Activate process record/replay.
@@ -53,7 +53,7 @@ proc test { update_thread_list always_inserted non_stop } {
# RSP, we can't issue commands until the target replies to vCont.
# Not an issue with the non-stop RSP variant, which has a
# non-blocking vCont.
- if {$non_stop=="off" && [gdb_is_target_remote]} {
+ if {$non_stop=="off" && [gdb_protocol_is_remote]} {
return -1
}
@@ -120,7 +120,7 @@ proc run_test { spawn_inferior } {
# In both cases the stop arrives while GDB is processing the
# detach, however, for remote targets GDB doesn't report the stop,
# while for local targets GDB does report the stop.
- if {![gdb_is_target_remote]} {
+ if {![gdb_protocol_is_remote]} {
set stop_re "\\\[Thread.*exited\\\]\r\n"
} else {
set stop_re ""
@@ -52,7 +52,7 @@ proc do_test { non_stop cond_bp_target } {
# Whether it's known that the test fails.
set should_kfail 0
- if {![gdb_is_target_remote]} {
+ if {![gdb_protocol_is_remote]} {
set should_kfail 1
} else {
if {!$cond_bp_target} {
@@ -288,16 +288,16 @@ proc tracepoint_install_in_trace_disabled { trace_type } {
global pcreg
global gdb_prompt
+ # This test only makes sense with remote targets.
+ if ![gdb_protocol_is_remote] {
+ return
+ }
+
clean_restart ${testfile}
if ![runto_main] {
return -1
}
- # This test only makes sense with the remote target.
- if ![gdb_is_target_remote] {
- return
- }
-
gdb_test_no_output "delete break 1"
# Set a tracepoint we'll never meet. Just to avoid the
@@ -189,7 +189,7 @@ proc test_fast_tracepoints {} {
# fast tracepoints RSP feature, and confirm fast tracepoints
# can no longer be downloaded.
set test "fast tracepoint could not be downloaded with the feature disabled"
- if [gdb_is_target_remote] {
+ if [gdb_protocol_is_remote] {
gdb_test "set remote fast-tracepoints-packet off"
gdb_test_multiple "tstart" $test {
@@ -20,6 +20,10 @@
load_lib trace-support.exp
+# Check whether we're testing with the remote or extended-remote
+# targets.
+require gdb_protocol_is_remote
+
standard_testfile
if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug nopie}]} {
@@ -30,12 +34,7 @@ if ![runto_main] {
return -1
}
-# Check whether we're testing with the remote or extended-remote
-# targets, and whether the target supports tracepoints.
-
-if ![gdb_is_target_remote] {
- return -1
-}
+# Check whether the target supports tracepoints.
if ![gdb_target_supports_trace] {
unsupported "current target does not support trace"
@@ -4700,7 +4700,8 @@ proc gdb_is_target_remote_prompt { prompt_regexp } {
#
# This is meant to be used on testcases that connect to targets
# different from the default board protocol. For most tests, you can
-# check whether gdb_protocol is "remote" or "extended-remote" instead.
+# check whether gdb_protocol is "remote" or "extended-remote" instead
+# (or call gdb_protocol_is_remote for either).
#
# NOTE: GDB must be running BEFORE this procedure is called!
@@ -4731,6 +4732,14 @@ proc gdb_protocol_is_native { } {
return [expr {[target_info gdb_protocol] == ""}]
}
+# Returns true if gdb_protocol is either "remote" or
+# "extended-remote".
+
+proc gdb_protocol_is_remote { } {
+ return [expr {[target_info gdb_protocol] == "remote"
+ || [target_info gdb_protocol] == "extended-remote"}]
+}
+
# Like istarget, but checks a list of targets.
proc is_any_target {args} {
foreach targ $args {