This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Add -verify option to load command
- From: Simon Marchi <simon dot marchi at polymtl dot ca>
- To: Luis Machado <lgustavo at codesourcery dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 06 Jan 2017 14:17:03 -0500
- Subject: Re: [PATCH] Add -verify option to load command
- Authentication-results: sourceware.org; auth=none
- References: <1483720912-6563-1-git-send-email-lgustavo@codesourcery.com>
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