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

[Simon Marchi <simon.marchi@polymtl.ca>]

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