| Message ID | 20200115115137.17771-3-luis.machado@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Handle additional brk instruction patterns | expand |
Minor nit below. Happy for it to be pushed with that one change. Alan. > On 15 Jan 2020, at 11:51, Luis Machado <luis.machado@linaro.org> 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 <luis.machado@linaro.org> > > * 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 <http://www.gnu.org/licenses/>. */ > + > +int main(void) New line between int and main. > +{ > + /* 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 <http://www.gnu.org/licenses/>. > +# > +# 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 > + > +while {$keep_going} { > + > + set test "brk instruction $count causes SIGTRAP" > + > + # 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." > +} > -- > 2.17.1 >
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 <luis.machado@linaro.org> > > * 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 <http://www.gnu.org/licenses/>. */ > + > +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 <http://www.gnu.org/licenses/>. > +# > +# 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 changing the timeout is really necessary, look into using with_timeout_factor. > + > +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. > + > + # 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. Is there any reason why $count would be greater than $expected_taps? If no, I would test for "$count == $expected_traps". Simon
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 <luis.machado@linaro.org> >> >> * 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 <http://www.gnu.org/licenses/>. */ >> + >> +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 <http://www.gnu.org/licenses/>. >> +# >> +# 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? From 67daaf5b752d6a168452f2d77b50410dd0a78a4f Mon Sep 17 00:00:00 2001 From: Luis Machado <luis.machado@linaro.org> Date: Mon, 13 Jan 2020 12:31:01 -0300 Subject: [PATCH] [AArch64] Test handling of additional brk instruction patterns New in v5: - Use gdb_test_name for gdb_test_multiple. - Use gdb_assert. - Verify count matches the expected sigtraps exactly. New in v4: - Fix formatting nit in gdb/testsuite/gdb.arch/aarch64-brk-patterns.c. 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-29 Luis Machado <luis.machado@linaro.org> * 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 | 31 ++++++++ .../gdb.arch/aarch64-brk-patterns.exp | 73 +++++++++++++++++++ 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..920ba8e2cb --- /dev/null +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c @@ -0,0 +1,31 @@ +/* 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 <http://www.gnu.org/licenses/>. */ + +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..dc95e3f6f5 --- /dev/null +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp @@ -0,0 +1,73 @@ +# 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 <http://www.gnu.org/licenses/>. +# +# 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 oldtimeout $timeout +set timeout 10 + +# Make sure we have a lower timeout in case GDB doesn't support a particular +# instruction. Such instruction will cause GDB to loop infinitely. +while {$keep_going} { + # Continue to next program breakpoint instruction. + gdb_test_multiple "continue" "brk instruction $count causes SIGTRAP" { + -re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" { + pass $gdb_test_name + + # 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 + } + # We've reached the end of the test. + -re "exited normally.*$gdb_prompt $" { + set keep_going 0 + } + timeout { + fail $gdb_test_name + set keep_going 0 + } + } +} + +set timeout $oldtimeout + +# Verify we stopped at the expected number of SIGTRAP's. +gdb_assert {$count == $expected_traps} "all brk instructions triggered" -- 2.17.1
On 2020-01-29 6:30 a.m., Luis Machado wrote: >>> +# 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. Ok. The worry I have with it is that if the target board has raised the timeout on purpose, because it's testing with a slow target/link/emulator, then this will cancel it. In this case I'd just leave the timeout as it is. Normally, GDB won't be broken for this test case, so it won't matter. Moreover, the timeout here on my x86 machine is 10 seconds by default. I just checked on an AArch64 box on the compile farm, it's 10 there too. So in which case was it useful to set it to 10? >> 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. I'm not sure that's desirable for a test case to ever reduce the timeout, for the reason explained above. Simon
On 1/29/20 11:01 AM, Simon Marchi wrote: > On 2020-01-29 6:30 a.m., Luis Machado wrote: >>>> +# 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. > > Ok. The worry I have with it is that if the target board has raised the timeout > on purpose, because it's testing with a slow target/link/emulator, then this will > cancel it. In this case I'd just leave the timeout as it is. Normally, GDB won't > be broken for this test case, so it won't matter. > > Moreover, the timeout here on my x86 machine is 10 seconds by default. I just checked > on an AArch64 box on the compile farm, it's 10 there too. So in which case was it > useful to set it to 10? > Interesting. In my mind the default was from 30 to 60. Maybe that has changed over the years. If the boards are free to set it, then that is more desirable. I'll let it be then.
On 1/29/20 11:04 AM, Luis Machado wrote: > > > On 1/29/20 11:01 AM, Simon Marchi wrote: >> On 2020-01-29 6:30 a.m., Luis Machado wrote: >>>>> +# 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. >> >> Ok. The worry I have with it is that if the target board has raised >> the timeout >> on purpose, because it's testing with a slow target/link/emulator, >> then this will >> cancel it. In this case I'd just leave the timeout as it is. >> Normally, GDB won't >> be broken for this test case, so it won't matter. >> >> Moreover, the timeout here on my x86 machine is 10 seconds by >> default. I just checked >> on an AArch64 box on the compile farm, it's 10 there too. So in which >> case was it >> useful to set it to 10? >> > > Interesting. In my mind the default was from 30 to 60. Maybe that has > changed over the years. If the boards are free to set it, then that is > more desirable. > > I'll let it be then. Here's the updated patch. From a2814e51d2dbec1130cf53b24a3f71041791fe69 Mon Sep 17 00:00:00 2001 From: Luis Machado <luis.machado@linaro.org> Date: Mon, 13 Jan 2020 12:31:01 -0300 Subject: [PATCH] [AArch64] Test handling of additional brk instruction patterns New in v5: - Use gdb_test_name for gdb_test_multiple. - Use gdb_assert. - Verify count matches the expected sigtraps exactly. New in v4: - Fix formatting nit in gdb/testsuite/gdb.arch/aarch64-brk-patterns.c. 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-29 Luis Machado <luis.machado@linaro.org> * 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 | 31 +++++++++ .../gdb.arch/aarch64-brk-patterns.exp | 69 +++++++++++++++++++ 2 files changed, 100 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..920ba8e2cb --- /dev/null +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.c @@ -0,0 +1,31 @@ +/* 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 <http://www.gnu.org/licenses/>. */ + +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..5cb55da219 --- /dev/null +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp @@ -0,0 +1,69 @@ +# 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 <http://www.gnu.org/licenses/>. +# +# 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 + +# Make sure we have a lower timeout in case GDB doesn't support a particular +# instruction. Such instruction will cause GDB to loop infinitely. +while {$keep_going} { + # Continue to next program breakpoint instruction. + gdb_test_multiple "continue" "brk instruction $count causes SIGTRAP" { + -re "Program received signal SIGTRAP, Trace/breakpoint trap.*$gdb_prompt $" { + pass $gdb_test_name + + # 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 + } + # We've reached the end of the test. + -re "exited normally.*$gdb_prompt $" { + set keep_going 0 + } + timeout { + fail $gdb_test_name + set keep_going 0 + } + } +} + +# Verify we stopped at the expected number of SIGTRAP's. +gdb_assert {$count == $expected_traps} "all brk instructions triggered" -- 2.17.1
On 2020-01-29 9:09 a.m., Luis Machado wrote: > On 1/29/20 11:04 AM, Luis Machado wrote: >> >> >> On 1/29/20 11:01 AM, Simon Marchi wrote: >>> On 2020-01-29 6:30 a.m., Luis Machado wrote: >>>>>> +# 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. >>> >>> Ok. The worry I have with it is that if the target board has raised >>> the timeout >>> on purpose, because it's testing with a slow target/link/emulator, >>> then this will >>> cancel it. In this case I'd just leave the timeout as it is. >>> Normally, GDB won't >>> be broken for this test case, so it won't matter. >>> >>> Moreover, the timeout here on my x86 machine is 10 seconds by >>> default. I just checked >>> on an AArch64 box on the compile farm, it's 10 there too. So in which >>> case was it >>> useful to set it to 10? >>> >> >> Interesting. In my mind the default was from 30 to 60. Maybe that has >> changed over the years. If the boards are free to set it, then that is >> more desirable. >> >> I'll let it be then. > > Here's the updated patch. Thanks, that looks good to me. Simon
On 1/29/20 11:11 AM, Simon Marchi wrote: > On 2020-01-29 9:09 a.m., Luis Machado wrote: >> On 1/29/20 11:04 AM, Luis Machado wrote: >>> >>> >>> On 1/29/20 11:01 AM, Simon Marchi wrote: >>>> On 2020-01-29 6:30 a.m., Luis Machado wrote: >>>>>>> +# 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. >>>> >>>> Ok. The worry I have with it is that if the target board has raised >>>> the timeout >>>> on purpose, because it's testing with a slow target/link/emulator, >>>> then this will >>>> cancel it. In this case I'd just leave the timeout as it is. >>>> Normally, GDB won't >>>> be broken for this test case, so it won't matter. >>>> >>>> Moreover, the timeout here on my x86 machine is 10 seconds by >>>> default. I just checked >>>> on an AArch64 box on the compile farm, it's 10 there too. So in which >>>> case was it >>>> useful to set it to 10? >>>> >>> >>> Interesting. In my mind the default was from 30 to 60. Maybe that has >>> changed over the years. If the boards are free to set it, then that is >>> more desirable. >>> >>> I'll let it be then. >> >> Here's the updated patch. > > Thanks, that looks good to me. > > Simon > Pushed now. Thanks!
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 <http://www.gnu.org/licenses/>. */ + +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 <http://www.gnu.org/licenses/>. +# +# 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 + +while {$keep_going} { + + set test "brk instruction $count causes SIGTRAP" + + # 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." +}