| Message ID | 20240507022249.554831-4-thiago.bauermann@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for AArch64 MOPS instructions | expand |
On 2024-05-07 03:22, Thiago Jung Bauermann wrote: > +# Run a test on the target to see if it supports Aarch64 MOPS (Memory > +# Operations) extensions. Return 0 if so, 1 if it does not. This "Return 0 if so, 1 if it does not." comment seems backwards. > Note this causes > +# a restart of GDB. Might want to apply a similar logic to can_spawn_for_attach, wrt gdb_exit. See the tail end of that function. Maybe we can factor that out and reuse it. > + > +gdb_caching_proc allow_aarch64_mops_tests {} {
Hello Pedro, Thank you for your review! Pedro Alves <pedro@palves.net> writes: > On 2024-05-07 03:22, Thiago Jung Bauermann wrote: >> +# Run a test on the target to see if it supports Aarch64 MOPS (Memory >> +# Operations) extensions. Return 0 if so, 1 if it does not. > > This > > "Return 0 if so, 1 if it does not." > > comment seems backwards. Yes, indeed. Fixed the thinko for v4. >> Note this causes >> +# a restart of GDB. > > Might want to apply a similar logic to can_spawn_for_attach, wrt gdb_exit. > See the tail end of that function. Maybe we can factor that out and > reuse it. I looked into it, but I wasn't able to understand what is the problem that gdb_exit causes. Does it mean that any require predicate that restarts GDB needs to use that two-tier caching scheme? Many of the predicates that test for hardware features restart GDB, so this would be a refactor affecting many predicates. I don't mind doing it but I'd suggest doing so as a separate patch. It would also be useful to have a reproducer of the problem that the double-caching fixes to check that I'm effectively solving the problem. >> + >> +gdb_caching_proc allow_aarch64_mops_tests {} {
On 5/21/24 23:03, Thiago Jung Bauermann wrote: > Hello Pedro, > > Thank you for your review! > > Pedro Alves <pedro@palves.net> writes: > >> On 2024-05-07 03:22, Thiago Jung Bauermann wrote: >>> +# Run a test on the target to see if it supports Aarch64 MOPS (Memory >>> +# Operations) extensions. Return 0 if so, 1 if it does not. >> >> This >> >> "Return 0 if so, 1 if it does not." >> >> comment seems backwards. > > Yes, indeed. Fixed the thinko for v4. > >>> Note this causes >>> +# a restart of GDB. >> >> Might want to apply a similar logic to can_spawn_for_attach, wrt gdb_exit. >> See the tail end of that function. Maybe we can factor that out and >> reuse it. > > I looked into it, but I wasn't able to understand what is the problem > that gdb_exit causes. Does it mean that any require predicate that > restarts GDB needs to use that two-tier caching scheme? > Let me try to explain the idea. Consider two demonstrator test-cases: ... diff --git a/gdb/testsuite/gdb.testsuite/example-2.exp b/gdb/testsuite/gdb.testsuite/example-2.exp new file mode 100644 index 00000000000..0efa36e8cec --- /dev/null +++ b/gdb/testsuite/gdb.testsuite/example-2.exp @@ -0,0 +1,8 @@ +if { [foo] } { + verbose -log "foo!" +} + +clean_restart + +gdb_test "print 1" + diff --git a/gdb/testsuite/gdb.testsuite/example.exp b/gdb/testsuite/gdb.testsuite/example.exp new file mode 100644 index 00000000000..2e463a911b6 --- /dev/null +++ b/gdb/testsuite/gdb.testsuite/example.exp @@ -0,0 +1,8 @@ +clean_restart + +if { [foo] } { + verbose -log "foo!" +} + +gdb_test "print 1" + diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index 87e6ee050f2..d176a584a21 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -10618,5 +10618,10 @@ gdb_caching_proc root_user {} { return [expr $uid == 0] } +gdb_caching_proc foo {} { + gdb_exit + return 1 +} + # Always load compatibility stuff. load_lib future.exp ... This runs fine like this: ... $ ( cd build/gdb/testsuite/; make check TESTS="gdb.testsuite/example*.exp" ) ... But after swapping the two test-cases: ... $ swap.sh gdb/testsuite/gdb.testsuite/example-2.exp gdb/testsuite/gdb.testsuite/example.exp ... it fails like this instead: ... ERROR: no fileid for xerxes ... So: - obviously, using a gdb_exit in a proc results in trouble if the proc is called at a point in the test-case where we don't want a gdb_exit - using a gdb_exit in a caching proc may or may not result in trouble if the proc is called at a point in the test-case where we don't want gdb_exit. Whether we run into trouble depends on whether the result of the proc is already cached or not. This behaviour is undesirable. The simplest way of fixing this, is by splitting up the proc into an uncached and cached part, and moving the gdb_exit to the uncached part. This makes sure we that run into trouble in each test-case, independent of whether the result is already cached or not. That however is somewhat too restrictive in the case that there are more call sites in a single test-case. We only need the gdb_exit in the first call site, so we add the second tier of caching to make sure that we don't call gdb_exit from the uncached part more than once in a test-case. This two-tier scheme is convenient for can_spawn_for_attach, because it's a pre-existing proc that didn't use gdb_exit and consequently was used in random places in test-cases, and also multiple times in a single test-case. For a proc like allow_aarch64_mops_tests, which is only used at the very start of a test-case, once, IMO it's not necessary, and not worth the effort of splitting up the proc. However, if we factor this approach out into a special variant, say gdb_caching_proc_with_gdb_exit or some such, then it could be done relatively easily. Thanks, - Tom > Many of the predicates that test for hardware features restart GDB, so > this would be a refactor affecting many predicates. I don't mind doing > it but I'd suggest doing so as a separate patch. It would also be useful > to have a reproducer of the problem that the double-caching fixes to > check that I'm effectively solving the problem. > >>> + >>> +gdb_caching_proc allow_aarch64_mops_tests {} { >
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
Tom> - obviously, using a gdb_exit in a proc results in trouble if the proc
Tom> is called at a point in the test-case where we don't want a gdb_exit
Tom> - using a gdb_exit in a caching proc may or may not result in trouble if
Tom> the proc is called at a point in the test-case where we don't want
Tom> gdb_exit. Whether we run into trouble depends on whether the result
Tom> of the proc is already cached or not. This behaviour is undesirable.
I didn't really follow this thread, but I tend to think that caching
procs should be in two categories: those that require gdb to already be
running, and those that are standalone. Having a proc like this call
gdb_exit seems like a recipe for some confusion, to me. Even having one
start a new gdb seems like trouble, since IIRC that's hard to do without
introducing new test results, but the nature of a caching proc is that
the test names would then depend on which process wins the race to run
it first. I've thought sometimes about adding some mechanism to enforce
this split.
Tom
Hello Tom, Tom de Vries <tdevries@suse.de> writes: > On 5/21/24 23:03, Thiago Jung Bauermann wrote: >> Hello Pedro, >> Thank you for your review! >> Pedro Alves <pedro@palves.net> writes: >> >>>> Note this causes >>>> +# a restart of GDB. >>> >>> Might want to apply a similar logic to can_spawn_for_attach, wrt gdb_exit. >>> See the tail end of that function. Maybe we can factor that out and >>> reuse it. >> >> I looked into it, but I wasn't able to understand what is the problem >> that gdb_exit causes. Does it mean that any require predicate that >> restarts GDB needs to use that two-tier caching scheme? >> > > Let me try to explain the idea. Consider two demonstrator test-cases: <snip> Thank you for the explanation and clear example. I understand the problem now. > This two-tier scheme is convenient for can_spawn_for_attach, because it's a pre-existing > proc that didn't use gdb_exit and consequently was used in random places in test-cases, > and also multiple times in a single test-case. > > For a proc like allow_aarch64_mops_tests, which is only used at the very start of a > test-case, once, IMO it's not necessary, and not worth the effort of splitting up the > proc. Yes, I agree that this issue doesn't affect the require predicate proc. > However, if we factor this approach out into a special variant, say > gdb_caching_proc_with_gdb_exit or some such, then it could be done relatively easily. If there are other caching procs that exit or restart GDB that are used in the middle of testcases, then that is indeed a good idea.
diff --git a/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c b/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c new file mode 100644 index 000000000000..b981f033d210 --- /dev/null +++ b/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c @@ -0,0 +1,66 @@ +/* This test program is part of GDB, the GNU debugger. + + Copyright 2024 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +int +main (void) +{ + char source[40] __attribute__ ((aligned (8))) + = "This is a relatively long string..."; + char a[40] __attribute__ ((aligned (8))) + = "String to be overwritten with zeroes"; + char b[40] __attribute__ ((aligned (8))) + = "Another string to be memcopied..."; + char c[40] __attribute__ ((aligned (8))) + = "Another string to be memmoved..."; + char *p, *q; + long size, zero; + + /* Break here. */ + p = a; + size = sizeof (a); + zero = 0; + /* memset implemented in MOPS instructions. */ + __asm__ volatile ("setp [%0]!, %1!, %2\n\t" + "setm [%0]!, %1!, %2\n\t" + "sete [%0]!, %1!, %2\n\t" + : "+&r"(p), "+&r"(size) + : "r"(zero) + : "memory"); + + p = b; + q = source; + size = sizeof (b); + /* memmove implemented in MOPS instructions. */ + __asm__ volatile ("cpyp [%0]!, [%1]!, %2!\n\t" + "cpym [%0]!, [%1]!, %2!\n\t" + "cpye [%0]!, [%1]!, %2!\n\t" + : "+&r" (p), "+&r" (q), "+&r" (size) + : + : "memory"); + p = c; + q = source; + size = sizeof (c); + /* memcpy implemented in MOPS instructions. */ + __asm__ volatile ("cpyfp [%0]!, [%1]!, %2!\n\t" + "cpyfm [%0]!, [%1]!, %2!\n\t" + "cpyfe [%0]!, [%1]!, %2!\n\t" + : "+&r" (p), "+&r" (q), "+&r" (size) + : + : "memory"); + + return 0; +} diff --git a/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp b/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp new file mode 100644 index 000000000000..9e210602d800 --- /dev/null +++ b/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp @@ -0,0 +1,79 @@ +# Copyright 2024 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Test a binary that uses MOPS (Memory Operations) instructions. +# This test is similar to gdb.base/memops-watchpoint.exp, but specifically +# tests MOPS instructions rather than whatever instructions are used in the +# system libc's implementation of memset/memcpy/memmove. + +require allow_hw_watchpoint_tests allow_aarch64_mops_tests + +standard_testfile + +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \ + [list debug additional_flags=-march=armv9.3-a]] } { + return -1 +} + +set linespec ${srcfile}:[gdb_get_line_number "Break here"] +if ![runto ${linespec}] { + return -1 +} + +gdb_test "watch -location a\[28\]" \ + "(Hardware w|W)atchpoint ${decimal}: -location a\\\[28\\\]" \ + "set watch on a" +gdb_test "watch -location b\[28\]" \ + "(Hardware w|W)atchpoint ${decimal}: -location b\\\[28\\\]" \ + "set watchpoint on b" +gdb_test "watch -location c\[28\]" \ + "(Hardware w|W)atchpoint ${decimal}: -location c\\\[28\\\]" \ + "set watchpoint on c" + +gdb_test "continue" \ + [multi_line \ + "Continuing\\." \ + "" \ + "Hardware watchpoint ${decimal}: -location a\\\[28\\\]" \ + "" \ + "Old value = 104 'h'" \ + "New value = 0 '\\\\000'" \ + "$hex in main \\(\\) at .*aarch64-mops-watchpoint.c:$decimal" \ + "${decimal}\\s+__asm__ volatile \\(\"setp.*\\\\n\\\\t\""] \ + "continue until set watchpoint hits" + +gdb_test "continue" \ + [multi_line \ + "Continuing\\." \ + "" \ + "Hardware watchpoint ${decimal}: -location b\\\[28\\\]" \ + "" \ + "Old value = 101 'e'" \ + "New value = 114 'r'" \ + "$hex in main \\(\\) at .*aarch64-mops-watchpoint.c:$decimal" \ + "${decimal}\\s+__asm__ volatile \\(\"cpyp.*\\\\n\\\\t\""] \ + "continue until cpy watchpoint hits" + +gdb_test "continue" \ + [multi_line \ + "Continuing\\." \ + "" \ + "Hardware watchpoint ${decimal}: -location c\\\[28\\\]" \ + "" \ + "Old value = 100 'd'" \ + "New value = 114 'r'" \ + "$hex in main \\(\\) at .*aarch64-mops-watchpoint.c:$decimal" \ + "${decimal}\\s+__asm__ volatile \\(\"cpyfp.*\\\\n\\\\t\""] \ + "continue until cpyf watchpoint hits" diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp index fe3f05c18df9..78c926ac80b6 100644 --- a/gdb/testsuite/lib/gdb.exp +++ b/gdb/testsuite/lib/gdb.exp @@ -4497,6 +4497,67 @@ proc aarch64_supports_sme_svl { length } { return 1 } +# Run a test on the target to see if it supports Aarch64 MOPS (Memory +# Operations) extensions. Return 0 if so, 1 if it does not. Note this causes +# a restart of GDB. + +gdb_caching_proc allow_aarch64_mops_tests {} { + global srcdir subdir gdb_prompt inferior_exited_re + + set me "allow_aarch64_mops_tests" + + if { ![is_aarch64_target]} { + return 0 + } + + # ARMv9.3-A contains the MOPS extension. The test program doesn't use it, + # but take the opportunity to check whether the toolchain knows about MOPS. + set compile_flags "{additional_flags=-march=armv9.3-a}" + + # Compile a program that tests the MOPS feature. + set src { + #include <stdbool.h> + #include <sys/auxv.h> + + #ifndef HWCAP2_MOPS + #define HWCAP2_MOPS (1UL << 43) + #endif + + int main() { + bool mops_supported = getauxval (AT_HWCAP2) & HWCAP2_MOPS; + + return !mops_supported; + } + } + + if {![gdb_simple_compile $me $src executable $compile_flags]} { + return 0 + } + + # Compilation succeeded so now run it via gdb. + clean_restart $obj + gdb_run_cmd + gdb_expect { + -re ".*$inferior_exited_re with code 01.*${gdb_prompt} $" { + verbose -log "\n$me mops support not detected" + set allow_mops_tests 0 + } + -re ".*$inferior_exited_re normally.*${gdb_prompt} $" { + verbose -log "\n$me: mops support detected" + set allow_mops_tests 1 + } + default { + warning "\n$me: default case taken" + set allow_mops_tests 0 + } + } + gdb_exit + remote_file build delete $obj + + verbose "$me: returning $allow_mops_tests" 2 + return $allow_mops_tests +} + # A helper that compiles a test case to see if __int128 is supported. proc gdb_int128_helper {lang} { return [gdb_can_simple_compile "i128-for-$lang" {