From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 83887 invoked by alias); 6 Jan 2017 19:17:08 -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 83878 invoked by uid 89); 6 Jan 2017 19:17:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=sneak, (unknown), hear X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 06 Jan 2017 19:17:05 +0000 Received: by simark.ca (Postfix, from userid 33) id ECCA41E809; Fri, 6 Jan 2017 14:17:03 -0500 (EST) To: Luis Machado Subject: Re: [PATCH] Add -verify option to load command X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Fri, 06 Jan 2017 19:17:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: <1483720912-6563-1-git-send-email-lgustavo@codesourcery.com> References: <1483720912-6563-1-git-send-email-lgustavo@codesourcery.com> Message-ID: <01a92fce926525df375f1317da45e2df@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.3 X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg00111.txt.bz2 On 2017-01-06 11:41, Luis Machado wrote: > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index 44ae068..39de23c 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -19592,7 +19592,7 @@ Show the current status of displaying > communications between > @table @code > > @kindex load @var{filename} > -@item load @var{filename} > +@item load @var{filename} [-verify] I just noticed the documentation here doesn't talk about OFFSET. > @@ -2099,19 +2101,30 @@ generic_load (const char *args, int from_tty) > filename = tilde_expand (argv[0]); > make_cleanup (xfree, filename); > > - if (argv[1] != NULL) > + arg_idx = 1; > + if (argv[arg_idx] != NULL) > { > const char *endptr; > > - cbdata.load_offset = strtoulst (argv[1], &endptr, 0); > + if (strcmp (argv[arg_idx], "-verify") == 0) > + { > + verify = 1; > + arg_idx++; > + } > + > + if (argv[arg_idx] != NULL) > + { > + cbdata.load_offset = strtoul ((const char *) argv[arg_idx], > + (char **) &endptr, 0); > > - /* If the last word was not a valid number then > - treat it as a file name with spaces in. */ > - if (argv[1] == endptr) > - error (_("Invalid download offset:%s."), argv[1]); > + /* If the last word was not a valid number then > + treat it as a file name with spaces in. */ > + if (argv[arg_idx] == endptr) > + error (_("Invalid download offset:%s."), argv[arg_idx]); You could sneak a space after the colon here :). I know that's old code, but I don't really understand it. According to the help text of load, from your other patch, the offset can be an expression, s I assume "$foo + 1" should work. The check with strtoul would clearly not accept that. > @@ -2140,7 +2153,7 @@ generic_load (const char *args, int from_tty) > steady_clock::time_point start_time = steady_clock::now (); > > if (target_write_memory_blocks (cbdata.requests, flash_discard, > - load_progress) != 0) > + load_progress, verify) != 0) > error (_("Load failed")); > > steady_clock::time_point end_time = steady_clock::now (); > @@ -3952,8 +3965,8 @@ that lies within the boundaries of this symbol > file in memory."), > c = add_cmd ("load", class_files, load_command, _("\ > Dynamically load FILE into the running program, and record its > symbols\n\ > for access from GDB.\n\ > -A load offset may also be given.\n\ > -Usage: load [FILE] [offset expression]"), &cmdlist); > +A load offset and write verification option may also be given.\n\ > +Usage: load [FILE] [-verify] [offset expression]"), &cmdlist); Is there a reason why you placed the [-verify] between the two other arguments and not before them? That's where I would usually expect the "dash" arguments to be placed in the usage message. From what I understand it is possible to use load without specifying FILE, which will load the executable currently loaded in gdb. So I think all these forms should be valid: (gdb) load -verify (gdb) load myfile (gdb) load -verify myfile (gdb) load myfile myoffset (gdb) load -verify myfile myoffset Ideally, we should be able to place the "dash" arguments anywhere, just like with any good command line tool. Since we don't have that, I think that having between the command and the positional arguments makes more sense. That's my opinion though, I'm curious to hear what others think. > + /* Do we need to verify if the data was properly written to the > target's > + memory? */ > + if (verify) > + { > + /* Go through all memory regions that GDB wrote to and verify > the > + contents. */ > + for (i = 0; VEC_iterate (memory_write_request_s, blocks, i, r); > ++i) > + if (target_verify_memory (r->data, r->begin, r->end - r->begin) <= 0) > + error ("Load verification failed for region starting at 0x%x", > + (unsigned int) r->begin); > + else > + current_uiout->message ("Verified load, size 0x%x lma 0x%x\n", > + (unsigned int) (r->end - r->begin), > + (unsigned int) r->begin); I guess the end and begin fields of memory_write_request should be of type CORE_ADDR, and this should use paddress. > diff --git a/gdb/target.h b/gdb/target.h > index e239c2c..6fc89d4 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -1497,11 +1497,14 @@ enum flash_preserve_mode > feedback to user. It will be called with the baton corresponding > to the request currently being written. It may also be called > with a NULL baton, when preserved flash sectors are being > rewritten. > + VERIFY is non-zero if verification should be performed for the data > written > + to the target's memory and zero if no verification should be > performed. Make sure that the line wrapping matches the other arguments. > > The function returns 0 on success, and error otherwise. */ > int target_write_memory_blocks (VEC(memory_write_request_s) *requests, > enum flash_preserve_mode preserve_flash_p, > - void (*progress_cb) (ULONGEST, void *)); > + void (*progress_cb) (ULONGEST, void *), > + int verify); bool? (for the whole call chain) Thanks, Simon