diff mbox

Add -verify option to load command

Message ID 1483720912-6563-1-git-send-email-lgustavo@codesourcery.com
State New
Headers show

Commit Message

Luis Machado Jan. 6, 2017, 4:41 p.m. UTC
This is also a leftover bit from flash programming that can also be used by
other targets as needed.  GDB already has some minimal memory verification
functionality.

The following patch adds a new -verify parameter to the load command that
forces GDB to verify all the memory written during the load operation.

This is useful to make sure bare-metal targets have had the program and data
loaded successfully.  I wouldn't say this is too useful for hosted
environments, but not harmful either.

What do you think?

gdb/doc/ChangeLog:

2017-01-05  Mike Wrighton  <mike_wrighton@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	gdb/doc/
	* gdb.texinfo (load): Added verify flag to load command description.

gdb/ChangeLog:

2017-01-05  Mike Wrighton  <mike_wrighton@codesourcery.com>
	    Luis Machado  <lgustavo@codesourcery.com>

	* symfile.c (generic_load): Add -verify arg.
	(_initialize_symfile): Document new -verify option in the load
	command's help text.
	* target-memory.c (target_write_memory_blocks): Add verify parameter
	and handle it.
	* target.h (target_write_memory_blocks): Add verify argument and
	document it.
---
 gdb/doc/gdb.texinfo |  5 ++++-
 gdb/symfile.c       | 35 ++++++++++++++++++++++++-----------
 gdb/target-memory.c | 20 +++++++++++++++++++-
 gdb/target.h        |  5 ++++-
 4 files changed, 51 insertions(+), 14 deletions(-)

Comments

Eli Zaretskii Jan. 6, 2017, 6:17 p.m. UTC | #1
> From: Luis Machado <lgustavo@codesourcery.com>
> Date: Fri, 6 Jan 2017 10:41:52 -0600
> 
> --- 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]
>  @anchor{load}
>  Depending on what remote debugging facilities are configured into
>  @value{GDBN}, the @code{load} command may be available.  Where it exists, it
> @@ -19614,6 +19614,9 @@ specifies a fixed address.
>  Depending on the remote side capabilities, @value{GDBN} may be able to
>  load programs into flash memory.
>  
> +The optional @code{-verify} argument enables verification of the data that was
> +loaded to the target's memory.

I think the manual should explain what "verification" means in this
context.

Thanks.
Luis Machado Jan. 6, 2017, 6:30 p.m. UTC | #2
On 01/06/2017 12:17 PM, Eli Zaretskii wrote:
>> From: Luis Machado <lgustavo@codesourcery.com>
>> Date: Fri, 6 Jan 2017 10:41:52 -0600
>>
>> --- 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]
>>  @anchor{load}
>>  Depending on what remote debugging facilities are configured into
>>  @value{GDBN}, the @code{load} command may be available.  Where it exists, it
>> @@ -19614,6 +19614,9 @@ specifies a fixed address.
>>  Depending on the remote side capabilities, @value{GDBN} may be able to
>>  load programs into flash memory.
>>
>> +The optional @code{-verify} argument enables verification of the data that was
>> +loaded to the target's memory.
>
> I think the manual should explain what "verification" means in this
> context.
>
> Thanks.
>

Something along the lines of...

"GDB will make sure all the memory regions that were written to the 
target's memory have their correct values, otherwise an error will be 
issued."?

Should i go even further to explain how the comparison is made?
Simon Marchi Jan. 6, 2017, 7:17 p.m. UTC | #3
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
Luis Machado Jan. 6, 2017, 7:32 p.m. UTC | #4
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
Simon Marchi Jan. 6, 2017, 7:59 p.m. UTC | #5
On 2017-01-06 14:32, Luis Machado wrote:
>> 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.

Indeed, perhaps it should have been an option "-offset <offset>".

>> (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. :-)

The problem with at the end is that it's harder to handle when you also 
have a variable number of positional arguments.

For example, if you had:

   mycommand [-foo|-bar|-baz] ALPHA [BETA] [GAMMA]

where you can have one or more of -foo/-bar-baz, one required argument 
ALPHA and two optional positional arguments BETA and GAMMA.  With this 
form, it's easy to take care of -foo/-bar/-baz.  As soon as you have an 
argument that doesn't match those, the rest of argv is the positional 
arguments (possibly with -- if you wanted to be able to pass -foo as the 
ALPHA parameter).

If they were at the end like so:

   mycommand ALPHA [BETA] [GAMMA] [-foo|-bar|-baz]

it becomes difficult to determine whether the argument that follows 
ALPHA is BETA or one of -foo/-bar/-baz...

Anyway, the real solution would be to use an appropriate argument 
parsing library, so we wouldn't have to reinvent the wheel for every 
command, and maybe have some automatically generated help message.  
Crazy stuff :).

Thanks,

Simon
Eli Zaretskii Jan. 7, 2017, 7:53 a.m. UTC | #6
> CC: <gdb-patches@sourceware.org>
> From: Luis Machado <lgustavo@codesourcery.com>
> Date: Fri, 6 Jan 2017 12:30:06 -0600
> 
> >> +The optional @code{-verify} argument enables verification of the data that was
> >> +loaded to the target's memory.
> >
> > I think the manual should explain what "verification" means in this
> > context.
> >
> > Thanks.
> >
> 
> Something along the lines of...
> 
> "GDB will make sure all the memory regions that were written to the 
> target's memory have their correct values, otherwise an error will be 
> issued."?
> 
> Should i go even further to explain how the comparison is made?

I'd suggest

  GDB will read back the memory regions that were written, and if some
  value is not as written, will issue an error message.
diff mbox

Patch

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]
 @anchor{load}
 Depending on what remote debugging facilities are configured into
 @value{GDBN}, the @code{load} command may be available.  Where it exists, it
@@ -19614,6 +19614,9 @@  specifies a fixed address.
 Depending on the remote side capabilities, @value{GDBN} may be able to
 load programs into flash memory.
 
+The optional @code{-verify} argument enables verification of the data that was
+loaded to the target's memory.
+
 @code{load} does not repeat if you press @key{RET} again after using it.
 @end table
 
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 0af1900..4fdde17 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2080,6 +2080,8 @@  generic_load (const char *args, int from_tty)
   struct load_section_data cbdata;
   struct load_progress_data total_progress;
   struct ui_out *uiout = current_uiout;
+  int arg_idx;
+  int verify = 0;
 
   CORE_ADDR entry;
   char **argv;
@@ -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]);
 
-      if (argv[2] != NULL)
-	error (_("Too many parameters."));
+	  if (argv[arg_idx + 1] != NULL)
+	    error (_("Too many parameters."));
+	}
     }
 
   /* Open the file for loading.  */
@@ -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);
   set_cmd_completer (c, filename_completer);
 
   add_prefix_cmd ("overlay", class_support, overlay_command,
diff --git a/gdb/target-memory.c b/gdb/target-memory.c
index 1c8faa8..615bab8 100644
--- a/gdb/target-memory.c
+++ b/gdb/target-memory.c
@@ -310,7 +310,8 @@  cleanup_write_requests_vector (void *p)
 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)
 {
   struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
   VEC(memory_write_request_s) *blocks = VEC_copy (memory_write_request_s,
@@ -433,6 +434,23 @@  target_write_memory_blocks (VEC(memory_write_request_s) *requests,
       target_flash_done ();
     }
 
+
+  /* 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);
+    }
+
  out:
   do_cleanups (back_to);
 
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.
 
    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);
 
 /* Print a line about the current target.  */