[PATCH, v2] Add command to erase all flash memory regions

[PATCH, v2] Add command to erase all flash memory regions

[Luis Machado]

Changes in v2:

- Added NEWS entry.
- Fixed long lines.
- Address printing with paddress.

Years ago we contributed flash programming patches upstream.  The following
patch is a leftover one that complements that functionality by adding a new
command to erase all reported flash memory blocks.

The command is most useful when we're dealing with flash-enabled targets
(mostly bare-metal) and we need to reset the board for some reason.

The wiping out of flash memory regions should help the target come up with a
known clean state from which the user can load a new image and resume
debugging. It is convenient enough to do this from the debugger, and there is
also an MI command to expose this functionality to the IDE's.

Regression tested on Ubuntu 16.04 x86-64. No regressions.

Thoughts?

gdb/doc/ChangeLog:

2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	* gdb.texinfo (-target-flash-erase): New MI command description.
	(flash-erase): New CLI command description.

gdb/ChangeLog:

2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	* NEWS (New commands): Mention flash-erase.
	(New MI commands): Mention target-flash-erase.
	* mi/mi-cmds.c (mi_cmd_target_flash_erase): Added target-flash-erase MI
	command.
	* mi/mi-cmds.h (mi_cmd_target_flash_erase): New declaration.
	* mi/mi-main.c (mi_cmd_target_flash_erase): New function.
	* target-memory.c: Include "memattr.h" and "ui-out.h".
	* target.c (flash_erase_all_command): New function.
	(initialize_targets): Add new flash-erase command.
	* target.h (flash_erase_all_command): New declaration.
---
 gdb/NEWS            | 11 +++++++++++
 gdb/doc/gdb.texinfo | 32 ++++++++++++++++++++++++++++++++
 gdb/mi/mi-cmds.c    |  1 +
 gdb/mi/mi-cmds.h    |  1 +
 gdb/mi/mi-main.c    |  5 +++++
 gdb/target.c        | 45 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/target.h        |  3 +++
 7 files changed, 98 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index b976815..4d9effa 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -57,6 +57,17 @@ FreeBSD/mips			mips*-*-freebsd
 Synopsys ARC			arc*-*-elf32
 FreeBSD/mips			mips*-*-freebsd
 
+* New commands
+
+flash-erase
+  Erases all the flash memory regions reported by the target.
+
+* New MI commands
+
+target-flash-erase
+  Erases all the flash memory regions reported by the target.  This is
+  equivalent to the CLI command flash-erase.
+
 *** Changes in GDB 7.12
 
 * GDB and GDBserver now build with a C++ compiler by default.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 14628fa..24d6cf7 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -19626,6 +19626,16 @@ load programs into flash memory.
 @code{load} does not repeat if you press @key{RET} again after using it.
 @end table
 
+@table @code
+
+@kindex flash-erase
+@item flash-erase
+@anchor{flash-erase}
+
+Erases all known flash memory regions on the target.
+
+@end table
+
 @node Byte Order
 @section Choosing Target Byte Order
 
@@ -31863,6 +31873,28 @@ No equivalent.
 @subsubheading Example
 N.A.
 
+@subheading The @code{-target-flash-erase} Command
+@findex -target-flash-erase
+
+@subsubheading Synopsis
+
+@smallexample
+ -target-flash-erase
+@end smallexample
+
+Erases all known flash memory regions on the target.
+
+The corresponding @value{GDBN} command is @samp{flash-erase}.
+
+The output is a list of flash regions that have been erased, sorted by
+address/size.
+
+@smallexample
+(gdb)
+-target-flash-erase
+^done,erased-regions=@{address="0x0",size="262144"@}
+(gdb)
+@end smallexample
 
 @subheading The @code{-target-select} Command
 @findex -target-select
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index cdea008..abb70bd 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -147,6 +147,7 @@ static struct mi_cmd mi_cmds[] =
   DEF_MI_CMD_MI ("target-file-delete", mi_cmd_target_file_delete),
   DEF_MI_CMD_MI ("target-file-get", mi_cmd_target_file_get),
   DEF_MI_CMD_MI ("target-file-put", mi_cmd_target_file_put),
+  DEF_MI_CMD_MI ("target-flash-erase", mi_cmd_target_flash_erase),
   DEF_MI_CMD_CLI ("target-select", "target", 1),
   DEF_MI_CMD_MI ("thread-info", mi_cmd_thread_info),
   DEF_MI_CMD_MI ("thread-list-ids", mi_cmd_thread_list_ids),
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 8bd947b..d0906e6 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -93,6 +93,7 @@ extern mi_cmd_argv_ftype mi_cmd_target_detach;
 extern mi_cmd_argv_ftype mi_cmd_target_file_get;
 extern mi_cmd_argv_ftype mi_cmd_target_file_put;
 extern mi_cmd_argv_ftype mi_cmd_target_file_delete;
+extern mi_cmd_argv_ftype mi_cmd_target_flash_erase;
 extern mi_cmd_argv_ftype mi_cmd_thread_info;
 extern mi_cmd_argv_ftype mi_cmd_thread_list_ids;
 extern mi_cmd_argv_ftype mi_cmd_thread_select;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 22803cb..e432a01 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -552,6 +552,11 @@ mi_cmd_target_detach (char *command, char **argv, int argc)
   detach_command (NULL, 0);
 }
 
+void mi_cmd_target_flash_erase (char *command, char **argv, int argc)
+{
+  flash_erase_all_command (NULL, 0);
+}
+
 void
 mi_cmd_thread_select (char *command, char **argv, int argc)
 {
diff --git a/gdb/target.c b/gdb/target.c
index be7367c..02e80d2 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3943,6 +3943,48 @@ do_monitor_command (char *cmd,
   target_rcmd (cmd, gdb_stdtarg);
 }
 
+void
+flash_erase_all_command (char *cmd, int from_tty)
+{
+  int i;
+  int found_flash_region = 0;
+  struct mem_region *m;
+  struct mem_attrib *attrib;
+  struct cleanup *cleanup_tuple;
+  struct gdbarch *gdbarch = target_gdbarch ();
+
+  VEC(mem_region_s) * mem_regions = target_memory_map ();
+
+  /* Iterate over all memory regions.  */
+  for (i = 0; VEC_iterate (mem_region_s, mem_regions, i, m); i++)
+    {
+      attrib = &m->attrib;
+
+      /* Is this a flash memory region?  */
+      if (attrib->mode == MEM_FLASH)
+        {
+          found_flash_region = 1;
+          target_flash_erase (m->lo, m->hi - m->lo);
+          cleanup_tuple =
+	      make_cleanup_ui_out_tuple_begin_end (current_uiout,
+						   "erasing-regions");
+          current_uiout->message ("Erasing flash memory region at address ");
+          current_uiout->field_fmt ("address", "%s", paddress (gdbarch,
+								 m->lo));
+          current_uiout->message (", size = ");
+          current_uiout->field_fmt ("size", "%lu",
+				    (unsigned long) (m->hi - m->lo));
+          current_uiout->message ("\n");
+          do_cleanups (cleanup_tuple);
+        }
+    }
+
+  if (found_flash_region)
+    target_flash_done ();
+  else
+    current_uiout->message ("No flash memory regions found.\n");
+}
+
 /* Print the name of each layers of our target stack.  */
 
 static void
@@ -4233,6 +4275,9 @@ Otherwise, any attempt to interrupt or stop will be ignored."),
 			   set_target_permissions, NULL,
 			   &setlist, &showlist);
 
