Re: [PATCH] Removal of uses of MAX_REGISTER_SIZE

[Alan Hayward <Alan.Hayward@arm.com>]

> On 8 Feb 2017, at 17:09, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> @@ -1135,8 +1135,8 @@ register_changed_p (int regnum, struct regcache *prev_regs,
>> 		    struct regcache *this_regs)
>> {
>>   struct gdbarch *gdbarch = get_regcache_arch (this_regs);
>> -  gdb_byte prev_buffer[MAX_REGISTER_SIZE];
>> -  gdb_byte this_buffer[MAX_REGISTER_SIZE];
>> +  std::vector<gdb_byte> prev_buffer (register_size (gdbarch, regnum));
>> +  std::vector<gdb_byte> this_buffer (register_size (gdbarch, regnum));
>>   enum register_status prev_status;
>>   enum register_status this_status;
>> 
> 
> This function should be moved to regcache.c, because it is about
> comparing bytes of a certain register in both regcaches.  Then, wen can
> compare raw registers from register_buffer, and pseudo registers from
> the values.
> 
>> @@ -1146,13 +1146,13 @@ register_changed_p (int regnum, struct regcache *prev_regs,
>>     return 1;
>> 
>>   /* Get register contents and compare.  */
>> -  prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer);
>> -  this_status = regcache_cooked_read (this_regs, regnum, this_buffer);
>> +  prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer.data ());
>> +  this_status = regcache_cooked_read (this_regs, regnum, this_buffer.data ());
>> 
>>   if (this_status != prev_status)
>>     return 1;
>>   else if (this_status == REG_VALID)
>> -    return memcmp (prev_buffer, this_buffer,
>> +    return memcmp (prev_buffer.data (), this_buffer.data (),
>> 		   register_size (gdbarch, regnum)) != 0;
>>   else
>>     return 0;
> 
> -- 
> Yao (齐尧)


Ignore my previous reply to this comment. I've since noticed you can just use
regcache_cooked_read_value.

New patch below.

I removed the error case from mi-main becuase it was impossible for the code
to get here - register_changed_p would always return 0 or 1.

Tested on a build of all targets using make check with target boards unix and
native-gdbserver. This testing also included the two patches:
* M68K_MAX_REGISTER_SIZE and I386_MAX_REGISTER_SIZE changes
* stack.c changes

Ok?

Alan.


2017-02-13  Alan Hayward  <alan.hayward@arm.com>

	* gdb/mi/mi-main.c (mi_cmd_data_list_changed_registers): Use
	regcache_register_changed_p
	(register_changed_p): Remove.
	* gdb/regcache.c (regcache_register_changed_p): New function.
	* gdb/regcache.h (regcache_register_changed_p): New declaration.


diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 57c23ebf5d6b2d3b398aa40ebd9b3cb70c56125c..d22cb9b59d3c5d65609bf3762457bba90401fd8f 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -91,8 +91,6 @@ static void mi_execute_cli_command (const char *cmd, int args_p,
 				    const char *args);
 static void mi_execute_async_cli_command (char *cli_command,
 					  char **argv, int argc);
-static int register_changed_p (int regnum, struct regcache *,
-			       struct regcache *);
 static void output_register (struct frame_info *, int regnum, int format,
 			     int skip_unavailable);

@@ -1098,11 +1096,7 @@ mi_cmd_data_list_changed_registers (char *command, char **argv, int argc)
 	  if (gdbarch_register_name (gdbarch, regnum) == NULL
 	      || *(gdbarch_register_name (gdbarch, regnum)) == '\0')
 	    continue;
-	  changed = register_changed_p (regnum, prev_regs, this_regs);
-	  if (changed < 0)
-	    error (_("-data-list-changed-registers: "
-		     "Unable to read register contents."));
-	  else if (changed)
+	  if (regcache_register_changed_p (regnum, prev_regs, this_regs))
 	    uiout->field_int (NULL, regnum);
 	}
     }
@@ -1117,11 +1111,7 @@ mi_cmd_data_list_changed_registers (char *command, char **argv, int argc)
 	  && gdbarch_register_name (gdbarch, regnum) != NULL
 	  && *gdbarch_register_name (gdbarch, regnum) != '\000')
 	{
-	  changed = register_changed_p (regnum, prev_regs, this_regs);
-	  if (changed < 0)
-	    error (_("-data-list-changed-registers: "
-		     "Unable to read register contents."));
-	  else if (changed)
+	  if (regcache_register_changed_p (regnum, prev_regs, this_regs))
 	    uiout->field_int (NULL, regnum);
 	}
       else
@@ -1130,34 +1120,6 @@ mi_cmd_data_list_changed_registers (char *command, char **argv, int argc)
   do_cleanups (cleanup);
 }

