[PATCH 1/2] gdb/testsuite: add simavr.exp board

[PATCH 1/2] gdb/testsuite: add simavr.exp board

[Simon Marchi]

This patch adds a board file for against a simavr target (so, for the
AVR architecture).

simavr, when started with option -g, runs a GDB stub on port 1234.  In
the current latest release (1.6), the port is hardcoded to 1234.  But in
master, there is the option to choose another port.  So while the board
file hardcodes the port today, in the future it should be possible to
let the user choose a port, or automatically select a free port.

It is easy enough to run, make sure you have avr-gcc/avr-g++ and simavr
installed, and as usual just run:

    make check RUNTESTFLAGS="--target_board=simavr"

The following environment variables influence the behavior of the board
file:

  - SIMAVR_MCU: type of chip to simulate
  - SIMAVR_PATH: path to simavr binary (useful if you build your own
    simavr or for some reason it is not simply called `simavr`.

After sending "target remote :1234" to GDB, I tried using gdb_expect to
wait for the "Remote debugging using :1234" message and the GDB prompt.
That didn't work well, but gdb_test_multiple does work.  For example,
with test gdb.base/break.exp, it blocks at some point when using

    gdb_expect {
	-re "Remote debugging using :1234.*$gdb_prompt " {}
    }

If anybody knows how to make gdb_expect work here, I could use it
(although gdb_test_multiple works fine, except it will probably generate
a FAIL in cause of failure).

As expected, there are a lot of failures.  Many tests use some features
not supported by such a target, and I suppose there are real GDB bugs
too.  But a lot also passes (including tests that actually run stuff),
so this board file should still help to validate changes to the AVR
architecture support.

These are the results I got of running tests gdb.base/*.exp:

    # of expected passes            20926
    # of unexpected failures        2257
    # of expected failures          14
    # of unknown successes          1
    # of known failures             13
    # of unresolved testcases       592
    # of untested testcases         156
    # of unsupported tests          30
    # of paths in test names        3
    # of duplicate test names       56

gdb/testsuite/ChangeLog:

	* boards/simavr.exp: New file.
---
 gdb/testsuite/boards/simavr.exp | 91 +++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 gdb/testsuite/boards/simavr.exp

diff --git a/gdb/testsuite/boards/simavr.exp b/gdb/testsuite/boards/simavr.exp
new file mode 100644
index 000000000000..8e4ba8d0810f
--- /dev/null
+++ b/gdb/testsuite/boards/simavr.exp
@@ -0,0 +1,91 @@
+# 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/>.
+
+# Let the user override the path to the simavr binary with the SIMAVR_PATH
+# environment variable.
+
+if { [info exists ::env(SIMAVR_PATH)] } {
+    set simavr_path $::env(SIMAVR_PATH)
+} else {
+    set simavr_path simavr
+}
+
+# Let the user override the simulated AVR chip with the SIMAVR_PATH environment
+# variable.
+#
+# The value passed here must be supported by avr-gcc (see the -mmcu flag in the
+# `AVR Options` section of the gcc(1) man page) and by simavr (see output of
+# `simavr --list-cores`).
+
+if { [info exists ::env(SIMAVR_MCU)] } {
+    set simavr_mcu $::env(SIMAVR_MCU)
+} else {
+    set simavr_mcu atmega2560
+}
+
+set simavr_last_load_file ""
+set simavr_spawn_id ""
+
+set_board_info compiler avr-gcc
+set_board_info c++compiler avr-g++
+
+set_board_info cflags "-mmcu=${simavr_mcu}"
+set_board_info ldflags "-mmcu=${simavr_mcu}"
+
+set_board_info use_gdb_stub 1
+set_board_info gdb_protocol "remote"
+set_board_info gdb,do_reload_on_run 1
+set_board_info noargs 1
+set_board_info gdb,noinferiorio 1
+set_board_info gdb,nofileio 1
+set_board_info gdb,noresults 1
+set_board_info gdb,nosignals 1
+
+proc gdb_load { file } {
+    global simavr_last_load_file
+    global simavr_spawn_id
+    global simavr_mcu
+    global simavr_path
+
+    if { $file == "" } {
+	set file $simavr_last_load_file
+    } else {
+	set simavr_last_load_file $file
+    }
+
+    gdb_file_cmd $file
+
+    # Close any previous simavr instance.
+    if { $simavr_spawn_id != "" } {
+	close -i $simavr_spawn_id
+	set simavr_spawn_id ""
+    }
+
+    # Run simavr.
+    set cmd "spawn -noecho ${simavr_path} --mcu ${simavr_mcu} -g $file"
+    verbose -log "Spawning simavr: $cmd"
+    eval $cmd
+    set simavr_spawn_id $spawn_id
+    gdb_expect {
+	-i $simavr_spawn_id -re ".*avr_gdb_init listening on port 1234" {}
+	timeout { error "unable to start simavr" }
+    }
+
+    # Connect to simavr.
+    send_gdb "target remote :1234\n"
+    gdb_test_multiple "" "" {
+	-wrap -re "Remote debugging using :1234.*" {}
+    }
+}
-- 
2.26.2

[PATCH 2/2] gdb: make avr_integer_to_address generate code or data address based on type

[Simon Marchi]

From: Cristiano De Alti <cristiano_dealti@hotmail.com>

I (Simon Marchi) am re-sending this patch written by Cristiano De Alti:

    https://sourceware.org/legacy-ml/gdb-patches/2016-03/msg00318.html

The commit message is new but the code has not changed.

The AVR architecture is a Harvard one, meaning it has different memory
spaces for code and data.  In GDB, this is dealt with by having the data
(SRAM) addresses start at 0x00800000.  When interpreting an integer as
an address (converting to a CORE_ADDR), we currently always generate a
data address.  This doesn't work for some cases described below, where
the integer is meant to represent a code address.

This patch changes avr_integer_to_address so that it generates the
correct type of address (code or data) based on the passed type.

Using the simavr.exp board, I didn't see any regressions when running
the gdb.base/*.exp tests.  A few tests go from fail to pass, but none
from pass to fail.  There are a few new fails and unresolved, but it's
just because some tests manage to make more progress before failing in a
different way.

In practice, it fixes disassembling by address, as described in the PR:

    - (gdb) disassemble 0x12a,0x12b
    - Dump of assembler code from 0x12a to 0x12b:
    -    0x0000012a <main+0>: push    r28
    - End of assembler dump.

    + (gdb) disassemble 0x12a,0x12b
    + Dump of assembler code from 0x80012a to 0x80012b:
    +    0x0080012a:  nop
    + End of assembler dump.

And also, setting a breakpoint by address:

    - (gdb) p &main
    - $1 = (int (*)(void)) 0x12a <main>
    - (gdb) b *0x12a
    - Breakpoint 1 at 0x80012a

    + (gdb) p &main
    + $1 = (int (*)(void)) 0x12a <main>
    + (gdb) b *0x12a
    + Breakpoint 1 at 0x12a: file test-avr.c, line 3.
    + Note: automatically using hardware breakpoints for read-only addresses.

gdb/ChangeLog:

	PR gdb/13519
	* avr-tdep.c (avr_integer_to_address): Return data or code
	address accordingly to the second 'type' argument of the
	function.
---
 gdb/avr-tdep.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c
index fd602e35e50d..74ab531711e9 100644
--- a/gdb/avr-tdep.c
+++ b/gdb/avr-tdep.c
@@ -363,7 +363,10 @@ avr_integer_to_address (struct gdbarch *gdbarch,
 {
   ULONGEST addr = unpack_long (type, buf);
 
-  return avr_make_saddr (addr);
+  if (TYPE_DATA_SPACE (type))
+    return avr_make_saddr (addr);
+  else
+    return avr_make_iaddr (addr);
 }
 
 static CORE_ADDR
-- 
2.26.2

Re: [PATCH 1/2] gdb/testsuite: add simavr.exp board

[Pedro Alves]

On 5/24/20 3:20 PM, Simon Marchi via Gdb-patches wrote:
> After sending "target remote :1234" to GDB, I tried using gdb_expect to
> wait for the "Remote debugging using :1234" message and the GDB prompt.
> That didn't work well, but gdb_test_multiple does work.  For example,
> with test gdb.base/break.exp, it blocks at some point when using
> 
>     gdb_expect {
> 	-re "Remote debugging using :1234.*$gdb_prompt " {}
>     }
> 
> If anybody knows how to make gdb_expect work here, I could use it
> (although gdb_test_multiple works fine, except it will probably generate
> a FAIL in cause of failure).

Strange.  gdb_test_multiple is just a wrapper around gdb_expect, so it
should work.  Maybe it's sporadic, and gdb_test_multiple only seems
to work better?  I'd look at RUNTESTFLAGS="-v -v -v --debug" output
to figure out why the pattern didn't match, causing the hang.

Thanks,
Pedro Alves

Re: [PATCH 2/2] gdb: make avr_integer_to_address generate code or data address based on type

[Pedro Alves]

On 5/24/20 3:20 PM, Simon Marchi via Gdb-patches wrote:
> From: Cristiano De Alti <cristiano_dealti@hotmail.com>
> 
> I (Simon Marchi) am re-sending this patch written by Cristiano De Alti:
> 
>     https://sourceware.org/legacy-ml/gdb-patches/2016-03/msg00318.html
> 
> The commit message is new but the code has not changed.
> 
> The AVR architecture is a Harvard one, meaning it has different memory
> spaces for code and data.  In GDB, this is dealt with by having the data
> (SRAM) addresses start at 0x00800000.  When interpreting an integer as
> an address (converting to a CORE_ADDR), we currently always generate a
> data address.  This doesn't work for some cases described below, where
> the integer is meant to represent a code address.
> 
> This patch changes avr_integer_to_address so that it generates the
> correct type of address (code or data) based on the passed type.
> 
> Using the simavr.exp board, I didn't see any regressions when running
> the gdb.base/*.exp tests.  A few tests go from fail to pass, but none
> from pass to fail.  There are a few new fails and unresolved, but it's
> just because some tests manage to make more progress before failing in a
> different way.
> 
> In practice, it fixes disassembling by address, as described in the PR:
> 
>     - (gdb) disassemble 0x12a,0x12b
>     - Dump of assembler code from 0x12a to 0x12b:
>     -    0x0000012a <main+0>: push    r28
>     - End of assembler dump.
> 
>     + (gdb) disassemble 0x12a,0x12b
>     + Dump of assembler code from 0x80012a to 0x80012b:
>     +    0x0080012a:  nop
>     + End of assembler dump.
> 
> And also, setting a breakpoint by address:
> 
>     - (gdb) p &main
>     - $1 = (int (*)(void)) 0x12a <main>
>     - (gdb) b *0x12a
>     - Breakpoint 1 at 0x80012a
> 
>     + (gdb) p &main
>     + $1 = (int (*)(void)) 0x12a <main>
>     + (gdb) b *0x12a
>     + Breakpoint 1 at 0x12a: file test-avr.c, line 3.
>     + Note: automatically using hardware breakpoints for read-only addresses.
> 
> gdb/ChangeLog:
> 
> 	PR gdb/13519
> 	* avr-tdep.c (avr_integer_to_address): Return data or code
> 	address accordingly to the second 'type' argument of the
> 	function.

Thank you for writing the commit log.  LGTM.

Pedro Alves

Re: [PATCH 1/2] gdb/testsuite: add simavr.exp board

[Simon Marchi]

On 2020-05-24 12:33 p.m., Pedro Alves wrote:
> On 5/24/20 3:20 PM, Simon Marchi via Gdb-patches wrote:
>> After sending "target remote :1234" to GDB, I tried using gdb_expect to
>> wait for the "Remote debugging using :1234" message and the GDB prompt.
>> That didn't work well, but gdb_test_multiple does work.  For example,
>> with test gdb.base/break.exp, it blocks at some point when using
>>
>>     gdb_expect {
>> 	-re "Remote debugging using :1234.*$gdb_prompt " {}
>>     }
>>
>> If anybody knows how to make gdb_expect work here, I could use it
>> (although gdb_test_multiple works fine, except it will probably generate
>> a FAIL in cause of failure).
> 
> Strange.  gdb_test_multiple is just a wrapper around gdb_expect, so it
> should work.  Maybe it's sporadic, and gdb_test_multiple only seems
> to work better?  I'd look at RUNTESTFLAGS="-v -v -v --debug" output
> to figure out why the pattern didn't match, causing the hang.

Ah, the issue wasn't really gdb_expect or the regex.

It's just that gdb_run_cmd expects gdb_reload to return 0 in order
to proceed:

 268             if { [gdb_reload] != 0 } {
 269                 return
 270             }

And tcl has this common shell behavior where a procedure returns
the same thing as the last thing it called, without an explicit
"return":

$ cat test.tcl
proc p1 {} {
  return 17
}

proc p2 {} {
  p1
}

puts [p2]
$ tclsh test.tcl
17

And gdb_test_multiple just happened to return 0, but gdb_expect
did not.  Using gdb_expect and adding a `return 0` in my proc
made it work.

Here's the patch with that fixed:

From 742643b78c9307f9a1e3d6103efb4834295099db Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Sun, 24 May 2020 10:20:39 -0400
Subject: [PATCH] gdb/testsuite: add simavr.exp board

This patch adds a board file for against a simavr target (so, for the
AVR architecture).

simavr, when started with option -g, runs a GDB stub on port 1234.  In
the current latest release (1.6), the port is hardcoded to 1234.  But in
master, there is the option to choose another port.  So while the board
file hardcodes the port today, in the future it should be possible to
let the user choose a port, or automatically select a free port.

It is easy enough to run, make sure you have avr-gcc/avr-g++ and simavr
installed, and as usual just run:

    make check RUNTESTFLAGS="--target_board=simavr"

The following environment variables influence the behavior of the board
file:

  - SIMAVR_MCU: type of chip to simulate
  - SIMAVR_PATH: path to simavr binary (useful if you build your own
    simavr or for some reason it is not simply called `simavr`.

As expected, there are a lot of failures.  Many tests use some features
not supported by such a target, and I suppose there are real GDB bugs
too.  But a lot also passes (including tests that actually run stuff),
so this board file should still help to validate changes to the AVR
architecture support.

These are the results I got of running tests gdb.base/*.exp:

    # of expected passes            20926
    # of unexpected failures        2257
    # of expected failures          14
    # of unknown successes          1
    # of known failures             13
    # of unresolved testcases       592
    # of untested testcases         156
    # of unsupported tests          30
    # of paths in test names        3
    # of duplicate test names       56

gdb/testsuite/ChangeLog:

	* boards/simavr.exp: New file.

Change-Id: Ib7fa8c4e2e90b08b104bb9b552df37779de3bc21
---
 gdb/testsuite/boards/simavr.exp | 94 +++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)
 create mode 100644 gdb/testsuite/boards/simavr.exp

diff --git a/gdb/testsuite/boards/simavr.exp b/gdb/testsuite/boards/simavr.exp
new file mode 100644
index 000000000000..660af7ba83f8
--- /dev/null
+++ b/gdb/testsuite/boards/simavr.exp
@@ -0,0 +1,94 @@
+# 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/>.
+
+# Let the user override the path to the simavr binary with the SIMAVR_PATH
+# environment variable.
+
+if { [info exists ::env(SIMAVR_PATH)] } {
+    set simavr_path $::env(SIMAVR_PATH)
+} else {
+    set simavr_path simavr
+}
+
+# Let the user override the simulated AVR chip with the SIMAVR_PATH environment
+# variable.
+#
+# The value passed here must be supported by avr-gcc (see the -mmcu flag in the
+# `AVR Options` section of the gcc(1) man page) and by simavr (see output of
+# `simavr --list-cores`).
+
+if { [info exists ::env(SIMAVR_MCU)] } {
+    set simavr_mcu $::env(SIMAVR_MCU)
+} else {
+    set simavr_mcu atmega2560
+}
+
+set simavr_last_load_file ""
+set simavr_spawn_id ""
+
+set_board_info compiler avr-gcc
+set_board_info c++compiler avr-g++
+
+set_board_info cflags "-mmcu=${simavr_mcu}"
+set_board_info ldflags "-mmcu=${simavr_mcu}"
+
+set_board_info use_gdb_stub 1
+set_board_info gdb_protocol "remote"
+set_board_info gdb,do_reload_on_run 1
+set_board_info noargs 1
+set_board_info gdb,noinferiorio 1
+set_board_info gdb,nofileio 1
+set_board_info gdb,noresults 1
+set_board_info gdb,nosignals 1
+
+proc gdb_load { file } {
+    global simavr_last_load_file
+    global simavr_spawn_id
+    global simavr_mcu
+    global simavr_path
+    global gdb_prompt
+
+    if { $file == "" } {
+	set file $simavr_last_load_file
+    } else {
+	set simavr_last_load_file $file
+    }
+
+    gdb_file_cmd $file
+
+    # Close any previous simavr instance.
+    if { $simavr_spawn_id != "" } {
+	close -i $simavr_spawn_id
+	set simavr_spawn_id ""
+    }
+
+    # Run simavr.
+    set cmd "spawn -noecho ${simavr_path} --mcu ${simavr_mcu} -g $file"
+    verbose -log "Spawning simavr: $cmd"
+    eval $cmd
+    set simavr_spawn_id $spawn_id
+    gdb_expect {
+	-i $simavr_spawn_id -re ".*avr_gdb_init listening on port 1234" {}
+	timeout { error "unable to start simavr" }
+    }
+
+    # Connect to simavr.
+    send_gdb "target remote :1234\n"
+    gdb_expect {
+	-re ".*Remote debugging using :1234.*\[\r\n\]+$gdb_prompt $" {}
+    }
+
+    return 0
+}
-- 
2.26.2

Re: [PATCH 1/2] gdb/testsuite: add simavr.exp board

[Pedro Alves]

On 5/25/20 2:29 PM, Simon Marchi via Gdb-patches wrote:

> Ah, the issue wasn't really gdb_expect or the regex.
> 
> It's just that gdb_run_cmd expects gdb_reload to return 0 in order
> to proceed:
> 
>  268             if { [gdb_reload] != 0 } {
>  269                 return
>  270             }
> 
> And tcl has this common shell behavior where a procedure returns
> the same thing as the last thing it called, without an explicit
> "return":
> 
> $ cat test.tcl
> proc p1 {} {
>   return 17
> }
> 
> proc p2 {} {
>   p1
> }
> 
> puts [p2]
> $ tclsh test.tcl
> 17
> 

Yeah.

> And gdb_test_multiple just happened to return 0, but gdb_expect
> did not.  Using gdb_expect and adding a `return 0` in my proc
> made it work.
> 

Thanks for digging into it.

> Here's the patch with that fixed:

Don't you want to handle timeout (and maybe unexpected output) in
the new gdb_expect call?

Regardless, this LGTM.  It doesn't impact anything else and can always be
improved afterwards.

Thanks,
Pedro Alves

Re: [PATCH 1/2] gdb/testsuite: add simavr.exp board

[Simon Marchi]

On 2020-05-25 10:44 a.m., Pedro Alves wrote:
>> Here's the patch with that fixed:
> 
> Don't you want to handle timeout (and maybe unexpected output) in
> the new gdb_expect call?

It's a good idea to add a timeout.  I added this line:

  timeout { error "unable to connect to simavr stub" }

If for some reason we can't connect (I tested it by purposely expecting the wrong string),
it will throw and abort the test promptly with an error.  Without the timeout, the test
continues pointlessly and it takes ages because we wait for many tests to time out.

> Regardless, this LGTM.  It doesn't impact anything else and can always be
> improved afterwards.

Thanks, I pushed the series with that fixed.  If you find anything else to improve, don't
hesitate to suggest it, I can fix it up later.

Simon