Re: [RFA, 1 of 3] save/restore process record, part 1 (exec_entry)

[Michael Snyder <msnyder@vmware.com>]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]

Joel Brobecker wrote:
>> 2009-10-16  Hui Zhu  <teawater@gmail.com>
>> 	    Michael Snyder  <msnyder@vmware.com>
>>
>> 	* record.c (record_exec_entry): New function.  Emulate one
>> 	instruction, forward or backward.  Abstracted from record_wait.
>> 	(record_wait) Call record_exec_entry.
> 
> I can personnally only comment on details, since I don't know much
> about process record. Not sure who from the Global Maintainers actually
> know much about it except you, Michael :).

I know -- do you think, if I add more comments and documentation,
it will help encourage a few of the other GMs to take an interest?
;-)

In the meantime, Hui and I depend on each other's review.


>> +static inline void
>> +record_exec_entry (struct regcache *regcache, struct gdbarch *gdbarch,
>> +                   struct record_entry *entry)
> 
> We're really pushing for having all functions properly documented.
> Can you add a comment explaining that this function does? The function
> name makes it more or less obvious, I guess, but I'd personally rather
> have a consistent (mindless) approach of documenting everything rather
> than having to judge on a case-by-case basis.

OK, see attached revision.  Thanks for the request.

> Also, I can't help but wonder about the use of "inline" in this case.
> I'm always reluctant to use this sort of feature until I can be proven
> that this helps performance. Since this is a static function, I would
> imagine that the compiler would have more knowledge of whether the
> function should be inlined or not? Or is that too naive?

"Inline", like "register", is just a suggestion to the compiler.
The compiler will do as it pleases, based on other factors.

This function gets called once per instruction in playback mode.
Hence it seemed like it would benefit from inlining.  It's probably
too big to be inlined anyway, but there again, the compiler will
make up its own mind.

I don't know whether we have a policy about using "inline" in
gdb code.  Maybe we should.  I'll be willing to take it out,
but until I hear a positive request, I'll leave it in.  I doubt
if I can measure the performance difference, if any.

Revised diff attached.


[-- Attachment #2: tmp1b.txt --]
[-- Type: text/plain, Size: 6123 bytes --]

2009-10-16  Hui Zhu  <teawater@gmail.com>
	    Michael Snyder  <msnyder@vmware.com>

	* record.c (record_exec_entry): New function.  Emulate one
	instruction, forward or backward.  Abstracted from record_wait.
	(record_wait) Call record_exec_entry.

--- record.0.c	2009-10-16 14:28:07.000000000 -0700
+++ record.1.c	2009-10-17 09:50:42.000000000 -0700
@@ -588,6 +588,80 @@ record_gdb_operation_disable_set (void)
   return old_cleanups;
 }
 
