Re: [PATCH] Fixup internal variables' endian (again)

[Andrew STUBBS <andrew.stubbs@st.com>]

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

Daniel Jacobowitz wrote:
> So, just minor comments.
> 
>> +  /* Values are always stored in the target's byte order.  When connected to a
>> +     target this will most likely always be correct, so there's normally no
>> +     need to worry about it.
>> +
>> +     However, internal variables can be set up before the target endian is
>> +     known and so may become out of date.  Fix it up before anybody sees.
>> +
>> +     Since internal variables cannot hold complex types, such as structs,
>> +     unions, arrays and strings - those are always held in target memory and
>> +     the variable just holds a reference, there is no need to worry about
>> +     those either.
>> +
>> +     Floating point values vary differently across endianness - many targets
>> +     just keep them the same.  If they do no work correctly on your host/target
>> +     then add support as required here.  */
> 
> How do you feel about replacing those last two paragraphs with:
> 
>     Internal variables usually hold simple scalar values, and we can
>     correct those.  More complex values (e.g. structures and floating
>     point types) are left alone, because they would be too complicated
>     to correct.
> 
> I don't think we really want someone to fix this by adding floating
> point support.

Having taken some time looking into this I thought that doing a brain
dump might be useful to other people. It's not that structs 'are too
complicated to correct' - AFAICT they are almost always right because
they require target memory to exist. The user has to go out of their way
to confuse the debugger before they are wrong.

As to the second point, why shouldn't people fix up broken values? If it
is displayed incorrectly then that is a bug and the comment says that
this is the place to fix it. People wouldn't have to 'add floating point
support' as such, merely reverse the byte order of some binary data. It
just so happens that the only scalar data not currently handled is
floating point (AFAIK). I don't actually know of a host/target where
this is a problem, but I'm sure there are some out there.

In fact, it might be as simple as:

          {
          case TYPE_CODE_INT:
          case TYPE_CODE_PTR:
+#ifdef ENDIAN_SENSITIVE_FLOATS
+	 case TYPE_CODE_FLOAT:
+#endif
            /* Reverse the bytes.  */
            for (i=0,j=TYPE_LENGTH (value_enclosing_type (val))-1; i<j;
i++,j--)

Anyway, at the end of the day it's just a comment, your text works, and
if anybody wants to fix something they don't need a comment to give them
permission.

Updated patch attached. I also noticed that I was using both type and 
enclosing_type - I don't think there was any reason for that so both now 
use enclosing type.

Andrew


[-- Attachment #2: endian.patch --]
[-- Type: text/plain, Size: 3170 bytes --]

2006-03-30  Andrew Stubbs  <andrew.stubbs@st.com>

	* value.h (struct internalvar): Add field 'endian'.
	* value.c (lookup_internalvar): Initialise endian.
	(value_of_internalvar): Flip the endian of built-in types if required.
	(set_internalvar): Set the endian.
	(show_convenience): Access the value through value_of_internalvar().


Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c	2006-02-01 23:14:10.000000000 +0000
+++ src/gdb/value.c	2006-03-30 12:13:27.000000000 +0100
@@ -755,6 +755,7 @@ lookup_internalvar (char *name)
   var = (struct internalvar *) xmalloc (sizeof (struct internalvar));
   var->name = concat (name, (char *)NULL);
   var->value = allocate_value (builtin_type_void);
+  var->endian = TARGET_BYTE_ORDER;
   release_value (var->value);
   var->next = internalvars;
   internalvars = var;
@@ -765,12 +766,46 @@ struct value *
 value_of_internalvar (struct internalvar *var)
 {
   struct value *val;
+  int i, j;
+  gdb_byte temp;
 
   val = value_copy (var->value);
   if (value_lazy (val))
     value_fetch_lazy (val);
   VALUE_LVAL (val) = lval_internalvar;
   VALUE_INTERNALVAR (val) = var;
+
+  /* Values are always stored in the target's byte order.  When connected to a
+     target this will most likely always be correct, so there's normally no
+     need to worry about it.
+
+     However, internal variables can be set up before the target endian is
+     known and so may become out of date.  Fix it up before anybody sees.
+
+     Internal variables usually hold simple scalar values, and we can
+     correct those.  More complex values (e.g. structures and floating
+     point types) are left alone, because they would be too complicated
+     to correct.  */
+
+  if (var->endian != TARGET_BYTE_ORDER)
+    {
+      gdb_byte *array = value_contents_raw (val);
+      struct type *type = check_typedef (value_enclosing_type (val));
+      switch (TYPE_CODE (type))
+	{
+	case TYPE_CODE_INT:
+	case TYPE_CODE_PTR:
+	  /* Reverse the bytes.  */
+	  for (i=0, j=TYPE_LENGTH (type) - 1; i < j; i++, j--)
+	    {
+	      temp = array[j];
+	      array[j] = array[i];
+	      array[i] = temp;
+	    }
+	  break;
+	}
+    }
+
   return val;
 }
 
@@ -809,6 +844,7 @@ set_internalvar (struct internalvar *var
      long.  */
   xfree (var->value);
   var->value = newval;
+  var->endian = TARGET_BYTE_ORDER;
   release_value (newval);
   /* End code which must not call error().  */
 }
@@ -877,7 +913,8 @@ show_convenience (char *ignore, int from
 	  varseen = 1;
 	}
       printf_filtered (("$%s = "), var->name);
-      value_print (var->value, gdb_stdout, 0, Val_pretty_default);
+      value_print (value_of_internalvar (var), gdb_stdout,
+		   0, Val_pretty_default);
       printf_filtered (("\n"));
     }
   if (!varseen)
Index: src/gdb/value.h
===================================================================
--- src.orig/gdb/value.h	2006-02-01 23:14:10.000000000 +0000
+++ src/gdb/value.h	2006-03-30 11:17:29.000000000 +0100
@@ -245,6 +245,7 @@ struct internalvar
   struct internalvar *next;
   char *name;
   struct value *value;
+  int endian;
 };
 
 \f