[PATCH] Fixup internal variables' endian (again)

[PATCH] Fixup internal variables' endian (again)

[Andrew STUBBS]

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

Hi all,

This is a rework of a patch I submitted last autumn. I have updated it 
to fit with the new convenience variable preservation and rethought the 
requirements.

The patch fixes up the endian of all integer and pointer internal 
variables and leaves all other types alone.

My understanding is that an internal variable cannot contain anything 
other than a built-in type. Any other value is merely a memory 
reference. Indeed, value_of_internalvar() goes out of it's way to ensure 
that values loaded from memory are never saved long term. The result is 
that there is no point in attempting to do anything with the endian of 
these values because they will always be of the same endian as the 
target/program being debugged.

The upshot is that, although it is possible to contrive an example [1] 
that causes gdb to print values in the wrong endian, in the normal case, 
in which the user does not change the endian except upon connection to 
the target, the values will always be presented as they expect. 
Including internal variables written prior to connection.

[1] Once connected to the target, write data in endian A, create a 
variable referencing that data, type 'set endian B' and the print the 
data - it comes out reversed.

If I have something wrong here please point it out.

Thanks

Andrew Stubbs

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

2006-03-03  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-03-03 17:16:03.000000000 +0000
+++ src/gdb/value.c	2006-03-03 17:16:20.000000000 +0000
@@ -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,49 @@ struct value *
 value_of_internalvar (struct internalvar *var)
 {
   struct value *val;
+  int i, j;
+  gdb_byte temp;
+  gdb_byte *array;
 
   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.
+
+     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.  */
+  if (var->endian != TARGET_BYTE_ORDER)
+    {
+      array = value_contents_raw (val);
+      switch (TYPE_CODE (value_type (val)))
+        {
+        case TYPE_CODE_INT:
+        case TYPE_CODE_PTR:
+          /* Reverse the bytes.  */
+          for (i=0,j=TYPE_LENGTH (value_enclosing_type (val))-1; i<j; i++,j--)
+	    {
+	      temp = array[j];
+	      array[j] = array[i];
+	      array[i] = temp;
+	    }
+	  break;
+	}
+    }
+
   return val;
 }
 
@@ -809,6 +847,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 +916,7 @@ 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-20 18:39:55.000000000 +0000
+++ src/gdb/value.h	2006-03-03 15:11:42.000000000 +0000
@@ -245,6 +245,7 @@ struct internalvar
   struct internalvar *next;
   char *name;
   struct value *value;
+  int endian;
 };
 
 \f

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

[Daniel Jacobowitz]

On Fri, Mar 03, 2006 at 06:01:32PM +0000, Andrew STUBBS wrote:
> Hi all,
> 
> This is a rework of a patch I submitted last autumn. I have updated it 
> to fit with the new convenience variable preservation and rethought the 
> requirements.
> 
> The patch fixes up the endian of all integer and pointer internal 
> variables and leaves all other types alone.

Hi Andrew,

This seems like a sensible thing to do now, and a sensible way to do
it.

> My understanding is that an internal variable cannot contain anything 
> other than a built-in type. Any other value is merely a memory 
> reference. Indeed, value_of_internalvar() goes out of it's way to ensure 
> that values loaded from memory are never saved long term. The result is 
> that there is no point in attempting to do anything with the endian of 
> these values because they will always be of the same endian as the 
> target/program being debugged.

I'm pretty sure that we can come up with ways to get structs into
internal variables, et cetera.  But, this is a best-effort thing;
I think it's OK as long as we do better than we do now, since it's
pretty apparent that what we do now is not useful.

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.

> +  if (var->endian != TARGET_BYTE_ORDER)
> +    {
> +      array = value_contents_raw (val);
> +      switch (TYPE_CODE (value_type (val)))

You should use type = check_typedef (value_type (val)) here; that's the
other easy case.

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

Formatting; you've skipped a lot of customary whitespace here.  We have
vertical space and we know how to use it!

	  for (i=0, j=TYPE_LENGTH (value_enclosing_type (val)) - 1;
	       i < j;
	       i++, j--)

Also, I think you used eight spaces instead of a tab here; the diff
looked funny.

> -      value_print (var->value, gdb_stdout, 0, Val_pretty_default);
> +      value_print (value_of_internalvar (var), gdb_stdout, 0, Val_pretty_default);

And this line needs to be wrapped, it's too long now.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Andrew STUBBS]

[-- 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

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

[Daniel Jacobowitz]

On Thu, Mar 30, 2006 at 12:14:34PM +0100, Andrew STUBBS wrote:
> 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.

Sorry, what I was trying to imply is that this might be a property of
the current GDB implementation, and for C, but I don't think it's a
reliable assumption into the future.

Creating a non-lazy internal variable of struct type is as simple as:

(gdb) print args
(gdb) set $foo = $1

You don't need a target for that; a symbol file will do.  And it will
last after disconnecting from one target and connecting to another.

You can do it without reading from the symbol file's memory too:

set $bar = (struct captured_main_args){1}

Vector registers have struct types, too, but without a symbol file you
currently can't get at that type.

So, if you've got a symbol file and the endianness is set wrong, you
can do this.  I guess that counts as going out of their way, but it
doesn't match what you wrote originally, which was that you could never
produce such internalvars.

> 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.

Except that's just not enough.  It depends on the floatformat.  We'd
need for each architecture to specify the big and little endian
floatformats, instead of the current endianness floatformat.  Well, or
define a new gdbarch hook; investigating the supported floatformats it
might work for everything but ARM.  OK, I take it back, it's easier
than it looks.

> It
> just so happens that the only scalar data not currently handled is
> floating point (AFAIK).

Have you looked at the TYPE_CODE_* values?  TYPE_CODE_ENUM,
TYPE_CODE_FLAGS, TYPE_CODE_FLT, TYPE_CODE_REF, TYPE_CODE_BOOL,
TYPE_CODE_RANGE, and I'm not sure about sets/bitstrings, but those
aren't used much now.

> I don't actually know of a host/target where
> this is a problem, but I'm sure there are some out there.

Where switching endianness affects the floating point format? 
Everything but ARM FPA, as far as I know.

> 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.

Good catch.
> +	  for (i=0, j=TYPE_LENGTH (type) - 1; i < j; i++, j--)

Could you put spaces around the equals signs, please.

OK with that change.


-- 
Daniel Jacobowitz
CodeSourcery

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

[Andrew STUBBS]

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

Daniel Jacobowitz wrote:
> So, if you've got a symbol file and the endianness is set wrong, you
> can do this.  I guess that counts as going out of their way, but it
> doesn't match what you wrote originally, which was that you could never
> produce such internalvars.

Ah well, there's always another, more devious, way to do everything.
But, as you say, once the symbol table is loaded there ought to be no
endian issues unless the user brings it on themselves.

The patch solves the problem where built-in types are set before the
file is loaded, which is what I wanted to fix.

>> I don't actually know of a host/target where
>> this is a problem, but I'm sure there are some out there.
> 
> Where switching endianness affects the floating point format? 
> Everything but ARM FPA, as far as I know.

Well, I've tested it on x86 and sh4, and on both those platforms you can
'set endian ..' as much as you like and the floats are always shown
correctly. On a Sparc the numbers do come through garbled in the wrong
endian.

> Could you put spaces around the equals signs, please.
> 
> OK with that change.

Thanks, committed with that change.

The final patch is attached.

Andrew Stubbs

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

2006-03-31  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-31 11:10:21.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