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