+  add_com ("flash-erase", no_class, flash_erase_all_command,
+           _("Erase all flash memory regions."));
+
   add_setshow_boolean_cmd ("auto-connect-native-target", class_support,
 			   &auto_connect_native_target, _("\
 Set whether GDB may automatically connect to the native target."), _("\
diff --git a/gdb/target.h b/gdb/target.h
index f2b9181..e239c2c 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1451,6 +1451,9 @@ extern int target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
    is returned.  */
 VEC(mem_region_s) *target_memory_map (void);
 
+/* Erases all flash memory regions on the target.  */
+void flash_erase_all_command (char *cmd, int from_tty);
+
 /* Erase the specified flash region.  */
 void target_flash_erase (ULONGEST address, LONGEST length);
 
-- 
2.7.4

Re: [PATCH, v2] Add command to erase all flash memory regions

[Eli Zaretskii]

> From: Luis Machado <lgustavo@codesourcery.com>
> Date: Mon, 9 Jan 2017 13:04:01 -0600
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index b976815..4d9effa 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -57,6 +57,17 @@ FreeBSD/mips			mips*-*-freebsd
>  Synopsys ARC			arc*-*-elf32
>  FreeBSD/mips			mips*-*-freebsd

This is OK, except...

> +* New commands
> +
> +flash-erase
> +  Erases all the flash memory regions reported by the target.
> +
> +* New MI commands
> +
> +target-flash-erase

The MI command is "-target-flash-erase", with the leading dash.

Otherwise, the documentation parts are okay.

Thanks.

Re: [PATCH, v2] Add command to erase all flash memory regions

[Luis Machado]

On 01/09/2017 01:22 PM, Eli Zaretskii wrote:
>> From: Luis Machado <lgustavo@codesourcery.com>
>> Date: Mon, 9 Jan 2017 13:04:01 -0600
>>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index b976815..4d9effa 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -57,6 +57,17 @@ FreeBSD/mips			mips*-*-freebsd
>>  Synopsys ARC			arc*-*-elf32
>>  FreeBSD/mips			mips*-*-freebsd
>
> This is OK, except...
>
>> +* New commands
>> +
>> +flash-erase
>> +  Erases all the flash memory regions reported by the target.
>> +
>> +* New MI commands
>> +
>> +target-flash-erase
>
> The MI command is "-target-flash-erase", with the leading dash.
>
> Otherwise, the documentation parts are okay.
>

Thanks. I'll fix this locally.

> Thanks.
>

Re: [PATCH, v2] Add command to erase all flash memory regions

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 4078 bytes --]

cc-ing the list now.

On 01/09/2017 03:32 PM, Simon Marchi wrote:
> On 17-01-09 02:04 PM, Luis Machado wrote:
>> Changes in v2:
>>
>> - Added NEWS entry.
>> - Fixed long lines.
>> - Address printing with paddress.
>>
>> Years ago we contributed flash programming patches upstream.  The following
>> patch is a leftover one that complements that functionality by adding a new
>> command to erase all reported flash memory blocks.
>>
>> The command is most useful when we're dealing with flash-enabled targets
>> (mostly bare-metal) and we need to reset the board for some reason.
>>
>> The wiping out of flash memory regions should help the target come up with a
>> known clean state from which the user can load a new image and resume
>> debugging. It is convenient enough to do this from the debugger, and there is
>> also an MI command to expose this functionality to the IDE's.
>>
>> Regression tested on Ubuntu 16.04 x86-64. No regressions.
>>
>> Thoughts?
>>
>> gdb/doc/ChangeLog:
>>
>> 2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
>> 	    Luis Machado  <lgustavo@codesourcery.com>
>>
>> 	* gdb.texinfo (-target-flash-erase): New MI command description.
>> 	(flash-erase): New CLI command description.
>>
>> gdb/ChangeLog:
>>
>> 2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
>> 	    Luis Machado  <lgustavo@codesourcery.com>
>>
>> 	* NEWS (New commands): Mention flash-erase.
>> 	(New MI commands): Mention target-flash-erase.
>> 	* mi/mi-cmds.c (mi_cmd_target_flash_erase): Added target-flash-erase MI
>> 	command.
>> 	* mi/mi-cmds.h (mi_cmd_target_flash_erase): New declaration.
>> 	* mi/mi-main.c (mi_cmd_target_flash_erase): New function.
>> 	* target-memory.c: Include "memattr.h" and "ui-out.h".
>
> I think this is outdated.
>

Indeed. I originally separated this piece from the one implementing 
memory verification. Fixed.

>> 	* target.c (flash_erase_all_command): New function.
>> 	(initialize_targets): Add new flash-erase command.
>> 	* target.h (flash_erase_all_command): New declaration.
>
> ...
>
>> @@ -31863,6 +31873,28 @@ No equivalent.
>>  @subsubheading Example
>>  N.A.
>>
>> +@subheading The @code{-target-flash-erase} Command
>> +@findex -target-flash-erase
>> +
>> +@subsubheading Synopsis
>> +
>> +@smallexample
>> + -target-flash-erase
>> +@end smallexample
>> +
>> +Erases all known flash memory regions on the target.
>> +
>> +The corresponding @value{GDBN} command is @samp{flash-erase}.
>> +
>> +The output is a list of flash regions that have been erased, sorted by
>> +address/size.
>
> What does it mean to be sorted by address/size?
>

It is actually an oversight. No sorting is done (the regions are shown 
in the same order as they are provided by the target).

In reality we only display the starting address and the size of each 
memory region.

>> @@ -3943,6 +3943,48 @@ do_monitor_command (char *cmd,
>>    target_rcmd (cmd, gdb_stdtarg);
>>  }
>>
>> +void
>> +flash_erase_all_command (char *cmd, int from_tty)
>> +{
>> +  int i;
>> +  int found_flash_region = 0;
>
> bool?
>

Fixed.

>> +  struct mem_region *m;
>> +  struct mem_attrib *attrib;
>> +  struct cleanup *cleanup_tuple;
>
> attrib and cleanup_tuple could be declared in the for loop.
>

Moved these within the loop.

>> +  struct gdbarch *gdbarch = target_gdbarch ();
>> +  VEC(mem_region_s) * mem_regions = target_memory_map ();
>
> Spurious space after *.
>

Fixed.

>> +
>> +  /* Iterate over all memory regions.  */
>> +  for (i = 0; VEC_iterate (mem_region_s, mem_regions, i, m); i++)
>
> Since we do C++ now, I guess we can declare the iteration variable in place.
>

Done.

>> @@ -4233,6 +4275,9 @@ Otherwise, any attempt to interrupt or stop will be ignored."),
>>  			   set_target_permissions, NULL,
>>  			   &setlist, &showlist);
>>
>> +  add_com ("flash-erase", no_class, flash_erase_all_command,
>
> To keep following the unwritten rule for naming command functions, could you name this one flash_erase_command?
>

I've fixed this as well. Thanks for spotting it.

> Otherwise, LGTM.
>

Updated patch attached.

Thanks,
Luis


[-- Attachment #2: 0001-Add-command-to-erase-all-flash-memory-regions.patch --]
[-- Type: text/x-patch, Size: 8004 bytes --]

[PATCH, v3] Add command to erase all flash memory regions

Changes in v3:

- Addressed Simon's comments on formatting.
- Adjusted command text in the manual entry.
- Fixed up ChangeLog.
- Renamed flash_erase_all_command to flash_erase_command.

Changes in v2:

- Added NEWS entry.
- Fixed long lines.
- Address printing with paddress.

Years ago we contributed flash programming patches upstream.  The following
patch is a leftover one that complements that functionality by adding a new
command to erase all reported flash memory blocks.

The command is most useful when we're dealing with flash-enabled targets
(mostly bare-metal) and we need to reset the board for some reason.

The wiping out of flash memory regions should help the target come up with a
known clean state from which the user can load a new image and resume
debugging. It is convenient enough to do this from the debugger, and there is
also an MI command to expose this functionality to the IDE's.

Regression tested on Ubuntu 16.04 x86-64. No regressions.

Thoughts?

gdb/doc/ChangeLog:

2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	* gdb.texinfo (-target-flash-erase): New MI command description.
	(flash-erase): New CLI command description.

gdb/ChangeLog:

2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	* NEWS (New commands): Mention flash-erase.
	(New MI commands): Mention target-flash-erase.
	* mi/mi-cmds.c (mi_cmd_target_flash_erase): Added target-flash-erase MI
	command.
	* mi/mi-cmds.h (mi_cmd_target_flash_erase): New declaration.
	* mi/mi-main.c (mi_cmd_target_flash_erase): New function.
	* target.c (flash_erase_command): New function.
	(initialize_targets): Add new flash-erase command.
	* target.h (flash_erase_command): New declaration.
---
 gdb/NEWS            | 11 +++++++++++
 gdb/doc/gdb.texinfo | 32 ++++++++++++++++++++++++++++++++
 gdb/mi/mi-cmds.c    |  1 +
 gdb/mi/mi-cmds.h    |  1 +
 gdb/mi/mi-main.c    |  5 +++++
 gdb/target.c        | 45 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/target.h        |  3 +++
 7 files changed, 98 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index b976815..21e8cd3 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -57,6 +57,17 @@ FreeBSD/mips			mips*-*-freebsd
 Synopsys ARC			arc*-*-elf32
 FreeBSD/mips			mips*-*-freebsd
 
+* New commands
+
+flash-erase
+  Erases all the flash memory regions reported by the target.
+
+* New MI commands
+
+-target-flash-erase
+  Erases all the flash memory regions reported by the target.  This is
+  equivalent to the CLI command flash-erase.
+
 *** Changes in GDB 7.12
 
 * GDB and GDBserver now build with a C++ compiler by default.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 14628fa..d562896 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -19626,6 +19626,16 @@ load programs into flash memory.
 @code{load} does not repeat if you press @key{RET} again after using it.
 @end table
 
+@table @code
+
+@kindex flash-erase
+@item flash-erase
+@anchor{flash-erase}
+
+Erases all known flash memory regions on the target.
+
+@end table
+
 @node Byte Order
 @section Choosing Target Byte Order
 
@@ -31863,6 +31873,28 @@ No equivalent.
 @subsubheading Example
 N.A.
 
+@subheading The @code{-target-flash-erase} Command
+@findex -target-flash-erase
+
+@subsubheading Synopsis
+
+@smallexample
+ -target-flash-erase
+@end smallexample
+
+Erases all known flash memory regions on the target.
+
+The corresponding @value{GDBN} command is @samp{flash-erase}.
+
+The output is a list of flash regions that have been erased, with starting
+addresses and memory region sizes.
+
+@smallexample
+(gdb)
+-target-flash-erase
+^done,erased-regions=@{address="0x0",size="262144"@}
+(gdb)
+@end smallexample
 
 @subheading The @code{-target-select} Command
 @findex -target-select
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index cdea008..abb70bd 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -147,6 +147,7 @@ static struct mi_cmd mi_cmds[] =
   DEF_MI_CMD_MI ("target-file-delete", mi_cmd_target_file_delete),
   DEF_MI_CMD_MI ("target-file-get", mi_cmd_target_file_get),
   DEF_MI_CMD_MI ("target-file-put", mi_cmd_target_file_put),
+  DEF_MI_CMD_MI ("target-flash-erase", mi_cmd_target_flash_erase),
   DEF_MI_CMD_CLI ("target-select", "target", 1),
   DEF_MI_CMD_MI ("thread-info", mi_cmd_thread_info),
   DEF_MI_CMD_MI ("thread-list-ids", mi_cmd_thread_list_ids),
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 8bd947b..d0906e6 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -93,6 +93,7 @@ extern mi_cmd_argv_ftype mi_cmd_target_detach;
 extern mi_cmd_argv_ftype mi_cmd_target_file_get;
 extern mi_cmd_argv_ftype mi_cmd_target_file_put;
 extern mi_cmd_argv_ftype mi_cmd_target_file_delete;
+extern mi_cmd_argv_ftype mi_cmd_target_flash_erase;
 extern mi_cmd_argv_ftype mi_cmd_thread_info;
 extern mi_cmd_argv_ftype mi_cmd_thread_list_ids;
 extern mi_cmd_argv_ftype mi_cmd_thread_select;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 22803cb..e432a01 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -552,6 +552,11 @@ mi_cmd_target_detach (char *command, char **argv, int argc)
   detach_command (NULL, 0);
 }
 
+void mi_cmd_target_flash_erase (char *command, char **argv, int argc)
+{
+  flash_erase_all_command (NULL, 0);
+}
+
 void
 mi_cmd_thread_select (char *command, char **argv, int argc)
 {
diff --git a/gdb/target.c b/gdb/target.c
index be7367c..4fb39c0 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3943,6 +3943,48 @@ do_monitor_command (char *cmd,
   target_rcmd (cmd, gdb_stdtarg);
 }
 
+void
+flash_erase_command (char *cmd, int from_tty)
+{
+  bool found_flash_region = false;
+  struct mem_region *m;
+  struct gdbarch *gdbarch = target_gdbarch ();
+
+  VEC(mem_region_s) *mem_regions = target_memory_map ();
+
+  /* Iterate over all memory regions.  */
+  for (int i = 0; VEC_iterate (mem_region_s, mem_regions, i, m); i++)
+    {
+      struct mem_attrib *attrib;
+      attrib = &m->attrib;
+
+      /* Is this a flash memory region?  */
+      if (attrib->mode == MEM_FLASH)
+        {
+	  struct cleanup *cleanup_tuple;
+
+          found_flash_region = true;
+          target_flash_erase (m->lo, m->hi - m->lo);
+          cleanup_tuple =
+	      make_cleanup_ui_out_tuple_begin_end (current_uiout,
+						   "erasing-regions");
+          current_uiout->message ("Erasing flash memory region at address ");
+          current_uiout->field_fmt ("address", "%s", paddress (gdbarch,
+								 m->lo));
+          current_uiout->message (", size = ");
+          current_uiout->field_fmt ("size", "%lu",
+				    (unsigned long) (m->hi - m->lo));
+          current_uiout->message ("\n");
+          do_cleanups (cleanup_tuple);
+        }
+    }
+
+  if (found_flash_region)
+    target_flash_done ();
+  else
+    current_uiout->message ("No flash memory regions found.\n");
+}
+
 /* Print the name of each layers of our target stack.  */
 
 static void
@@ -4233,6 +4275,9 @@ Otherwise, any attempt to interrupt or stop will be ignored."),
 			   set_target_permissions, NULL,
 			   &setlist, &showlist);
 
+  add_com ("flash-erase", no_class, flash_erase_command,
+           _("Erase all flash memory regions."));
+
   add_setshow_boolean_cmd ("auto-connect-native-target", class_support,
 			   &auto_connect_native_target, _("\
 Set whether GDB may automatically connect to the native target."), _("\
diff --git a/gdb/target.h b/gdb/target.h
index f2b9181..8df117e 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1451,6 +1451,9 @@ extern int target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
    is returned.  */
 VEC(mem_region_s) *target_memory_map (void);
 
+/* Erases all flash memory regions on the target.  */
+void flash_erase_command (char *cmd, int from_tty);
+
 /* Erase the specified flash region.  */
 void target_flash_erase (ULONGEST address, LONGEST length);
 
-- 
2.7.4

Re: [PATCH, v2] Add command to erase all flash memory regions

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 4497 bytes --]

Actually, there was one call to flash_erase_all_command that needed to 
be updated in mi/mi-main.c. Fixed patch attached.

On 01/09/2017 04:14 PM, Luis Machado wrote:
> cc-ing the list now.
>
> On 01/09/2017 03:32 PM, Simon Marchi wrote:
>> On 17-01-09 02:04 PM, Luis Machado wrote:
>>> Changes in v2:
>>>
>>> - Added NEWS entry.
>>> - Fixed long lines.
>>> - Address printing with paddress.
>>>
>>> Years ago we contributed flash programming patches upstream.  The
>>> following
>>> patch is a leftover one that complements that functionality by adding
>>> a new
>>> command to erase all reported flash memory blocks.
>>>
>>> The command is most useful when we're dealing with flash-enabled targets
>>> (mostly bare-metal) and we need to reset the board for some reason.
>>>
>>> The wiping out of flash memory regions should help the target come up
>>> with a
>>> known clean state from which the user can load a new image and resume
>>> debugging. It is convenient enough to do this from the debugger, and
>>> there is
>>> also an MI command to expose this functionality to the IDE's.
>>>
>>> Regression tested on Ubuntu 16.04 x86-64. No regressions.
>>>
>>> Thoughts?
>>>
>>> gdb/doc/ChangeLog:
>>>
>>> 2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
>>>         Luis Machado  <lgustavo@codesourcery.com>
>>>
>>>     * gdb.texinfo (-target-flash-erase): New MI command description.
>>>     (flash-erase): New CLI command description.
>>>
>>> gdb/ChangeLog:
>>>
>>> 2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
>>>         Luis Machado  <lgustavo@codesourcery.com>
>>>
>>>     * NEWS (New commands): Mention flash-erase.
>>>     (New MI commands): Mention target-flash-erase.
>>>     * mi/mi-cmds.c (mi_cmd_target_flash_erase): Added
>>> target-flash-erase MI
>>>     command.
>>>     * mi/mi-cmds.h (mi_cmd_target_flash_erase): New declaration.
>>>     * mi/mi-main.c (mi_cmd_target_flash_erase): New function.
>>>     * target-memory.c: Include "memattr.h" and "ui-out.h".
>>
>> I think this is outdated.
>>
>
> Indeed. I originally separated this piece from the one implementing
> memory verification. Fixed.
>
>>>     * target.c (flash_erase_all_command): New function.
>>>     (initialize_targets): Add new flash-erase command.
>>>     * target.h (flash_erase_all_command): New declaration.
>>
>> ...
>>
>>> @@ -31863,6 +31873,28 @@ No equivalent.
>>>  @subsubheading Example
>>>  N.A.
>>>
>>> +@subheading The @code{-target-flash-erase} Command
>>> +@findex -target-flash-erase
>>> +
>>> +@subsubheading Synopsis
>>> +
>>> +@smallexample
>>> + -target-flash-erase
>>> +@end smallexample
>>> +
>>> +Erases all known flash memory regions on the target.
>>> +
>>> +The corresponding @value{GDBN} command is @samp{flash-erase}.
>>> +
>>> +The output is a list of flash regions that have been erased, sorted by
>>> +address/size.
>>
>> What does it mean to be sorted by address/size?
>>
>
> It is actually an oversight. No sorting is done (the regions are shown
> in the same order as they are provided by the target).
>
> In reality we only display the starting address and the size of each
> memory region.
>
>>> @@ -3943,6 +3943,48 @@ do_monitor_command (char *cmd,
>>>    target_rcmd (cmd, gdb_stdtarg);
>>>  }
>>>
>>> +void
>>> +flash_erase_all_command (char *cmd, int from_tty)
>>> +{
>>> +  int i;
>>> +  int found_flash_region = 0;
>>
>> bool?
>>
>
> Fixed.
>
>>> +  struct mem_region *m;
>>> +  struct mem_attrib *attrib;
>>> +  struct cleanup *cleanup_tuple;
>>
>> attrib and cleanup_tuple could be declared in the for loop.
>>
>
> Moved these within the loop.
>
>>> +  struct gdbarch *gdbarch = target_gdbarch ();
>>> +  VEC(mem_region_s) * mem_regions = target_memory_map ();
>>
>> Spurious space after *.
>>
>
> Fixed.
>
>>> +
>>> +  /* Iterate over all memory regions.  */
>>> +  for (i = 0; VEC_iterate (mem_region_s, mem_regions, i, m); i++)
>>
>> Since we do C++ now, I guess we can declare the iteration variable in
>> place.
>>
>
> Done.
>
>>> @@ -4233,6 +4275,9 @@ Otherwise, any attempt to interrupt or stop
>>> will be ignored."),
>>>                 set_target_permissions, NULL,
>>>                 &setlist, &showlist);
>>>
>>> +  add_com ("flash-erase", no_class, flash_erase_all_command,
>>
>> To keep following the unwritten rule for naming command functions,
>> could you name this one flash_erase_command?
>>
>
> I've fixed this as well. Thanks for spotting it.
>
>> Otherwise, LGTM.
>>
>
> Updated patch attached.
>
> Thanks,
> Luis
>


[-- Attachment #2: 0001-Add-command-to-erase-all-flash-memory-regions.patch --]
[-- Type: text/x-patch, Size: 7999 bytes --]

[PATCH, v3] Add command to erase all flash memory regions

Changes in v3:

- Addressed Simon's comments on formatting.
- Adjusted command text in the manual entry.
- Fixed up ChangeLog.
- Renamed flash_erase_all_command to flash_erase_command.

Changes in v2:

- Added NEWS entry.
- Fixed long lines.
- Address printing with paddress.

Years ago we contributed flash programming patches upstream.  The following
patch is a leftover one that complements that functionality by adding a new
command to erase all reported flash memory blocks.

The command is most useful when we're dealing with flash-enabled targets
(mostly bare-metal) and we need to reset the board for some reason.

The wiping out of flash memory regions should help the target come up with a
known clean state from which the user can load a new image and resume
debugging. It is convenient enough to do this from the debugger, and there is
also an MI command to expose this functionality to the IDE's.

Regression tested on Ubuntu 16.04 x86-64. No regressions.

Thoughts?

gdb/doc/ChangeLog:

2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	* gdb.texinfo (-target-flash-erase): New MI command description.
	(flash-erase): New CLI command description.

gdb/ChangeLog:

2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	* NEWS (New commands): Mention flash-erase.
	(New MI commands): Mention target-flash-erase.
	* mi/mi-cmds.c (mi_cmd_target_flash_erase): Added target-flash-erase MI
	command.
	* mi/mi-cmds.h (mi_cmd_target_flash_erase): New declaration.
	* mi/mi-main.c (mi_cmd_target_flash_erase): New function.
	* target.c (flash_erase_command): New function.
	(initialize_targets): Add new flash-erase command.
	* target.h (flash_erase_command): New declaration.
---
 gdb/NEWS            | 11 +++++++++++
 gdb/doc/gdb.texinfo | 32 ++++++++++++++++++++++++++++++++
 gdb/mi/mi-cmds.c    |  1 +
 gdb/mi/mi-cmds.h    |  1 +
 gdb/mi/mi-main.c    |  5 +++++
 gdb/target.c        | 45 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/target.h        |  3 +++
 7 files changed, 98 insertions(+)

diff --git a/gdb/NEWS b/gdb/NEWS
index b976815..21e8cd3 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -57,6 +57,17 @@ FreeBSD/mips			mips*-*-freebsd
 Synopsys ARC			arc*-*-elf32
 FreeBSD/mips			mips*-*-freebsd
 
+* New commands
+
+flash-erase
+  Erases all the flash memory regions reported by the target.
+
+* New MI commands
+
+-target-flash-erase
+  Erases all the flash memory regions reported by the target.  This is
+  equivalent to the CLI command flash-erase.
+
 *** Changes in GDB 7.12
 
 * GDB and GDBserver now build with a C++ compiler by default.
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 14628fa..d562896 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -19626,6 +19626,16 @@ load programs into flash memory.
 @code{load} does not repeat if you press @key{RET} again after using it.
 @end table
 
+@table @code
+
+@kindex flash-erase
+@item flash-erase
+@anchor{flash-erase}
+
+Erases all known flash memory regions on the target.
+
+@end table
+
 @node Byte Order
 @section Choosing Target Byte Order
 
@@ -31863,6 +31873,28 @@ No equivalent.
 @subsubheading Example
 N.A.
 
+@subheading The @code{-target-flash-erase} Command
+@findex -target-flash-erase
+
+@subsubheading Synopsis
+
+@smallexample
+ -target-flash-erase
+@end smallexample
+
+Erases all known flash memory regions on the target.
+
+The corresponding @value{GDBN} command is @samp{flash-erase}.
+
+The output is a list of flash regions that have been erased, with starting
+addresses and memory region sizes.
+
+@smallexample
+(gdb)
+-target-flash-erase
+^done,erased-regions=@{address="0x0",size="262144"@}
+(gdb)
+@end smallexample
 
 @subheading The @code{-target-select} Command
 @findex -target-select
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index cdea008..abb70bd 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -147,6 +147,7 @@ static struct mi_cmd mi_cmds[] =
   DEF_MI_CMD_MI ("target-file-delete", mi_cmd_target_file_delete),
   DEF_MI_CMD_MI ("target-file-get", mi_cmd_target_file_get),
   DEF_MI_CMD_MI ("target-file-put", mi_cmd_target_file_put),
+  DEF_MI_CMD_MI ("target-flash-erase", mi_cmd_target_flash_erase),
   DEF_MI_CMD_CLI ("target-select", "target", 1),
   DEF_MI_CMD_MI ("thread-info", mi_cmd_thread_info),
   DEF_MI_CMD_MI ("thread-list-ids", mi_cmd_thread_list_ids),
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 8bd947b..d0906e6 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -93,6 +93,7 @@ extern mi_cmd_argv_ftype mi_cmd_target_detach;
 extern mi_cmd_argv_ftype mi_cmd_target_file_get;
 extern mi_cmd_argv_ftype mi_cmd_target_file_put;
 extern mi_cmd_argv_ftype mi_cmd_target_file_delete;
+extern mi_cmd_argv_ftype mi_cmd_target_flash_erase;
 extern mi_cmd_argv_ftype mi_cmd_thread_info;
 extern mi_cmd_argv_ftype mi_cmd_thread_list_ids;
 extern mi_cmd_argv_ftype mi_cmd_thread_select;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 22803cb..7a75d3b 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -552,6 +552,11 @@ mi_cmd_target_detach (char *command, char **argv, int argc)
   detach_command (NULL, 0);
 }
 
+void mi_cmd_target_flash_erase (char *command, char **argv, int argc)
+{
+  flash_erase_command (NULL, 0);
+}
+
 void
 mi_cmd_thread_select (char *command, char **argv, int argc)
 {
diff --git a/gdb/target.c b/gdb/target.c
index be7367c..4fb39c0 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3943,6 +3943,48 @@ do_monitor_command (char *cmd,
   target_rcmd (cmd, gdb_stdtarg);
 }
 
+void
+flash_erase_command (char *cmd, int from_tty)
+{
+  bool found_flash_region = false;
+  struct mem_region *m;
+  struct gdbarch *gdbarch = target_gdbarch ();
+
+  VEC(mem_region_s) *mem_regions = target_memory_map ();
+
+  /* Iterate over all memory regions.  */
+  for (int i = 0; VEC_iterate (mem_region_s, mem_regions, i, m); i++)
+    {
+      struct mem_attrib *attrib;
+      attrib = &m->attrib;
+
+      /* Is this a flash memory region?  */
+      if (attrib->mode == MEM_FLASH)
+        {
+	  struct cleanup *cleanup_tuple;
+
+          found_flash_region = true;
+          target_flash_erase (m->lo, m->hi - m->lo);
+          cleanup_tuple =
+	      make_cleanup_ui_out_tuple_begin_end (current_uiout,
+						   "erasing-regions");
+          current_uiout->message ("Erasing flash memory region at address ");
+          current_uiout->field_fmt ("address", "%s", paddress (gdbarch,
+								 m->lo));
+          current_uiout->message (", size = ");
+          current_uiout->field_fmt ("size", "%lu",
+				    (unsigned long) (m->hi - m->lo));
+          current_uiout->message ("\n");
+          do_cleanups (cleanup_tuple);
+        }
+    }
+
+  if (found_flash_region)
+    target_flash_done ();
+  else
+    current_uiout->message ("No flash memory regions found.\n");
+}
+
 /* Print the name of each layers of our target stack.  */
 
 static void
@@ -4233,6 +4275,9 @@ Otherwise, any attempt to interrupt or stop will be ignored."),
 			   set_target_permissions, NULL,
 			   &setlist, &showlist);
 
+  add_com ("flash-erase", no_class, flash_erase_command,
+           _("Erase all flash memory regions."));
+
   add_setshow_boolean_cmd ("auto-connect-native-target", class_support,
 			   &auto_connect_native_target, _("\
 Set whether GDB may automatically connect to the native target."), _("\
diff --git a/gdb/target.h b/gdb/target.h
index f2b9181..8df117e 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1451,6 +1451,9 @@ extern int target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
    is returned.  */
 VEC(mem_region_s) *target_memory_map (void);
 
+/* Erases all flash memory regions on the target.  */
+void flash_erase_command (char *cmd, int from_tty);
+
 /* Erase the specified flash region.  */
 void target_flash_erase (ULONGEST address, LONGEST length);
 
-- 
2.7.4

Re: [PATCH, v2] Add command to erase all flash memory regions

[Pedro Alves]

On 01/09/2017 07:04 PM, Luis Machado wrote:
> Changes in v2:
> 
> - Added NEWS entry.
> - Fixed long lines.
> - Address printing with paddress.
> 
> Years ago we contributed flash programming patches upstream.  The following
> patch is a leftover one that complements that functionality by adding a new
> command to erase all reported flash memory blocks.
> 
> The command is most useful when we're dealing with flash-enabled targets
> (mostly bare-metal) and we need to reset the board for some reason.
> 
> The wiping out of flash memory regions should help the target come up with a
> known clean state from which the user can load a new image and resume
> debugging. It is convenient enough to do this from the debugger, and there is
> also an MI command to expose this functionality to the IDE's.
> 
> Regression tested on Ubuntu 16.04 x86-64. No regressions.
> 
> Thoughts?
> 
> gdb/doc/ChangeLog:
> 
> 2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
> 	    Luis Machado  <lgustavo@codesourcery.com>
> 
> 	* gdb.texinfo (-target-flash-erase): New MI command description.
> 	(flash-erase): New CLI command description.
> 
> gdb/ChangeLog:
> 
> 2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
> 	    Luis Machado  <lgustavo@codesourcery.com>
> 
> 	* NEWS (New commands): Mention flash-erase.
> 	(New MI commands): Mention target-flash-erase.
> 	* mi/mi-cmds.c (mi_cmd_target_flash_erase): Added target-flash-erase MI
> 	command.
> 	* mi/mi-cmds.h (mi_cmd_target_flash_erase): New declaration.
> 	* mi/mi-main.c (mi_cmd_target_flash_erase): New function.
> 	* target-memory.c: Include "memattr.h" and "ui-out.h".
> 	* target.c (flash_erase_all_command): New function.
> 	(initialize_targets): Add new flash-erase command.
> 	* target.h (flash_erase_all_command): New declaration.
> ---
>  gdb/NEWS            | 11 +++++++++++
>  gdb/doc/gdb.texinfo | 32 ++++++++++++++++++++++++++++++++
>  gdb/mi/mi-cmds.c    |  1 +
>  gdb/mi/mi-cmds.h    |  1 +
>  gdb/mi/mi-main.c    |  5 +++++
>  gdb/target.c        | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  gdb/target.h        |  3 +++
>  7 files changed, 98 insertions(+)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index b976815..4d9effa 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -57,6 +57,17 @@ FreeBSD/mips			mips*-*-freebsd
>  Synopsys ARC			arc*-*-elf32
>  FreeBSD/mips			mips*-*-freebsd
>  
> +* New commands
> +
> +flash-erase
> +  Erases all the flash memory regions reported by the target.
> +
> +* New MI commands
> +
> +target-flash-erase
> +  Erases all the flash memory regions reported by the target.  This is
> +  equivalent to the CLI command flash-erase.
> +
>  *** Changes in GDB 7.12
>  
>  * GDB and GDBserver now build with a C++ compiler by default.
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 14628fa..24d6cf7 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -19626,6 +19626,16 @@ load programs into flash memory.
>  @code{load} does not repeat if you press @key{RET} again after using it.
>  @end table
>  
> +@table @code
> +
> +@kindex flash-erase
> +@item flash-erase
> +@anchor{flash-erase}
> +
> +Erases all known flash memory regions on the target.
> +
> +@end table
> +
>  @node Byte Order
>  @section Choosing Target Byte Order
>  
> @@ -31863,6 +31873,28 @@ No equivalent.
>  @subsubheading Example
>  N.A.
>  
> +@subheading The @code{-target-flash-erase} Command
> +@findex -target-flash-erase
> +
> +@subsubheading Synopsis
> +
> +@smallexample
> + -target-flash-erase
> +@end smallexample
> +
> +Erases all known flash memory regions on the target.
> +
> +The corresponding @value{GDBN} command is @samp{flash-erase}.
> +
> +The output is a list of flash regions that have been erased, sorted by
> +address/size.

Are they really sorted by size?

> +
> +@smallexample
> +(gdb)
> +-target-flash-erase
> +^done,erased-regions=@{address="0x0",size="262144"@}
> +(gdb)
> +@end smallexample
>  
>  @subheading The @code{-target-select} Command
>  @findex -target-select
> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> index cdea008..abb70bd 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -147,6 +147,7 @@ static struct mi_cmd mi_cmds[] =
>    DEF_MI_CMD_MI ("target-file-delete", mi_cmd_target_file_delete),
>    DEF_MI_CMD_MI ("target-file-get", mi_cmd_target_file_get),
>    DEF_MI_CMD_MI ("target-file-put", mi_cmd_target_file_put),
> +  DEF_MI_CMD_MI ("target-flash-erase", mi_cmd_target_flash_erase),
>    DEF_MI_CMD_CLI ("target-select", "target", 1),
>    DEF_MI_CMD_MI ("thread-info", mi_cmd_thread_info),
>    DEF_MI_CMD_MI ("thread-list-ids", mi_cmd_thread_list_ids),
> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index 8bd947b..d0906e6 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -93,6 +93,7 @@ extern mi_cmd_argv_ftype mi_cmd_target_detach;
>  extern mi_cmd_argv_ftype mi_cmd_target_file_get;
>  extern mi_cmd_argv_ftype mi_cmd_target_file_put;
>  extern mi_cmd_argv_ftype mi_cmd_target_file_delete;
> +extern mi_cmd_argv_ftype mi_cmd_target_flash_erase;
>  extern mi_cmd_argv_ftype mi_cmd_thread_info;
>  extern mi_cmd_argv_ftype mi_cmd_thread_list_ids;
>  extern mi_cmd_argv_ftype mi_cmd_thread_select;
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 22803cb..e432a01 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -552,6 +552,11 @@ mi_cmd_target_detach (char *command, char **argv, int argc)
>    detach_command (NULL, 0);
>  }
>  
> +void mi_cmd_target_flash_erase (char *command, char **argv, int argc)

Like break after void.

> +{
> +  flash_erase_all_command (NULL, 0);
> +}
> +
>  void
>  mi_cmd_thread_select (char *command, char **argv, int argc)
>  {
> diff --git a/gdb/target.c b/gdb/target.c
> index be7367c..02e80d2 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3943,6 +3943,48 @@ do_monitor_command (char *cmd,
>    target_rcmd (cmd, gdb_stdtarg);
>  }
>  
> +void
> +flash_erase_all_command (char *cmd, int from_tty)

Comment.

> +{
> +  int i;
> +  int found_flash_region = 0;

Use bool.  Add comment explaining why we need it?

> +  struct mem_region *m;
> +  struct mem_attrib *attrib;
> +  struct cleanup *cleanup_tuple;
> +  struct gdbarch *gdbarch = target_gdbarch ();


And move all these to the scope that needs them.  Maybe
even declare them on first use.

> +
> +  VEC(mem_region_s) * mem_regions = target_memory_map ();
> +
> +  /* Iterate over all memory regions.  */
> +  for (i = 0; VEC_iterate (mem_region_s, mem_regions, i, m); i++)
> +    {
> +      attrib = &m->attrib;
> +
> +      /* Is this a flash memory region?  */
> +      if (attrib->mode == MEM_FLASH)
> +        {
> +          found_flash_region = 1;
> +          target_flash_erase (m->lo, m->hi - m->lo);
> +          cleanup_tuple =

= goes on next line.

> +	      make_cleanup_ui_out_tuple_begin_end (current_uiout,
> +						   "erasing-regions");
> +          current_uiout->message ("Erasing flash memory region at address ");
> +          current_uiout->field_fmt ("address", "%s", paddress (gdbarch,
> +								 m->lo));
> +          current_uiout->message (", size = ");
> +          current_uiout->field_fmt ("size", "%lu",
> +				    (unsigned long) (m->hi - m->lo));

Pedantically, long is the wrong type to use, since it'll be 32-bit
on 64-bit Windows.  Use pulongest?

> +          current_uiout->message ("\n");
> +          do_cleanups (cleanup_tuple);
> +        }
> +    }
> +
> +  if (found_flash_region)
> +    target_flash_done ();

I was wondering how this is supposed to work if the command
is interrupted w/ ctrl-c midway.  If the command is interrupted before
sending the vFlashDone RSP command, then it seems to me that the stub
is doing to get confused, since it seems there's no RSP command to
mean "I'm starting a batch of flash operations from scratch, forget
any stale list of pending vFlashWrite/vFlashErase ops you may have.
Am I missing something that prevents that from happening?

> +  else
> +    current_uiout->message ("No flash memory regions found.\n");

i18n ?


> +-target-flash-erase
> +^done,erased-regions=@{address="0x0",size="262144"@}
> +(gdb)
> +@end smallexample

...

> +	      make_cleanup_ui_out_tuple_begin_end (current_uiout,
> +						   "erasing-regions");

"erased-regions" vs "erasing-regions"...

Also, should be size be in hex?  I.e,. 262144 vs 0x40000.
That would likely be a bit easier to debug when things go wrong.

Thanks,
Pedro Alves

Re: [PATCH, v2] Add command to erase all flash memory regions

[Pedro Alves]

>>>> +The output is a list of flash regions that have been erased, sorted by
>>>> +address/size.
>>>
>>> What does it mean to be sorted by address/size?
>>>
>>
>> It is actually an oversight. No sorting is done (the regions are shown
>> in the same order as they are provided by the target).

AFAICS, target_memory_map does a qsort on address.
But it's probably better to not commit to any ordering.

Thanks,
Pedro Alves

Re: [PATCH, v2] Add command to erase all flash memory regions

[Luis Machado]

On 01/10/2017 12:22 PM, Pedro Alves wrote:
> On 01/09/2017 07:04 PM, Luis Machado wrote:
>> Changes in v2:
>>
>> - Added NEWS entry.
>> - Fixed long lines.
>> - Address printing with paddress.
>>
>> Years ago we contributed flash programming patches upstream.  The following
>> patch is a leftover one that complements that functionality by adding a new
>> command to erase all reported flash memory blocks.
>>
>> The command is most useful when we're dealing with flash-enabled targets
>> (mostly bare-metal) and we need to reset the board for some reason.
>>
>> The wiping out of flash memory regions should help the target come up with a
>> known clean state from which the user can load a new image and resume
>> debugging. It is convenient enough to do this from the debugger, and there is
>> also an MI command to expose this functionality to the IDE's.
>>
>> Regression tested on Ubuntu 16.04 x86-64. No regressions.
>>
>> Thoughts?
>>
>> gdb/doc/ChangeLog:
>>
>> 2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
>> 	    Luis Machado  <lgustavo@codesourcery.com>
>>
>> 	* gdb.texinfo (-target-flash-erase): New MI command description.
>> 	(flash-erase): New CLI command description.
>>
>> gdb/ChangeLog:
>>
>> 2017-01-09  Mike Wrighton  <mike_wrighton@codesourcery.com>
>> 	    Luis Machado  <lgustavo@codesourcery.com>
>>
>> 	* NEWS (New commands): Mention flash-erase.
>> 	(New MI commands): Mention target-flash-erase.
>> 	* mi/mi-cmds.c (mi_cmd_target_flash_erase): Added target-flash-erase MI
>> 	command.
>> 	* mi/mi-cmds.h (mi_cmd_target_flash_erase): New declaration.
>> 	* mi/mi-main.c (mi_cmd_target_flash_erase): New function.
>> 	* target-memory.c: Include "memattr.h" and "ui-out.h".
>> 	* target.c (flash_erase_all_command): New function.
>> 	(initialize_targets): Add new flash-erase command.
>> 	* target.h (flash_erase_all_command): New declaration.
>> ---
>>  gdb/NEWS            | 11 +++++++++++
>>  gdb/doc/gdb.texinfo | 32 ++++++++++++++++++++++++++++++++
>>  gdb/mi/mi-cmds.c    |  1 +
>>  gdb/mi/mi-cmds.h    |  1 +
>>  gdb/mi/mi-main.c    |  5 +++++
>>  gdb/target.c        | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>  gdb/target.h        |  3 +++
>>  7 files changed, 98 insertions(+)
>>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index b976815..4d9effa 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -57,6 +57,17 @@ FreeBSD/mips			mips*-*-freebsd
>>  Synopsys ARC			arc*-*-elf32
>>  FreeBSD/mips			mips*-*-freebsd
>>
>> +* New commands
>> +
>> +flash-erase
>> +  Erases all the flash memory regions reported by the target.
>> +
>> +* New MI commands
>> +
>> +target-flash-erase
>> +  Erases all the flash memory regions reported by the target.  This is
>> +  equivalent to the CLI command flash-erase.
>> +
>>  *** Changes in GDB 7.12
>>
>>  * GDB and GDBserver now build with a C++ compiler by default.
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index 14628fa..24d6cf7 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -19626,6 +19626,16 @@ load programs into flash memory.
>>  @code{load} does not repeat if you press @key{RET} again after using it.
>>  @end table
>>
>> +@table @code
>> +
>> +@kindex flash-erase
>> +@item flash-erase
>> +@anchor{flash-erase}
>> +
>> +Erases all known flash memory regions on the target.
>> +
>> +@end table
>> +
>>  @node Byte Order
>>  @section Choosing Target Byte Order
>>
>> @@ -31863,6 +31873,28 @@ No equivalent.
>>  @subsubheading Example
>>  N.A.
>>
>> +@subheading The @code{-target-flash-erase} Command
>> +@findex -target-flash-erase
>> +
>> +@subsubheading Synopsis
>> +
>> +@smallexample
>> + -target-flash-erase
>> +@end smallexample
>> +
>> +Erases all known flash memory regions on the target.
>> +
>> +The corresponding @value{GDBN} command is @samp{flash-erase}.
>> +
>> +The output is a list of flash regions that have been erased, sorted by
>> +address/size.
>
> Are they really sorted by size?
>
>> +
>> +@smallexample
>> +(gdb)
>> +-target-flash-erase
>> +^done,erased-regions=@{address="0x0",size="262144"@}
>> +(gdb)
>> +@end smallexample
>>
>>  @subheading The @code{-target-select} Command
>>  @findex -target-select
>> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
>> index cdea008..abb70bd 100644
>> --- a/gdb/mi/mi-cmds.c
>> +++ b/gdb/mi/mi-cmds.c
>> @@ -147,6 +147,7 @@ static struct mi_cmd mi_cmds[] =
>>    DEF_MI_CMD_MI ("target-file-delete", mi_cmd_target_file_delete),
>>    DEF_MI_CMD_MI ("target-file-get", mi_cmd_target_file_get),
>>    DEF_MI_CMD_MI ("target-file-put", mi_cmd_target_file_put),
>> +  DEF_MI_CMD_MI ("target-flash-erase", mi_cmd_target_flash_erase),
>>    DEF_MI_CMD_CLI ("target-select", "target", 1),
>>    DEF_MI_CMD_MI ("thread-info", mi_cmd_thread_info),
>>    DEF_MI_CMD_MI ("thread-list-ids", mi_cmd_thread_list_ids),
>> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
>> index 8bd947b..d0906e6 100644
>> --- a/gdb/mi/mi-cmds.h
>> +++ b/gdb/mi/mi-cmds.h
>> @@ -93,6 +93,7 @@ extern mi_cmd_argv_ftype mi_cmd_target_detach;
>>  extern mi_cmd_argv_ftype mi_cmd_target_file_get;
>>  extern mi_cmd_argv_ftype mi_cmd_target_file_put;
>>  extern mi_cmd_argv_ftype mi_cmd_target_file_delete;
>> +extern mi_cmd_argv_ftype mi_cmd_target_flash_erase;
>>  extern mi_cmd_argv_ftype mi_cmd_thread_info;
>>  extern mi_cmd_argv_ftype mi_cmd_thread_list_ids;
>>  extern mi_cmd_argv_ftype mi_cmd_thread_select;
>> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
>> index 22803cb..e432a01 100644
>> --- a/gdb/mi/mi-main.c
>> +++ b/gdb/mi/mi-main.c
>> @@ -552,6 +552,11 @@ mi_cmd_target_detach (char *command, char **argv, int argc)
>>    detach_command (NULL, 0);
>>  }
>>
>> +void mi_cmd_target_flash_erase (char *command, char **argv, int argc)
>
> Like break after void.
>
>> +{
>> +  flash_erase_all_command (NULL, 0);
>> +}
>> +
>>  void
>>  mi_cmd_thread_select (char *command, char **argv, int argc)
>>  {
>> diff --git a/gdb/target.c b/gdb/target.c
>> index be7367c..02e80d2 100644
>> --- a/gdb/target.c
>> +++ b/gdb/target.c
>> @@ -3943,6 +3943,48 @@ do_monitor_command (char *cmd,
>>    target_rcmd (cmd, gdb_stdtarg);
>>  }
>>
>> +void
>> +flash_erase_all_command (char *cmd, int from_tty)
>
> Comment.
>
>> +{
>> +  int i;
>> +  int found_flash_region = 0;
>
> Use bool.  Add comment explaining why we need it?
>
>> +  struct mem_region *m;
>> +  struct mem_attrib *attrib;
>> +  struct cleanup *cleanup_tuple;
>> +  struct gdbarch *gdbarch = target_gdbarch ();
>
>
> And move all these to the scope that needs them.  Maybe
> even declare them on first use.
>
>> +
>> +  VEC(mem_region_s) * mem_regions = target_memory_map ();
>> +
>> +  /* Iterate over all memory regions.  */
>> +  for (i = 0; VEC_iterate (mem_region_s, mem_regions, i, m); i++)
>> +    {
>> +      attrib = &m->attrib;
>> +
>> +      /* Is this a flash memory region?  */
>> +      if (attrib->mode == MEM_FLASH)
>> +        {
>> +          found_flash_region = 1;
>> +          target_flash_erase (m->lo, m->hi - m->lo);
>> +          cleanup_tuple =
>
> = goes on next line.
>
>> +	      make_cleanup_ui_out_tuple_begin_end (current_uiout,
>> +						   "erasing-regions");
>> +          current_uiout->message ("Erasing flash memory region at address ");
>> +          current_uiout->field_fmt ("address", "%s", paddress (gdbarch,
>> +								 m->lo));
>> +          current_uiout->message (", size = ");
>> +          current_uiout->field_fmt ("size", "%lu",
>> +				    (unsigned long) (m->hi - m->lo));
>
> Pedantically, long is the wrong type to use, since it'll be 32-bit
> on 64-bit Windows.  Use pulongest?
>

I've used phex now. I agree it is more appropriate. I've updated the 
documentation as well.

Addressed the other comments locally as well.

>> +          current_uiout->message ("\n");
>> +          do_cleanups (cleanup_tuple);
>> +        }
>> +    }
>> +
>> +  if (found_flash_region)
>> +    target_flash_done ();
>
> I was wondering how this is supposed to work if the command
> is interrupted w/ ctrl-c midway.  If the command is interrupted before
> sending the vFlashDone RSP command, then it seems to me that the stub
> is doing to get confused, since it seems there's no RSP command to
> mean "I'm starting a batch of flash operations from scratch, forget
> any stale list of pending vFlashWrite/vFlashErase ops you may have.
> Am I missing something that prevents that from happening?

Yeah, that scenario can indeed happen if one interrupts the flash 
programming midway. It is a bit of an oversight in the design of this 
functionality i think.

The target stub, as it is today, will not be too confused about this. 
The memory regions will be appended to a list and the stub will wait for 
the vFlashDone packet to commit the changes.

The way the commit process is implemented will attempt to combine 
writes/deletions in a single operation. So, in the worst case, the stub 
will only execute these operations next time it sees vFlashDone.

Not ideal, but it is how it works nowadays.

Re: [PATCH, v2] Add command to erase all flash memory regions

[Pedro Alves]

On 01/11/2017 06:55 PM, Luis Machado wrote:
> On 01/10/2017 12:22 PM, Pedro Alves wrote:

>> I was wondering how this is supposed to work if the command
>> is interrupted w/ ctrl-c midway.  If the command is interrupted before
>> sending the vFlashDone RSP command, then it seems to me that the stub
>> is doing to get confused, since it seems there's no RSP command to
>> mean "I'm starting a batch of flash operations from scratch, forget
>> any stale list of pending vFlashWrite/vFlashErase ops you may have.
>> Am I missing something that prevents that from happening?
> 
> Yeah, that scenario can indeed happen if one interrupts the flash
> programming midway. It is a bit of an oversight in the design of this
> functionality i think.
> 
> The target stub, as it is today, will not be too confused about this.
> The memory regions will be appended to a list and the stub will wait for
> the vFlashDone packet to commit the changes.
> 
> The way the commit process is implemented will attempt to combine
> writes/deletions in a single operation. So, in the worst case, the stub
> will only execute these operations next time it sees vFlashDone.
> 
> Not ideal, but it is how it works nowadays.

Hmm, indeed.  Not as bad as I first though.  Thanks.

-- 
Pedro Alves