[PATCH 2/2,v3] [AArch64] Test handling of additional brk instruction patterns

[Luis Machado]

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AArch64-Test-handling-of-additional-brk-instruction-.patch
Type: text/x-patch
Size: 5450 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20200129/ce81bcfc/attachment.bin>