* [PING][PATCH] Add support for the __flash qualifier on AVR
2014-07-08 10:54 [PATCH] Add support for the __flash qualifier on AVR Pierre Langlois
@ 2014-07-15 10:36 ` Pierre Langlois
2014-07-15 14:00 ` [PATCH] " Joel Brobecker
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Pierre Langlois @ 2014-07-15 10:36 UTC (permalink / raw)
To: gdb-patches
Ping.
On 08/07/14 11:54, Pierre Langlois wrote:
> The __flash qualifier is part of the named address spaces for AVR [1]. It allows
> putting read-only data in the flash memory, normally reserved for code.
>
> When used together with a pointer, the DW_AT_address_class attribute is set to 1
> and allows GDB to detect that when it will be dereferenced, the data will be
> loaded from the flash memory (with the LPM instruction).
>
> We can now properly debug the following code:
>
> ~~~
> const __flash char data_in_flash = 0xab;
>
> int
> main (void)
> {
> const __flash char *pointer_to_flash = &data_in_flash;
> }
> ~~~
>
> ~~~
> (gdb) print pointer_to_flash
> $1 = 0x1e8 <data_in_flash> "\253"
> (gdb) print/x *pointer_to_flash
> $2 = 0xab
> (gdb) x/x pointer_to_flash
> 0x1e8 <data_in_flash>: 0xXXXXXXab
> ~~~
>
> Whereas previously, GDB would revert to the default address space which is RAM
> and mapped in higher memory:
>
> ~~~
> (gdb) print pointer_to_flash
> $1 = 0x8001e8 ""
> ~~~
>
> Thanks,
>
> Pierre
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html
>
> 2014-07-08 Pierre Langlois <pierre.langlois@embecosm.com>
>
> gdb/
> * avr-tdep.c (AVR_TYPE_ADDRESS_CLASS_FLASH): New macro.
> (AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH): Likewise.
> (avr_address_to_pointer): Check for AVR_TYPE_ADDRESS_CLASS_FLASH.
> (avr_pointer_to_address): Likewise.
> (avr_address_class_type_flags): New function.
> (avr_address_class_type_flags_to_name): Likewise.
> (avr_address_class_name_to_type_flags): Likewise.
> (avr_gdbarch_init): Set address_class_type_flags,
> address_class_type_flags_to_name and
> address_class_name_to_type_flags.
>
> gdb/testsuite/
> * gdb.arch/avr-flash-qualifer.c: New.
> * gdb.arch/avr-flash-qualifer.exp: New.
> ---
> gdb/avr-tdep.c | 77 ++++++++++++++++++++++++--
> gdb/testsuite/gdb.arch/avr-flash-qualifier.c | 32 +++++++++++
> gdb/testsuite/gdb.arch/avr-flash-qualifier.exp | 52 +++++++++++++++++
> 3 files changed, 156 insertions(+), 5 deletions(-)
> create mode 100644 gdb/testsuite/gdb.arch/avr-flash-qualifier.c
> create mode 100644 gdb/testsuite/gdb.arch/avr-flash-qualifier.exp
>
> diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c
> index 9b0bfaf..eb99748 100644
> --- a/gdb/avr-tdep.c
> +++ b/gdb/avr-tdep.c
> @@ -71,6 +71,16 @@
>
> /* Constants: prefixed with AVR_ to avoid name space clashes */
>
> +/* Address space flags */
> +
> +/* We are assigning the TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1 to the flash address
> + space. */
> +
> +#define AVR_TYPE_ADDRESS_CLASS_FLASH TYPE_ADDRESS_CLASS_1
> +#define AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH \
> + TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1
> +
> +
> enum
> {
> AVR_REG_W = 24,
> @@ -295,10 +305,19 @@ avr_address_to_pointer (struct gdbarch *gdbarch,
> {
> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>
> + /* Is it a data address in flash? */
> + if (AVR_TYPE_ADDRESS_CLASS_FLASH (type))
> + {
> + /* A data address in flash is always byte addressed. */
> + store_unsigned_integer (buf, TYPE_LENGTH (type), byte_order,
> + avr_convert_iaddr_to_raw (addr));
> + }
> /* Is it a code address? */
> - if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
> - || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
> + else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
> + || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
> {
> + /* A code address, either a function pointer or the program counter, is
> + word (16 bits) addressed. */
> store_unsigned_integer (buf, TYPE_LENGTH (type), byte_order,
> avr_convert_iaddr_to_raw (addr >> 1));
> }
> @@ -318,10 +337,13 @@ avr_pointer_to_address (struct gdbarch *gdbarch,
> CORE_ADDR addr
> = extract_unsigned_integer (buf, TYPE_LENGTH (type), byte_order);
>
> + /* Is it a data address in flash? */
> + if (AVR_TYPE_ADDRESS_CLASS_FLASH (type))
> + return avr_make_iaddr (addr);
> /* Is it a code address? */
> - if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
> - || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
> - || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
> + else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
> + || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
> + || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
> return avr_make_iaddr (addr << 1);
> else
> return avr_make_saddr (addr);
> @@ -1342,6 +1364,45 @@ avr_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
> return -1;
> }
>
> +/* Map DW_AT_address_class attributes to a type_instance_flag_value. Note that
> + this attribute is only valid with pointer types and therefore the flag is set
> + to the pointer type and not its target type. */
> +
> +static int
> +avr_address_class_type_flags (int byte_size, int dwarf2_addr_class)
> +{
> + // __flash qualifier
> + if (dwarf2_addr_class == 1 && byte_size == 2)
> + return AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH;
> + return 0;
> +}
> +
> +/* Convert a type_instance_flag_value to an address space qualifier and
> + vice-versa. */
> +
> +static const char*
> +avr_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)
> +{
> + if (type_flags & AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH)
> + return "flash";
> + else
> + return NULL;
> +}
> +
> +static int
> +avr_address_class_name_to_type_flags (struct gdbarch *gdbarch,
> + const char* name,
> + int *type_flags_ptr)
> +{
> + if (strcmp (name, "flash") == 0)
> + {
> + *type_flags_ptr = AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH;
> + return 1;
> + }
> + else
> + return 0;
> +}
> +
> /* Initialize the gdbarch structure for the AVR's. */
>
> static struct gdbarch *
> @@ -1452,6 +1513,12 @@ avr_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> set_gdbarch_unwind_pc (gdbarch, avr_unwind_pc);
> set_gdbarch_unwind_sp (gdbarch, avr_unwind_sp);
>
> + set_gdbarch_address_class_type_flags (gdbarch, avr_address_class_type_flags);
> + set_gdbarch_address_class_name_to_type_flags
> + (gdbarch, avr_address_class_name_to_type_flags);
> + set_gdbarch_address_class_type_flags_to_name
> + (gdbarch, avr_address_class_type_flags_to_name);
> +
> return gdbarch;
> }
>
> diff --git a/gdb/testsuite/gdb.arch/avr-flash-qualifier.c b/gdb/testsuite/gdb.arch/avr-flash-qualifier.c
> new file mode 100644
> index 0000000..4e5b451
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/avr-flash-qualifier.c
> @@ -0,0 +1,32 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2014 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/>. */
> +
> +const __flash char data_in_flash = 0xab;
> +
> +static void
> +pass_to_function (const __flash char *p)
> +{
> +}
> +
> +int
> +main (void)
> +{
> + const __flash char *pointer_to_flash = &data_in_flash;
> + /* break here. */
> + pass_to_function (&data_in_flash);
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.arch/avr-flash-qualifier.exp b/gdb/testsuite/gdb.arch/avr-flash-qualifier.exp
> new file mode 100644
> index 0000000..e9ccb4f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/avr-flash-qualifier.exp
> @@ -0,0 +1,52 @@
> +# Copyright 2014 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.
> +#
> +# Contributed by Pierre Langlois <pierre.langlois@embecosm.com>
> +# Tests for the AVR __flash named address space qualifier.
> +
> +if {![istarget "avr*"]} {
> + verbose "Skipping ${gdb_test_file_name}."
> + return
> +}
> +
> +# The __flash qualifier was added in GCC 4.7.
> +if {[test_compiler_info {gcc-[0-4]-[0-6]}]} {
> + verbose "Skipping ${gdb_test_file_name}."
> + return
> +}
> +
> +standard_testfile
> +if {[prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}]} {
> + return -1
> +}
> +
> +if ![runto [gdb_get_line_number "break here."]] {
> + untested "could not run to \"break here.\""
> + return -1
> +}
> +
> +gdb_test "print pointer_to_flash" \
> + " = $hex <data_in_flash> .*"
> +
> +gdb_breakpoint "pass_to_function"
> +gdb_continue_to_breakpoint "pass_to_function"
> +
> +gdb_test "print p" \
> + " = $hex <data_in_flash> .*"
> +
> +gdb_test "backtrace 1" \
> + "\#0 pass_to_function \\(p=$hex <data_in_flash> .*\\).*"
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Add support for the __flash qualifier on AVR
2014-07-08 10:54 [PATCH] Add support for the __flash qualifier on AVR Pierre Langlois
2014-07-15 10:36 ` [PING][PATCH] " Pierre Langlois
@ 2014-07-15 14:00 ` Joel Brobecker
2014-07-15 14:46 ` Pierre Langlois
2014-07-15 14:59 ` [PATCH v2] " Pierre Langlois
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Joel Brobecker @ 2014-07-15 14:00 UTC (permalink / raw)
To: Pierre Langlois; +Cc: gdb-patches
Hello,
> 2014-07-08 Pierre Langlois <pierre.langlois@embecosm.com>
>
> gdb/
> * avr-tdep.c (AVR_TYPE_ADDRESS_CLASS_FLASH): New macro.
> (AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH): Likewise.
> (avr_address_to_pointer): Check for AVR_TYPE_ADDRESS_CLASS_FLASH.
> (avr_pointer_to_address): Likewise.
> (avr_address_class_type_flags): New function.
> (avr_address_class_type_flags_to_name): Likewise.
> (avr_address_class_name_to_type_flags): Likewise.
> (avr_gdbarch_init): Set address_class_type_flags,
> address_class_type_flags_to_name and
> address_class_name_to_type_flags.
>
> gdb/testsuite/
> * gdb.arch/avr-flash-qualifer.c: New.
> * gdb.arch/avr-flash-qualifer.exp: New.
Sorry about the delay in reviewing this. Some comments below.
> +/* Map DW_AT_address_class attributes to a type_instance_flag_value. Note that
> + this attribute is only valid with pointer types and therefore the flag is set
> + to the pointer type and not its target type. */
Can you start the comment by saying that this function implements the
address_class_name_to_type_flags gdbarch method? You can then add
the comment above as a second paragraph, or perhaps as an implementation
comment inside the function body itself.
> +static int
> +avr_address_class_type_flags (int byte_size, int dwarf2_addr_class)
> +{
Please add a comment saying that this function implements the
address_class_type_flags gdbarch method. Generally speaking, all
new functions without exception should have an introductory comment.
> + // __flash qualifier
Please do not use C++-style comments for the moment.
> + if (dwarf2_addr_class == 1 && byte_size == 2)
> + return AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH;
> + return 0;
> +}
> +
> +/* Convert a type_instance_flag_value to an address space qualifier and
> + vice-versa. */
> +
> +static const char*
> +avr_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)
Same as above regarding the function's introductory comment. And also,
I'd prefer it if we avoided two functions being documented with
the same comment, as this opens the door for a function becoming
undocumented if the two functions somehow get separated.
> +{
> + if (type_flags & AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH)
> + return "flash";
> + else
> + return NULL;
> +}
> +
> +static int
> +avr_address_class_name_to_type_flags (struct gdbarch *gdbarch,
> + const char* name,
> + int *type_flags_ptr)
Missing introductory comment ("Implements the ...").
--
Joel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Add support for the __flash qualifier on AVR
2014-07-15 14:00 ` [PATCH] " Joel Brobecker
@ 2014-07-15 14:46 ` Pierre Langlois
0 siblings, 0 replies; 11+ messages in thread
From: Pierre Langlois @ 2014-07-15 14:46 UTC (permalink / raw)
To: Joel Brobecker; +Cc: gdb-patches
Hello Joel,
>
> Sorry about the delay in reviewing this. Some comments below.
>
That's OK, thank you very much for the review!
>> + // __flash qualifier
>
> Please do not use C++-style comments for the moment.
>
Oh dear, I'm sorry about this.
Thanks,
Pierre
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] Add support for the __flash qualifier on AVR
2014-07-08 10:54 [PATCH] Add support for the __flash qualifier on AVR Pierre Langlois
2014-07-15 10:36 ` [PING][PATCH] " Pierre Langlois
2014-07-15 14:00 ` [PATCH] " Joel Brobecker
@ 2014-07-15 14:59 ` Pierre Langlois
2014-07-15 15:01 ` Joel Brobecker
2014-07-15 16:42 ` [PUSHED] " Pierre Langlois
2014-07-21 16:02 ` [PATCH] " Pedro Alves
4 siblings, 1 reply; 11+ messages in thread
From: Pierre Langlois @ 2014-07-15 14:59 UTC (permalink / raw)
To: gdb-patches; +Cc: Pierre Langlois
The __flash qualifier is part of the named address spaces for AVR [1]. It allows
putting read-only data in the flash memory, normally reserved for code.
When used together with a pointer, the DW_AT_address_class attribute is set to 1
and allows GDB to detect that when it will be dereferenced, the data will be
loaded from the flash memory (with the LPM instruction).
We can now properly debug the following code:
~~~
const __flash char data_in_flash = 0xab;
int
main (void)
{
const __flash char *pointer_to_flash = &data_in_flash;
}
~~~
~~~
(gdb) print pointer_to_flash
$1 = 0x1e8 <data_in_flash> "\253"
(gdb) print/x *pointer_to_flash
$2 = 0xab
(gdb) x/x pointer_to_flash
0x1e8 <data_in_flash>: 0xXXXXXXab
~~~
Whereas previously, GDB would revert to the default address space which is RAM
and mapped in higher memory:
~~~
(gdb) print pointer_to_flash
$1 = 0x8001e8 ""
~~~
[1] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html
2014-07-08 Pierre Langlois <pierre.langlois@embecosm.com>
gdb/
* avr-tdep.c (AVR_TYPE_ADDRESS_CLASS_FLASH): New macro.
(AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH): Likewise.
(avr_address_to_pointer): Check for AVR_TYPE_ADDRESS_CLASS_FLASH.
(avr_pointer_to_address): Likewise.
(avr_address_class_type_flags): New function.
(avr_address_class_type_flags_to_name): Likewise.
(avr_address_class_name_to_type_flags): Likewise.
(avr_gdbarch_init): Set address_class_type_flags,
address_class_type_flags_to_name and
address_class_name_to_type_flags.
gdb/testsuite/
* gdb.arch/avr-flash-qualifer.c: New.
* gdb.arch/avr-flash-qualifer.exp: New.
---
gdb/avr-tdep.c | 85 ++++++++++++++++++++++++--
gdb/testsuite/gdb.arch/avr-flash-qualifier.c | 32 ++++++++++
gdb/testsuite/gdb.arch/avr-flash-qualifier.exp | 52 ++++++++++++++++
3 files changed, 164 insertions(+), 5 deletions(-)
create mode 100644 gdb/testsuite/gdb.arch/avr-flash-qualifier.c
create mode 100644 gdb/testsuite/gdb.arch/avr-flash-qualifier.exp
diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c
index 9b0bfaf..d3a0e87 100644
--- a/gdb/avr-tdep.c
+++ b/gdb/avr-tdep.c
@@ -71,6 +71,16 @@
/* Constants: prefixed with AVR_ to avoid name space clashes */
+/* Address space flags */
+
+/* We are assigning the TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1 to the flash address
+ space. */
+
+#define AVR_TYPE_ADDRESS_CLASS_FLASH TYPE_ADDRESS_CLASS_1
+#define AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH \
+ TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1
+
+
enum
{
AVR_REG_W = 24,
@@ -295,10 +305,19 @@ avr_address_to_pointer (struct gdbarch *gdbarch,
{
enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+ /* Is it a data address in flash? */
+ if (AVR_TYPE_ADDRESS_CLASS_FLASH (type))
+ {
+ /* A data address in flash is always byte addressed. */
+ store_unsigned_integer (buf, TYPE_LENGTH (type), byte_order,
+ avr_convert_iaddr_to_raw (addr));
+ }
/* Is it a code address? */
- if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
- || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
+ else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
+ || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
{
+ /* A code address, either a function pointer or the program counter, is
+ word (16 bits) addressed. */
store_unsigned_integer (buf, TYPE_LENGTH (type), byte_order,
avr_convert_iaddr_to_raw (addr >> 1));
}
@@ -318,10 +337,13 @@ avr_pointer_to_address (struct gdbarch *gdbarch,
CORE_ADDR addr
= extract_unsigned_integer (buf, TYPE_LENGTH (type), byte_order);
+ /* Is it a data address in flash? */
+ if (AVR_TYPE_ADDRESS_CLASS_FLASH (type))
+ return avr_make_iaddr (addr);
/* Is it a code address? */
- if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
- || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
- || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
+ else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
+ || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
+ || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
return avr_make_iaddr (addr << 1);
else
return avr_make_saddr (addr);
@@ -1342,6 +1364,53 @@ avr_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
return -1;
}
+/* Implementation of `address_class_type_flags' gdbarch method.
+
+ This method maps DW_AT_address_class attributes to a
+ type_instance_flag_value. */
+
+static int
+avr_address_class_type_flags (int byte_size, int dwarf2_addr_class)
+{
+ /* The value 1 of the DW_AT_address_class attribute corresponds to the __flash
+ qualifier. Note that this attribute is only valid with pointer types and
+ therefore the flag is set to the pointer type and not its target type. */
+ if (dwarf2_addr_class == 1 && byte_size == 2)
+ return AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH;
+ return 0;
+}
+
+/* Implementation of `address_class_type_flags_to_name' gdbarch method.
+
+ Convert a type_instance_flag_value to an address space qualifier. */
+
+static const char*
+avr_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)
+{
+ if (type_flags & AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH)
+ return "flash";
+ else
+ return NULL;
+}
+
+/* Implementation of `address_class_name_to_type_flags' gdbarch method.
+
+ Convert an address space qualifier to a type_instance_flag_value. */
+
+static int
+avr_address_class_name_to_type_flags (struct gdbarch *gdbarch,
+ const char* name,
+ int *type_flags_ptr)
+{
+ if (strcmp (name, "flash") == 0)
+ {
+ *type_flags_ptr = AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH;
+ return 1;
+ }
+ else
+ return 0;
+}
+
/* Initialize the gdbarch structure for the AVR's. */
static struct gdbarch *
@@ -1452,6 +1521,12 @@ avr_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
set_gdbarch_unwind_pc (gdbarch, avr_unwind_pc);
set_gdbarch_unwind_sp (gdbarch, avr_unwind_sp);
+ set_gdbarch_address_class_type_flags (gdbarch, avr_address_class_type_flags);
+ set_gdbarch_address_class_name_to_type_flags
+ (gdbarch, avr_address_class_name_to_type_flags);
+ set_gdbarch_address_class_type_flags_to_name
+ (gdbarch, avr_address_class_type_flags_to_name);
+
return gdbarch;
}
diff --git a/gdb/testsuite/gdb.arch/avr-flash-qualifier.c b/gdb/testsuite/gdb.arch/avr-flash-qualifier.c
new file mode 100644
index 0000000..4e5b451
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/avr-flash-qualifier.c
@@ -0,0 +1,32 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 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/>. */
+
+const __flash char data_in_flash = 0xab;
+
+static void
+pass_to_function (const __flash char *p)
+{
+}
+
+int
+main (void)
+{
+ const __flash char *pointer_to_flash = &data_in_flash;
+ /* break here. */
+ pass_to_function (&data_in_flash);
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/avr-flash-qualifier.exp b/gdb/testsuite/gdb.arch/avr-flash-qualifier.exp
new file mode 100644
index 0000000..e9ccb4f
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/avr-flash-qualifier.exp
@@ -0,0 +1,52 @@
+# Copyright 2014 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.
+#
+# Contributed by Pierre Langlois <pierre.langlois@embecosm.com>
+# Tests for the AVR __flash named address space qualifier.
+
+if {![istarget "avr*"]} {
+ verbose "Skipping ${gdb_test_file_name}."
+ return
+}
+
+# The __flash qualifier was added in GCC 4.7.
+if {[test_compiler_info {gcc-[0-4]-[0-6]}]} {
+ verbose "Skipping ${gdb_test_file_name}."
+ return
+}
+
+standard_testfile
+if {[prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}]} {
+ return -1
+}
+
+if ![runto [gdb_get_line_number "break here."]] {
+ untested "could not run to \"break here.\""
+ return -1
+}
+
+gdb_test "print pointer_to_flash" \
+ " = $hex <data_in_flash> .*"
+
+gdb_breakpoint "pass_to_function"
+gdb_continue_to_breakpoint "pass_to_function"
+
+gdb_test "print p" \
+ " = $hex <data_in_flash> .*"
+
+gdb_test "backtrace 1" \
+ "\#0 pass_to_function \\(p=$hex <data_in_flash> .*\\).*"
--
1.9.1
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] Add support for the __flash qualifier on AVR
2014-07-15 14:59 ` [PATCH v2] " Pierre Langlois
@ 2014-07-15 15:01 ` Joel Brobecker
2014-07-15 15:56 ` Pierre Langlois
0 siblings, 1 reply; 11+ messages in thread
From: Joel Brobecker @ 2014-07-15 15:01 UTC (permalink / raw)
To: Pierre Langlois; +Cc: gdb-patches
On Tue, Jul 15, 2014 at 03:48:03PM +0100, Pierre Langlois wrote:
> The __flash qualifier is part of the named address spaces for AVR [1]. It allows
> putting read-only data in the flash memory, normally reserved for code.
>
> When used together with a pointer, the DW_AT_address_class attribute is set to 1
> and allows GDB to detect that when it will be dereferenced, the data will be
> loaded from the flash memory (with the LPM instruction).
>
> We can now properly debug the following code:
[...]
> 2014-07-08 Pierre Langlois <pierre.langlois@embecosm.com>
>
> gdb/
> * avr-tdep.c (AVR_TYPE_ADDRESS_CLASS_FLASH): New macro.
> (AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH): Likewise.
> (avr_address_to_pointer): Check for AVR_TYPE_ADDRESS_CLASS_FLASH.
> (avr_pointer_to_address): Likewise.
> (avr_address_class_type_flags): New function.
> (avr_address_class_type_flags_to_name): Likewise.
> (avr_address_class_name_to_type_flags): Likewise.
> (avr_gdbarch_init): Set address_class_type_flags,
> address_class_type_flags_to_name and
> address_class_name_to_type_flags.
>
> gdb/testsuite/
> * gdb.arch/avr-flash-qualifer.c: New.
> * gdb.arch/avr-flash-qualifer.exp: New.
Thank you. This is now OK to commit with a couple of minor modifications.
First, would you mind reformatting the revision log so as to stay withing
76 characters instead of 80? This is because "git show" will indent that
revision log by 4 characters, thus causing formatting issues for those
of us who still use an 80 columns terminal.
Also:
> +static int
> +avr_address_class_type_flags (int byte_size, int dwarf2_addr_class)
> +{
> + /* The value 1 of the DW_AT_address_class attribute corresponds to the __flash
This line is too long (81 chars?)
> +int
> +main (void)
> +{
> + const __flash char *pointer_to_flash = &data_in_flash;
> + /* break here. */
Something I missed here. The project decided to try to follow the same
coding standards for our test programs as in the GDB code. So can you
skip a line after the variable declaration, and before the comment?
--
Joel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] Add support for the __flash qualifier on AVR
2014-07-15 15:01 ` Joel Brobecker
@ 2014-07-15 15:56 ` Pierre Langlois
0 siblings, 0 replies; 11+ messages in thread
From: Pierre Langlois @ 2014-07-15 15:56 UTC (permalink / raw)
To: Joel Brobecker; +Cc: gdb-patches
>> +static int
>> +avr_address_class_type_flags (int byte_size, int dwarf2_addr_class)
>> +{
>> + /* The value 1 of the DW_AT_address_class attribute corresponds to the __flash
>
> This line is too long (81 chars?)
>
This line is exactly 80 chars. I will reformat this comment with a limit of 74
chars, 80 chars do not really help readability in this case.
Thank you for your patience with my formatting issues! I will post the final
correct patch on this thread when I have pushed it.
Pierre
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PUSHED] Add support for the __flash qualifier on AVR
2014-07-08 10:54 [PATCH] Add support for the __flash qualifier on AVR Pierre Langlois
` (2 preceding siblings ...)
2014-07-15 14:59 ` [PATCH v2] " Pierre Langlois
@ 2014-07-15 16:42 ` Pierre Langlois
2014-07-21 16:02 ` [PATCH] " Pedro Alves
4 siblings, 0 replies; 11+ messages in thread
From: Pierre Langlois @ 2014-07-15 16:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Pierre Langlois
The __flash qualifier is part of the named address spaces for AVR [1]. It
allows putting read-only data in the flash memory, normally reserved for
code.
When used together with a pointer, the DW_AT_address_class attribute is set
to 1 and allows GDB to detect that when it will be dereferenced, the data
will be loaded from the flash memory (with the LPM instruction).
We can now properly debug the following code:
~~~
const __flash char data_in_flash = 0xab;
int
main (void)
{
const __flash char *pointer_to_flash = &data_in_flash;
}
~~~
~~~
(gdb) print pointer_to_flash
$1 = 0x1e8 <data_in_flash> "\253"
(gdb) print/x *pointer_to_flash
$2 = 0xab
(gdb) x/x pointer_to_flash
0x1e8 <data_in_flash>: 0xXXXXXXab
~~~
Whereas previously, GDB would revert to the default address space which is
RAM and mapped in higher memory:
~~~
(gdb) print pointer_to_flash
$1 = 0x8001e8 ""
~~~
[1] https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html
2014-07-15 Pierre Langlois <pierre.langlois@embecosm.com>
gdb/
* avr-tdep.c (AVR_TYPE_ADDRESS_CLASS_FLASH): New macro.
(AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH): Likewise.
(avr_address_to_pointer): Check for AVR_TYPE_ADDRESS_CLASS_FLASH.
(avr_pointer_to_address): Likewise.
(avr_address_class_type_flags): New function.
(avr_address_class_type_flags_to_name): Likewise.
(avr_address_class_name_to_type_flags): Likewise.
(avr_gdbarch_init): Set address_class_type_flags,
address_class_type_flags_to_name and
address_class_name_to_type_flags.
gdb/testsuite/
* gdb.arch/avr-flash-qualifer.c: New.
* gdb.arch/avr-flash-qualifer.exp: New.
---
gdb/ChangeLog | 13 ++++
gdb/avr-tdep.c | 86 ++++++++++++++++++++++++--
gdb/testsuite/ChangeLog | 5 ++
gdb/testsuite/gdb.arch/avr-flash-qualifier.c | 33 ++++++++++
gdb/testsuite/gdb.arch/avr-flash-qualifier.exp | 52 ++++++++++++++++
5 files changed, 184 insertions(+), 5 deletions(-)
create mode 100644 gdb/testsuite/gdb.arch/avr-flash-qualifier.c
create mode 100644 gdb/testsuite/gdb.arch/avr-flash-qualifier.exp
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2b6604b..6a05145 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,16 @@
+2014-07-15 Pierre Langlois <pierre.langlois@embecosm.com>
+
+ * avr-tdep.c (AVR_TYPE_ADDRESS_CLASS_FLASH): New macro.
+ (AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH): Likewise.
+ (avr_address_to_pointer): Check for AVR_TYPE_ADDRESS_CLASS_FLASH.
+ (avr_pointer_to_address): Likewise.
+ (avr_address_class_type_flags): New function.
+ (avr_address_class_type_flags_to_name): Likewise.
+ (avr_address_class_name_to_type_flags): Likewise.
+ (avr_gdbarch_init): Set address_class_type_flags,
+ address_class_type_flags_to_name and
+ address_class_name_to_type_flags.
+
2014-07-15 Pedro Alves <palves@redhat.com>
* linux-nat.c (kill_callback): Save errno and work with saved
diff --git a/gdb/avr-tdep.c b/gdb/avr-tdep.c
index 9b0bfaf..be0b543 100644
--- a/gdb/avr-tdep.c
+++ b/gdb/avr-tdep.c
@@ -71,6 +71,16 @@
/* Constants: prefixed with AVR_ to avoid name space clashes */
+/* Address space flags */
+
+/* We are assigning the TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1 to the flash address
+ space. */
+
+#define AVR_TYPE_ADDRESS_CLASS_FLASH TYPE_ADDRESS_CLASS_1
+#define AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH \
+ TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1
+
+
enum
{
AVR_REG_W = 24,
@@ -295,10 +305,19 @@ avr_address_to_pointer (struct gdbarch *gdbarch,
{
enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+ /* Is it a data address in flash? */
+ if (AVR_TYPE_ADDRESS_CLASS_FLASH (type))
+ {
+ /* A data address in flash is always byte addressed. */
+ store_unsigned_integer (buf, TYPE_LENGTH (type), byte_order,
+ avr_convert_iaddr_to_raw (addr));
+ }
/* Is it a code address? */
- if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
- || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
+ else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
+ || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
{
+ /* A code address, either a function pointer or the program counter, is
+ word (16 bits) addressed. */
store_unsigned_integer (buf, TYPE_LENGTH (type), byte_order,
avr_convert_iaddr_to_raw (addr >> 1));
}
@@ -318,10 +337,13 @@ avr_pointer_to_address (struct gdbarch *gdbarch,
CORE_ADDR addr
= extract_unsigned_integer (buf, TYPE_LENGTH (type), byte_order);
+ /* Is it a data address in flash? */
+ if (AVR_TYPE_ADDRESS_CLASS_FLASH (type))
+ return avr_make_iaddr (addr);
/* Is it a code address? */
- if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
- || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
- || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
+ else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
+ || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
+ || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
return avr_make_iaddr (addr << 1);
else
return avr_make_saddr (addr);
@@ -1342,6 +1364,54 @@ avr_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
return -1;
}
+/* Implementation of `address_class_type_flags' gdbarch method.
+
+ This method maps DW_AT_address_class attributes to a
+ type_instance_flag_value. */
+
+static int
+avr_address_class_type_flags (int byte_size, int dwarf2_addr_class)
+{
+ /* The value 1 of the DW_AT_address_class attribute corresponds to the
+ __flash qualifier. Note that this attribute is only valid with
+ pointer types and therefore the flag is set to the pointer type and
+ not its target type. */
+ if (dwarf2_addr_class == 1 && byte_size == 2)
+ return AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH;
+ return 0;
+}
+
+/* Implementation of `address_class_type_flags_to_name' gdbarch method.
+
+ Convert a type_instance_flag_value to an address space qualifier. */
+
+static const char*
+avr_address_class_type_flags_to_name (struct gdbarch *gdbarch, int type_flags)
+{
+ if (type_flags & AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH)
+ return "flash";
+ else
+ return NULL;
+}
+
+/* Implementation of `address_class_name_to_type_flags' gdbarch method.
+
+ Convert an address space qualifier to a type_instance_flag_value. */
+
+static int
+avr_address_class_name_to_type_flags (struct gdbarch *gdbarch,
+ const char* name,
+ int *type_flags_ptr)
+{
+ if (strcmp (name, "flash") == 0)
+ {
+ *type_flags_ptr = AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH;
+ return 1;
+ }
+ else
+ return 0;
+}
+
/* Initialize the gdbarch structure for the AVR's. */
static struct gdbarch *
@@ -1452,6 +1522,12 @@ avr_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
set_gdbarch_unwind_pc (gdbarch, avr_unwind_pc);
set_gdbarch_unwind_sp (gdbarch, avr_unwind_sp);
+ set_gdbarch_address_class_type_flags (gdbarch, avr_address_class_type_flags);
+ set_gdbarch_address_class_name_to_type_flags
+ (gdbarch, avr_address_class_name_to_type_flags);
+ set_gdbarch_address_class_type_flags_to_name
+ (gdbarch, avr_address_class_type_flags_to_name);
+
return gdbarch;
}
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b3dd34d..38357ea 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-07-15 Pierre Langlois <pierre.langlois@embecosm.com>
+
+ * gdb.arch/avr-flash-qualifer.c: New.
+ * gdb.arch/avr-flash-qualifer.exp: New.
+
2014-07-14 Pedro Alves <palves@redhat.com>
* gdb.base/paginate-after-ctrl-c-running.c: New file.
diff --git a/gdb/testsuite/gdb.arch/avr-flash-qualifier.c b/gdb/testsuite/gdb.arch/avr-flash-qualifier.c
new file mode 100644
index 0000000..7bfbe3a
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/avr-flash-qualifier.c
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 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/>. */
+
+const __flash char data_in_flash = 0xab;
+
+static void
+pass_to_function (const __flash char *p)
+{
+}
+
+int
+main (void)
+{
+ const __flash char *pointer_to_flash = &data_in_flash;
+
+ /* break here. */
+ pass_to_function (&data_in_flash);
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/avr-flash-qualifier.exp b/gdb/testsuite/gdb.arch/avr-flash-qualifier.exp
new file mode 100644
index 0000000..e9ccb4f
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/avr-flash-qualifier.exp
@@ -0,0 +1,52 @@
+# Copyright 2014 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.
+#
+# Contributed by Pierre Langlois <pierre.langlois@embecosm.com>
+# Tests for the AVR __flash named address space qualifier.
+
+if {![istarget "avr*"]} {
+ verbose "Skipping ${gdb_test_file_name}."
+ return
+}
+
+# The __flash qualifier was added in GCC 4.7.
+if {[test_compiler_info {gcc-[0-4]-[0-6]}]} {
+ verbose "Skipping ${gdb_test_file_name}."
+ return
+}
+
+standard_testfile
+if {[prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}]} {
+ return -1
+}
+
+if ![runto [gdb_get_line_number "break here."]] {
+ untested "could not run to \"break here.\""
+ return -1
+}
+
+gdb_test "print pointer_to_flash" \
+ " = $hex <data_in_flash> .*"
+
+gdb_breakpoint "pass_to_function"
+gdb_continue_to_breakpoint "pass_to_function"
+
+gdb_test "print p" \
+ " = $hex <data_in_flash> .*"
+
+gdb_test "backtrace 1" \
+ "\#0 pass_to_function \\(p=$hex <data_in_flash> .*\\).*"
--
1.9.1
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Add support for the __flash qualifier on AVR
2014-07-08 10:54 [PATCH] Add support for the __flash qualifier on AVR Pierre Langlois
` (3 preceding siblings ...)
2014-07-15 16:42 ` [PUSHED] " Pierre Langlois
@ 2014-07-21 16:02 ` Pedro Alves
2014-07-23 17:42 ` Pierre Langlois
4 siblings, 1 reply; 11+ messages in thread
From: Pedro Alves @ 2014-07-21 16:02 UTC (permalink / raw)
To: Pierre Langlois, gdb-patches
Hi Pierre,
On 07/08/2014 11:54 AM, Pierre Langlois wrote:
> The __flash qualifier is part of the named address spaces for AVR [1]. It allows
> putting read-only data in the flash memory, normally reserved for code.
>
> When used together with a pointer, the DW_AT_address_class attribute is set to 1
> and allows GDB to detect that when it will be dereferenced, the data will be
> loaded from the flash memory (with the LPM instruction).
>
Thanks. This looks good to me, with a couple nits pointed out
below addressed.
> +/* Address space flags */
> +
> +/* We are assigning the TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1 to the flash address
> + space. */
> /* Is it a code address? */
> - if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
> - || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
> + else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
> + || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
> {
> + /* A code address, either a function pointer or the program counter, is
> + word (16 bits) addressed. */
FYI, the "or the program counter" reference here looks a bit
confusion-inducing to me.
TYPE_CODE_FUNC -> function.
TYPE_CODE_METHOD -> C++ class member functions (methods).
I think you've added that, because the $pc register is of function
pointer type? I'd suggest just not bringing that up, as it was before.
> /* Is it a code address? */
> - if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
> - || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
> - || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
> + else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
> + || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
> + || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
Particularly since this bit didn't get that comment?
BTW, interesting/curious that TYPE_CODE_SPACE is handled
here but not in avr_address_to_pointer. Offhand, looks like a
bug, though off topic for this patch.
> return avr_make_iaddr (addr << 1);
> else
> return avr_make_saddr (addr);
> @@ -1342,6 +1364,45 @@ avr_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
> return -1;
> }
>
> +/* Map DW_AT_address_class attributes to a type_instance_flag_value. Note that
> + this attribute is only valid with pointer types and therefore the flag is set
> + to the pointer type and not its target type. */
> +
> +static int
> +avr_address_class_type_flags (int byte_size, int dwarf2_addr_class)
> +{
> + // __flash qualifier
Please use /**/-style comments, and write full sentences.
> + if (dwarf2_addr_class == 1 && byte_size == 2)
> + return AVR_TYPE_INSTANCE_FLAG_ADDRESS_CLASS_FLASH;
> + return 0;
> +}
> +
Thanks,
--
Pedro Alves
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Add support for the __flash qualifier on AVR
2014-07-21 16:02 ` [PATCH] " Pedro Alves
@ 2014-07-23 17:42 ` Pierre Langlois
2014-07-24 1:35 ` Pedro Alves
0 siblings, 1 reply; 11+ messages in thread
From: Pierre Langlois @ 2014-07-23 17:42 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
Hi Pedro,
On 21/07/14 16:45, Pedro Alves wrote:
> Hi Pierre,
>
> On 07/08/2014 11:54 AM, Pierre Langlois wrote:
>> The __flash qualifier is part of the named address spaces for AVR [1]. It allows
>> putting read-only data in the flash memory, normally reserved for code.
>>
>> When used together with a pointer, the DW_AT_address_class attribute is set to 1
>> and allows GDB to detect that when it will be dereferenced, the data will be
>> loaded from the flash memory (with the LPM instruction).
>>
>
> Thanks. This looks good to me, with a couple nits pointed out
> below addressed.
>
Thanks for looking at this. I have committed a better version of this patch
last week after review. So I just submitted another patch improving the
comments as suggested.
>> +/* Address space flags */
>> +
>> +/* We are assigning the TYPE_INSTANCE_FLAG_ADDRESS_CLASS_1 to the flash address
>> + space. */
>> /* Is it a code address? */
>> - if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
>> - || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
>> + else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
>> + || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD)
>> {
>> + /* A code address, either a function pointer or the program counter, is
>> + word (16 bits) addressed. */
>
> FYI, the "or the program counter" reference here looks a bit
> confusion-inducing to me.
>
> TYPE_CODE_FUNC -> function.
> TYPE_CODE_METHOD -> C++ class member functions (methods).
>
> I think you've added that, because the $pc register is of function
> pointer type? I'd suggest just not bringing that up, as it was before.
>
Yes you're right, I mentioned the $pc register because of its type. I
understand it's confusing now.
>
>> /* Is it a code address? */
>> - if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
>> - || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
>> - || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
>> + else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
>> + || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
>> + || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
>
> Particularly since this bit didn't get that comment?
>
> BTW, interesting/curious that TYPE_CODE_SPACE is handled
> here but not in avr_address_to_pointer. Offhand, looks like a
> bug, though off topic for this patch.
>
I am bit puzzled here but I suspect this was done on purpose. I'll submit a
patch so we can discuss it.
Thanks,
Pierre
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Add support for the __flash qualifier on AVR
2014-07-23 17:42 ` Pierre Langlois
@ 2014-07-24 1:35 ` Pedro Alves
0 siblings, 0 replies; 11+ messages in thread
From: Pedro Alves @ 2014-07-24 1:35 UTC (permalink / raw)
To: Pierre Langlois, gdb-patches
On 07/23/2014 05:52 PM, Pierre Langlois wrote:
> Hi Pedro,
>
> On 21/07/14 16:45, Pedro Alves wrote:
>> Hi Pierre,
>>
>> On 07/08/2014 11:54 AM, Pierre Langlois wrote:
>>> The __flash qualifier is part of the named address spaces for AVR [1]. It allows
>>> putting read-only data in the flash memory, normally reserved for code.
>>>
>>> When used together with a pointer, the DW_AT_address_class attribute is set to 1
>>> and allows GDB to detect that when it will be dereferenced, the data will be
>>> loaded from the flash memory (with the LPM instruction).
>>>
>>
>> Thanks. This looks good to me, with a couple nits pointed out
>> below addressed.
>>
>
> Thanks for looking at this. I have committed a better version of this patch
> last week after review. So I just submitted another patch improving the
> comments as suggested.
>
Whoops, I completely missed that. Sorry about that.
> Yes you're right, I mentioned the $pc register because of its type. I
> understand it's confusing now.
>
>>
>>> /* Is it a code address? */
>>> - if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
>>> - || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
>>> - || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
>>> + else if (TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_FUNC
>>> + || TYPE_CODE (TYPE_TARGET_TYPE (type)) == TYPE_CODE_METHOD
>>> + || TYPE_CODE_SPACE (TYPE_TARGET_TYPE (type)))
>>
>> Particularly since this bit didn't get that comment?
>>
>> BTW, interesting/curious that TYPE_CODE_SPACE is handled
>> here but not in avr_address_to_pointer. Offhand, looks like a
>> bug, though off topic for this patch.
>>
>
> I am bit puzzled here but I suspect this was done on purpose. I'll submit a
> patch so we can discuss it.
>
Thanks!
--
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 11+ messages in thread