From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27918 invoked by alias); 11 Jan 2017 18:56:06 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 27845 invoked by uid 89); 11 Jan 2017 18:56:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS,URIBL_RED autolearn=ham version=3.3.2 spammy=phex, H*f:sk:84c7a2e, H*i:sk:84c7a2e, H*MI:sk:84c7a2e X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 11 Jan 2017 18:55:55 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1cRO3s-0001vF-Pr from Luis_Gustavo@mentor.com ; Wed, 11 Jan 2017 10:55:52 -0800 Received: from [172.30.6.249] (147.34.91.1) by svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Wed, 11 Jan 2017 10:55:49 -0800 Reply-To: Luis Machado Subject: Re: [PATCH, v2] Add command to erase all flash memory regions References: <1483988641-27277-1-git-send-email-lgustavo@codesourcery.com> <84c7a2e2-ca66-a7ea-1a84-c355559a47b9@redhat.com> To: Pedro Alves , From: Luis Machado Message-ID: Date: Wed, 11 Jan 2017 18:56:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <84c7a2e2-ca66-a7ea-1a84-c355559a47b9@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg00195.txt.bz2 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 >> Luis Machado >> >> * gdb.texinfo (-target-flash-erase): New MI command description. >> (flash-erase): New CLI command description. >> >> gdb/ChangeLog: >> >> 2017-01-09 Mike Wrighton >> Luis Machado >> >> * 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.