RE: [PATCH 2/2][AArch64] Test handling of additional brk instruction patterns

["Aktemur, Tankut Baris" <tankut dot baris dot aktemur at intel dot com>]

On Tuesday, January 14, 2020 5:25 AM, Simon Marchi wrote:
> On 2020-01-13 12:25 p.m., Luis Machado wrote:
> > 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-13  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         | 67 +++++++++++++++++++
> >  2 files changed, 97 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..3532a0df95
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.arch/aarch64-brk-patterns.exp
> > @@ -0,0 +1,67 @@
> > +# 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] {

I'm not aware of a common convention of use of braces and spaces, but adding braces
as in {![runto_main]} and removing the spaces after/before the opening/closing braces
in { [prepare_for_testing...] } above could make the use more consistent within the
file.

> > +    untested "could not run to main"
> > +    return -1
> > +}
> > +
> > +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"
> > +      }
> > +      -re "exited normally.*$gdb_prompt $" {
> > +	  set keep_going 0
> > +      }
> > +      timeout {
> > +	  fail $test
> > +	  set keep_going 0
> > +      }
> > +  }
> > +  set count [expr "$count + 1"]

A minor thing.  This could be simplified as

  incr count

-Baris

> > +}
> > +
> > +set timeout $old_timeout
> > --
> > 2.17.1
> >
> 
> I don't see anything wrong with the test, but I was wondering if we should
> assert that $count is the right value in the end, to make sure that we did
> hit the expected number of breakpoints.
> 
> Simon
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928