From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 42204 invoked by alias); 10 Jan 2017 18:23:09 -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 42181 invoked by uid 89); 10 Jan 2017 18:23:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,URIBL_RED autolearn=ham version=3.3.2 spammy=H*i:sk:1483988, i18n, interrupted, UD:A X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 10 Jan 2017 18:22:58 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AD44921722; Tue, 10 Jan 2017 18:22:58 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v0AIMuqZ018909; Tue, 10 Jan 2017 13:22:57 -0500 Subject: Re: [PATCH, v2] Add command to erase all flash memory regions To: Luis Machado , gdb-patches@sourceware.org References: <1483988641-27277-1-git-send-email-lgustavo@codesourcery.com> From: Pedro Alves Message-ID: <84c7a2e2-ca66-a7ea-1a84-c355559a47b9@redhat.com> Date: Tue, 10 Jan 2017 18:23:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1483988641-27277-1-git-send-email-lgustavo@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-01/txt/msg00177.txt.bz2 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? > + 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