From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 100880 invoked by alias); 6 Jan 2017 19:33: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 100868 invoked by uid 89); 6 Jan 2017 19:33:08 -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 autolearn=ham version=3.3.2 spammy=sneak, myfile, myoffset, $foo 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; Fri, 06 Jan 2017 19:33:06 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1cPaG8-0003FR-PT from Luis_Gustavo@mentor.com ; Fri, 06 Jan 2017 11:33:04 -0800 Received: from [172.30.8.13] (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; Fri, 6 Jan 2017 11:33:00 -0800 Reply-To: Luis Machado Subject: Re: [PATCH] Add -verify option to load command References: <1483720912-6563-1-git-send-email-lgustavo@codesourcery.com> <01a92fce926525df375f1317da45e2df@polymtl.ca> To: Simon Marchi CC: From: Luis Machado Message-ID: <3a9784ff-cae4-f0f0-5209-636ca69dd1d5@codesourcery.com> Date: Fri, 06 Jan 2017 19:33: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: <01a92fce926525df375f1317da45e2df@polymtl.ca> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-01.mgc.mentorg.com (147.34.90.201) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg00112.txt.bz2 On 01/06/2017 01:17 PM, Simon Marchi wrote: > 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. > Ah, see? More documentation bits that are inconsistent. I'll fix this. Thanks for spotting it. >> @@ -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. > The problem with handling old code that we didn't fully write is that one may not always know the reason. This is one of those cases. >> @@ -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. > No. I think it is just the way it was implemented. I could refactor it to make it work better for sure. > 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: > I notice the current way load works is slightly off. For example, you can't do "load " without a symbol file. GDB will complain about not finding file . Sounds like that is another improvement. > (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. > I'm fine with whatever positioning is deemed appropriate. Personally, i like the -verify at the end. :-) >> + /* 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. > I'll get it fixed. Thanks. >> 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. > Indeed. >> >> 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) > That makes sense. > Thanks, > > Simon Thanks for reviewing it. Luis