-static int
-register_changed_p (int regnum, struct regcache *prev_regs,
-		    struct regcache *this_regs)
-{
-  struct gdbarch *gdbarch = get_regcache_arch (this_regs);
-  gdb_byte prev_buffer[MAX_REGISTER_SIZE];
-  gdb_byte this_buffer[MAX_REGISTER_SIZE];
-  enum register_status prev_status;
-  enum register_status this_status;
-
-  /* First time through or after gdbarch change consider all registers
-     as changed.  */
-  if (!prev_regs || get_regcache_arch (prev_regs) != gdbarch)
-    return 1;
-
-  /* Get register contents and compare.  */
-  prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer);
-  this_status = regcache_cooked_read (this_regs, regnum, this_buffer);
-
-  if (this_status != prev_status)
-    return 1;
-  else if (this_status == REG_VALID)
-    return memcmp (prev_buffer, this_buffer,
-		   register_size (gdbarch, regnum)) != 0;
-  else
-    return 0;
-}
-
 /* Return a list of register number and value pairs.  The valid
    arguments expected are: a letter indicating the format in which to
    display the registers contents.  This can be one of: x
diff --git a/gdb/regcache.h b/gdb/regcache.h
index e5a7cf553279b8cc0d546ec1b8274cbf97e246d5..174aa0166f419b40b0509b1be5c6dce3c2373fd0 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -228,4 +228,11 @@ extern void regcache_cpy (struct regcache *dest, struct regcache *src);
 extern void registers_changed (void);
 extern void registers_changed_ptid (ptid_t);

+/* Return true if the register value of regnum differs between prev_regs and
+   this_regs.  prev_regs can be null, but this_regs must be set.  */
+
+extern bool regcache_register_changed_p (int regnum,
+					 struct regcache *prev_regs,
+					 struct regcache *this_regs);
+
 #endif /* REGCACHE_H */
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 9d28aa2c2114e0f1c52758bb2fbe9669a329c13e..96cc94ee7cfeb60d3b6541ca0b49871e6087aa8a 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1251,6 +1251,74 @@ regcache_write_pc (struct regcache *regcache, CORE_ADDR pc)
 }


+/* Return true if the register value of regnum differs between prev_regs and
+   this_regs.  prev_regs can be null, but this_regs must be set.  */
+
+bool
+regcache_register_changed_p (int regnum, struct regcache *prev_regs,
+			     struct regcache *this_regs)
+{
+  struct gdbarch *gdbarch;
+  gdb_assert (regnum >= 0);
+  gdb_assert (this_regs);
+
+  /* if there are no previous registers consider all registers as changed.  */
+  if (!prev_regs)
+    return true;
+
+  gdb_assert (regnum < prev_regs->descr->nr_cooked_registers);
+  gdb_assert (regnum < this_regs->descr->nr_cooked_registers);
+  gdbarch = get_regcache_arch (this_regs);
+
+  /* If arches don't match then consider all registers as changed.  */
+  if (gdbarch != get_regcache_arch (prev_regs))
+    return true;
+
+  if (regnum < this_regs->descr->nr_raw_registers)
+    {
+      enum register_status prev_status, this_status;
+
+      prev_status = (enum register_status) prev_regs->register_status[regnum];
+      this_status = (enum register_status) this_regs->register_status[regnum];
+
+      if (this_status != prev_status)
+	return true;
+      else if (this_status == REG_VALID)
+	return memcmp (register_buffer (prev_regs, regnum),
+		       register_buffer (this_regs, regnum),
+		       register_size (gdbarch, regnum)) != 0;
+      else
+	return false;
+    }
+  else
+    {
+      struct value *prev_value, *this_value;
+      bool ret;
+
+      prev_value = regcache_cooked_read_value (prev_regs, regnum);
+      this_value = regcache_cooked_read_value (this_regs, regnum);
+
+      if (value_optimized_out (prev_value)
+	  || value_optimized_out (this_value))
+	ret = value_optimized_out (prev_value)
+	      != value_optimized_out (this_value);
+      else if (!value_entirely_available (prev_value)
+	  || !value_entirely_available (this_value))
+	ret = value_entirely_available (prev_value)
+	      != value_entirely_available (this_value);
+      else
+	ret = memcmp (value_contents_raw (prev_value),
+		      value_contents_raw (this_value),
+		      register_size (gdbarch, regnum)) != 0;
+
+      release_value (prev_value);
+      value_free (prev_value);
+      release_value (this_value);
+      value_free (this_value);
+      return ret;
+    }
+}
+
 static void
 reg_flush_command (char *command, int from_tty)
 {