On 1/29/20 12:18 AM, Simon Marchi wrote: > On 2020-01-15 6:51 a.m., Luis Machado wrote: >> New in v3: >> >> - Minor formatting and code cleanups. >> - Added count check to validate number of brk SIGTRAP's. >> - Moved count to SIGTRAP check conditional block. >> >> This test exercises the previous patch's code and makes sure GDB can >> properly get a SIGTRAP from various brk instruction patterns. >> >> GDB needs to be able to see the program exiting normally. If GDB doesn't >> support the additional brk instructions, we will see timeouts. >> >> We bail out with the first timeout since we won't be able to step through >> the program breakpoint anyway, so it is no use carrying on. >> >> gdb/testsuite/ChangeLog: >> >> 2020-01-15 Luis Machado >> >> * gdb.arch/aarch64-brk-patterns.c: New source file. >> * gdb.arch/aarch64-brk-patterns.exp: New test. >> --- >> gdb/testsuite/gdb.arch/aarch64-brk-patterns.c | 30 ++++++++ >> .../gdb.arch/aarch64-brk-patterns.exp | 74 +++++++++++++++++++ >> 2 files changed, 104 insertions(+) >> create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.c >> create mode 100644 gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp >> >> diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c >> new file mode 100644 >> index 0000000000..ccf9a35a94 >> --- /dev/null >> +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c >> @@ -0,0 +1,30 @@ >> +/* This file is part of GDB, the GNU debugger. >> + >> + Copyright 2020 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 . */ >> + >> +int main(void) >> +{ >> + /* Dummy instruction just so GDB doesn't stop at the first breakpoint >> + instruction. */ >> + __asm __volatile ("nop\n\t"); >> + >> + /* Multiple BRK instruction patterns. */ >> + __asm __volatile ("brk %0\n\t" ::"n"(0x0)); >> + __asm __volatile ("brk %0\n\t" ::"n"(0x900 + 0xf)); >> + __asm __volatile ("brk %0\n\t" ::"n"(0xf000)); >> + >> + return 0; >> +} >> diff --git a/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp >> new file mode 100644 >> index 0000000000..9a0ec81efa >> --- /dev/null >> +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp >> @@ -0,0 +1,74 @@ >> +# Copyright 2020 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 . >> +# >> +# This file is part of the gdb testsuite. >> + >> +# Test if GDB stops at various BRK instruction patterns inserted into >> +# the code. >> + >> +if {![is_aarch64_target]} { >> + verbose "Skipping ${gdb_test_file_name}." >> + return >> +} >> + >> +standard_testfile >> +if {[prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} { >> + return -1 >> +} >> + >> +if {![runto_main]} { >> + untested "could not run to main" >> + return -1 >> +} >> + >> +# Number of expected SIGTRAP's to get. This needs to be kept in sync >> +# with the source file. >> +set expected_traps 3 >> +set keep_going 1 >> +set count 0 >> +set old_timeout $timeout >> +set timeout 10 > > Any reason you are changing the timeout? There is nothing in the test that > looks like it would take time. > If GDB doesn't support one of these instructions, it will be caught in an infinite loop. The reduced timeout will prevent a long wait time until we bail out. > If changing the timeout is really necessary, look into using with_timeout_factor. > That would raise the timeout even further. We want a reduced one. It would be nice if we could reduce the timeout with with_timeout_factor. I gave it a try but it didn't work. I think we need adjustments to make it work with a floating point number. I'll look into it. >> + >> +while {$keep_going} { >> + >> + set test "brk instruction $count causes SIGTRAP" > > Instead of setting the test name like that, look into using the special $gdb_test_name > variable, available inside the gdb_test_multiple body. > Indeed. I'll tweak this to match the other reviews. >> + >> + # Continue to next program breakpoint instruction. >> + gdb_test_multiple "continue" $test { >> + -re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" { >> + pass $test >> + >> + # Insert a breakpoint at the program breakpoint instruction so GDB >> + # can step over it. >> + gdb_test "break" \ >> + "Breakpoint $decimal at $hex: file .*$srcfile, line $decimal.*" \ >> + "insert breakpoint at brk instruction $count" >> + incr count >> + } >> + -re "exited normally.*$gdb_prompt $" { >> + set keep_going 0 >> + } >> + timeout { >> + fail $test >> + set keep_going 0 >> + } >> + } >> +} >> + >> +set timeout $old_timeout >> + >> +if {$count < $expected_traps} { >> + fail "all brk instructions triggered." >> +} > > Use gdb_assert. > Ditto. > Is there any reason why $count would be greater than $expected_taps? If no, > I would test for "$count == $expected_traps". Not really. We'd want the exact match. Fixed now. Thanks for the review! How does the updated attached patch look?