This is the mail archive of the 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 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)



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