Add -verify option to load command
Commit Message
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
> 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.
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?
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
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
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
> 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.
@@ -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
@@ -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,
@@ -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);
@@ -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. */