+/* Execute one instruction from the record log.  Each instruction in
+   the log will be represented by an arbitrary sequence of register
+   entries and memory entries, followed by an 'end' entry.  */
+
+static inline void
+record_exec_insn (struct regcache *regcache, struct gdbarch *gdbarch,
+		  struct record_entry *entry)
+{
+  switch (entry->type)
+    {
+    case record_reg: /* reg */
+      {
+        gdb_byte reg[MAX_REGISTER_SIZE];
+
+        if (record_debug > 1)
+          fprintf_unfiltered (gdb_stdlog,
+                              "Process record: record_reg %s to "
+                              "inferior num = %d.\n",
+                              host_address_to_string (entry),
+                              entry->u.reg.num);
+
+        regcache_cooked_read (regcache, entry->u.reg.num, reg);
+        regcache_cooked_write (regcache, entry->u.reg.num, 
+			       record_get_loc (entry));
+        memcpy (record_get_loc (entry), reg, entry->u.reg.len);
+      }
+      break;
+
+    case record_mem: /* mem */
+      {
+	/* Nothing to do if the entry is flagged not_accessible.  */
+        if (!entry->u.mem.mem_entry_not_accessible)
+          {
+            gdb_byte *mem = alloca (entry->u.mem.len);
+
+            if (record_debug > 1)
+              fprintf_unfiltered (gdb_stdlog,
+                                  "Process record: record_mem %s to "
+                                  "inferior addr = %s len = %d.\n",
+                                  host_address_to_string (entry),
+                                  paddress (gdbarch, entry->u.mem.addr),
+                                  entry->u.mem.len);
+
+            if (target_read_memory (entry->u.mem.addr, mem, entry->u.mem.len))
+              {
+                entry->u.mem.mem_entry_not_accessible = 1;
+                if (record_debug)
+                  warning (_("Process record: error reading memory at "
+                             "addr = %s len = %d."),
+                           paddress (gdbarch, entry->u.mem.addr),
+                           entry->u.mem.len);
+              }
+            else
+              {
+                if (target_write_memory (entry->u.mem.addr, 
+					 record_get_loc (entry),
+					 entry->u.mem.len))
+                  {
+                    entry->u.mem.mem_entry_not_accessible = 1;
+                    if (record_debug)
+                      warning (_("Process record: error writing memory at "
+                                 "addr = %s len = %d."),
+                               paddress (gdbarch, entry->u.mem.addr),
+                               entry->u.mem.len);
+                  }
+                else
+                  memcpy (record_get_loc (entry), mem, entry->u.mem.len);
+              }
+          }
+      }
+      break;
+    }
+}
+
 static void
 record_open (char *name, int from_tty)
 {
@@ -884,77 +958,9 @@ record_wait (struct target_ops *ops,
 	      break;
 	    }
 
-	  /* Set ptid, register and memory according to record_list.  */
-	  if (record_list->type == record_reg)
-	    {
-	      /* reg */
-	      gdb_byte reg[MAX_REGISTER_SIZE];
-	      if (record_debug > 1)
-		fprintf_unfiltered (gdb_stdlog,
-				    "Process record: record_reg %s to "
-				    "inferior num = %d.\n",
-				    host_address_to_string (record_list),
-				    record_list->u.reg.num);
-	      regcache_cooked_read (regcache, record_list->u.reg.num, reg);
-	      regcache_cooked_write (regcache, record_list->u.reg.num,
-				     record_get_loc (record_list));
-	      memcpy (record_get_loc (record_list), reg, 
-		      record_list->u.reg.len);
-	    }
-	  else if (record_list->type == record_mem)
-	    {
-	      /* mem */
-	      /* Nothing to do if the entry is flagged not_accessible.  */
-	      if (!record_list->u.mem.mem_entry_not_accessible)
-		{
-		  gdb_byte *mem = alloca (record_list->u.mem.len);
-		  if (record_debug > 1)
-		    fprintf_unfiltered (gdb_stdlog,
-				        "Process record: record_mem %s to "
-				        "inferior addr = %s len = %d.\n",
-				        host_address_to_string (record_list),
-				        paddress (gdbarch,
-					          record_list->u.mem.addr),
-				        record_list->u.mem.len);
+          record_exec_insn (regcache, gdbarch, record_list);
 
-		  if (target_read_memory (record_list->u.mem.addr, mem,
-		                          record_list->u.mem.len))
-	            {
-		      if (execution_direction != EXEC_REVERSE)
-		        error (_("Process record: error reading memory at "
-			         "addr = %s len = %d."),
-		               paddress (gdbarch, record_list->u.mem.addr),
-		               record_list->u.mem.len);
-		      else
-			/* Read failed -- 
-			   flag entry as not_accessible.  */
-		        record_list->u.mem.mem_entry_not_accessible = 1;
-		    }
-		  else
-		    {
-		      if (target_write_memory (record_list->u.mem.addr,
-					       record_get_loc (record_list),
-		                               record_list->u.mem.len))
-	                {
-			  if (execution_direction != EXEC_REVERSE)
-			    error (_("Process record: error writing memory at "
-			             "addr = %s len = %d."),
-		                   paddress (gdbarch, record_list->u.mem.addr),
-		                   record_list->u.mem.len);
-			  else
-			    /* Write failed -- 
-			       flag entry as not_accessible.  */
-			    record_list->u.mem.mem_entry_not_accessible = 1;
-			}
-		      else
-		        {
-			  memcpy (record_get_loc (record_list), mem,
-				  record_list->u.mem.len);
-			}
-		    }
-		}
-	    }
-	  else
+	  if (record_list->type == record_end)
 	    {
 	      if (record_debug > 1)
 		fprintf_unfiltered (gdb_stdlog,