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

[Luis Machado <lgustavo@codesourcery.com>]

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.