Re: [PATCH] Add -verify option to load command

[Luis Machado <lgustavo@codesourcery.com>]

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 <offset>" without a symbol file. GDB will complain about 
not finding file <offset>. 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