This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]