[PATCH] Fixup convenience variables on endian switch

[PATCH] Fixup convenience variables on endian switch

[Andrew STUBBS]

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

Hi,

The attached patch attempts to do the Right Thing for internal variables 
when 'set endian' is used (explicitly or implicitly).

I have not (and cannot) test it with all varieties of host machine and 
type, but I am confident that it works on x86 Linux and sparc Solaris 
for integer (including pointer) and float types.

I have run the regression test suite on i686-pc-linux-gnu native and 
sh-elf cross.

Demonstration:

Before

   (gdb) set $a = 1
   (gdb) set $b = (float)1.1
   (gdb) set $c = (double)1.2
   (gdb) set $d = (long double)1.3
   (gdb) set $e = (long long)10000000
   (gdb) show conv
   $e = 10000000
   $d = 1.3000000000000000444089209850062616
   $c = 1.2
   $b = 1.10000002
   $a = 1
   $trace_frame = -1
   $tpnum = 0
   (gdb) show endian
   The target endianness is set automatically (currently little endian)
   (gdb) set endian big
   The target is assumed to be big endian
   (gdb) show conv
   $e = -9180983664580755456
   $d = 1.3000000000000000444089209850062616
   $c = 1.2
   $b = 1.10000002
   $a = 16777216
   $trace_frame = -1
   $tpnum = 0
   (gdb) set endian little
   The target is assumed to be little endian
   (gdb) show conv
   $e = 10000000
   $d = 1.3000000000000000444089209850062616
   $c = 1.2
   $b = 1.10000002
   $a = 1
   $trace_frame = -1
   $tpnum = 0

After

   (gdb) set $a = 1
   (gdb) set $b = (float)1.1
   (gdb) set $c = (double)1.2
   (gdb) set $d = (long double)1.3
   (gdb) set $e = (long long)10000000
   (gdb) show conv
   $e = 10000000
   $d = 1.3000000000000000444089209850062616
   $c = 1.2
   $b = 1.10000002
   $a = 1
   $trace_frame = -1
   $tpnum = 0
   (gdb) show endian
   The target endianness is set automatically (currently little endian)
   (gdb) set endian big
   The target is assumed to be big endian
   (gdb) show conv
   $e = 10000000
   $d = 1.3000000000000000444089209850062616
   $c = 1.2
   $b = 1.10000002
   $a = 1
   $trace_frame = -1
   $tpnum = 0
   (gdb) set endian little
   The target is assumed to be little endian
   (gdb) show conv
   $e = 10000000
   $d = 1.3000000000000000444089209850062616
   $c = 1.2
   $b = 1.10000002
   $a = 1
   $trace_frame = -1
   $tpnum = 0

Andrew Stubbs

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

2005-11-16  Andrew Stubbs  <andrew.stubbs@st.com>

	* arch-utils.c: Include value.h.
	(set_endian): Call fixup_all_internalvars_endian().
	* value.h (struct internalvar): Add endian field.
	(fixup_all_internalvars_endian): New prototype.
	* value.c (fixup_internalvar_endian): New function.
	(fixup_all_internalvars_endian): New function.
	(lookup_internalvar): Initialize endian field.
	(set_internalvar): Reset endian field.
	* Makefile.in (arch-utils.o): Add value.h to dependencies.


Index: src/gdb/arch-utils.c
===================================================================
--- src.orig/gdb/arch-utils.c	2005-11-07 11:50:22.000000000 +0000
+++ src/gdb/arch-utils.c	2005-11-07 16:40:56.000000000 +0000
@@ -32,6 +32,7 @@
 #include "sim-regno.h"
 #include "gdbcore.h"
 #include "osabi.h"
+#include "value.h"
 
 #include "version.h"
 
@@ -417,6 +418,8 @@
     internal_error (__FILE__, __LINE__,
 		    _("set_endian: bad value"));
   show_endian (gdb_stdout, from_tty, NULL, NULL);
+
+  fixup_all_internalvars_endian ();
 }
 
 /* Functions to manipulate the architecture of the target */
Index: src/gdb/value.h
===================================================================
--- src.orig/gdb/value.h	2005-11-07 11:50:22.000000000 +0000
+++ src/gdb/value.h	2005-11-07 16:40:56.000000000 +0000
@@ -245,6 +245,7 @@
   struct internalvar *next;
   char *name;
   struct value *value;
+  int endian;
 };
 
 \f
@@ -416,6 +417,8 @@
 
 extern struct internalvar *lookup_internalvar (char *name);
 
+extern void fixup_all_internalvars_endian (void);
+
 extern int value_equal (struct value *arg1, struct value *arg2);
 
 extern int value_less (struct value *arg1, struct value *arg2);
Index: src/gdb/value.c
===================================================================
--- src.orig/gdb/value.c	2005-11-07 16:40:54.000000000 +0000
+++ src/gdb/value.c	2005-11-07 16:40:56.000000000 +0000
@@ -768,6 +768,49 @@
 }
 
 
+/* Reverse the endianess of the variable.
+   This is used when the the endianness is changed with 'set endian'
+   or when a new file of opposite endianess is loaded.  */
+
+static void
+fixup_internalvar_endian (struct internalvar *var)
+{
+  int i, j;
+  gdb_byte temp;
+  gdb_byte *array = value_contents_raw (var->value);
+
+  /* Don't do anything if the endian has not changed.
+     Also disregard FP types because they don't seem to vary with
+     endian - at least, not on i686 Linux or sparc Solaris.  */
+  if (var->endian != TARGET_BYTE_ORDER
+      && TYPE_CODE (value_type (var->value)) != TYPE_CODE_FLT)
+    {
+      /* Reverse the bytes.
+	 This may not be the right thing for some types
+	 so don't be afraid of messing with it if you encounter problems. */
+      for (i=0,j=TYPE_LENGTH (var->value->enclosing_type)-1; i<j; i++,j--)
+	{
+	  temp = array[j];
+	  array[j] = array[i];
+	  array[i] = temp;
+	}
+      var->endian = TARGET_BYTE_ORDER;
+    }
+}
+
+
+/* Swap the endianness of all the internal variables. */
+
+void
+fixup_all_internalvars_endian (void)
+{
+  struct internalvar *iv;
+
+  for (iv=internalvars; iv!=NULL; iv=iv->next)
+    fixup_internalvar_endian (iv);
+}
+
+
 /* Look up an internal variable with name NAME.  NAME should not
    normally include a dollar sign.
 
@@ -786,6 +829,7 @@
   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;
@@ -840,6 +884,7 @@
      long.  */
   xfree (var->value);
   var->value = newval;
+  var->endian = TARGET_BYTE_ORDER;
   release_value (newval);
   /* End code which must not call error().  */
 }
Index: src/gdb/Makefile.in
===================================================================
--- src.orig/gdb/Makefile.in	2005-11-07 16:40:46.000000000 +0000
+++ src/gdb/Makefile.in	2005-11-07 16:45:08.000000000 +0000
@@ -1732,7 +1732,7 @@
 arch-utils.o: arch-utils.c $(defs_h) $(arch_utils_h) $(buildsym_h) \
 	$(gdbcmd_h) $(inferior_h) $(gdb_string_h) $(regcache_h) \
 	$(gdb_assert_h) $(sim_regno_h) $(gdbcore_h) $(osabi_h) $(version_h) \
-	$(floatformat_h)
+	$(floatformat_h) $(value_h)
 arm-linux-nat.o: arm-linux-nat.c $(defs_h) $(inferior_h) $(gdbcore_h) \
 	$(gdb_string_h) $(regcache_h) $(arm_tdep_h) $(gregset_h) \
 	$(target_h) $(linux_nat_h)

Re: [PATCH] Fixup convenience variables on endian switch

[Jim Blandy]

What do you do with convenience variables whose values are structures?

Re: [PATCH] Fixup convenience variables on endian switch

[Daniel Jacobowitz]

In addition to Jim's question about structures, and the overall
question of whether this is worth doing...

On Wed, Nov 16, 2005 at 02:30:41PM +0000, Andrew STUBBS wrote:
> +  /* Don't do anything if the endian has not changed.
> +     Also disregard FP types because they don't seem to vary with
> +     endian - at least, not on i686 Linux or sparc Solaris.  */

That's not correct.  The only reason it appears that way is because
those targets normally support only one endian, so their gdbarch system
only selects one set of floatformats.  Changing endianness can change
the layout of the standard floating point types arbitrarily.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] Fixup convenience variables on endian switch

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> In addition to Jim's question about structures, and the overall
> question of whether this is worth doing...
> 
> On Wed, Nov 16, 2005 at 02:30:41PM +0000, Andrew STUBBS wrote:
> 
>>+  /* Don't do anything if the endian has not changed.
>>+     Also disregard FP types because they don't seem to vary with
>>+     endian - at least, not on i686 Linux or sparc Solaris.  */
> 
> 
> That's not correct.  The only reason it appears that way is because
> those targets normally support only one endian, so their gdbarch system
> only selects one set of floatformats.  Changing endianness can change
> the layout of the standard floating point types arbitrarily.

OK, so as it is floats work on x86 Linux, sparc Solaris and any other 
platform that happens to do it the same way. On other targets the 
situation is no worse (no different) than it is now.

Is there anything I can do to make this better? If there is no 
definitive way then maintainers/interested parties can always fix it per 
host as they see the problem. A comment to that effect can certainly go in.

As to the struct problem, I admit I had not considered that one. A test 
for it must go in of course.

The problem I am actually trying to solve is that we have addresses and 
such set up by a script that is sourced before the endian of the target 
they will be used with is known.

It seems like quite a lot of effort to recursively walk through a stuct. 
This is probably more because I don't know how than anything else. I am 
sure it is possible.

Also, there are unions to consider. These are even harder because the 
requirement that ints are flipped and floats remain the same (on some 
hosts) is incompatible.

I propose to add a test for struct and union types (bitfields? any 
others?) and leave them alone. Pointers to these types will continue to 
work properly of course.

Andrew

Re: [PATCH] Fixup convenience variables on endian switch

[Jim Blandy]

On 11/17/05, Andrew STUBBS <andrew.stubbs@st.com> wrote:
> The problem I am actually trying to solve is that we have addresses and
> such set up by a script that is sourced before the endian of the target
> they will be used with is known.

So you only really need to preserve convenience variables whose types
are builtin types, and don't go away when symbol tables are reloaded. 
Wouldn't it be simpler just to have clear_internalvars only clear
variables whose types belong to objfiles?

Or you could define a hook that runs a user-defined command when the
architecture changes.  Then your script could define a command that
sets up your variables, and have GDB run that command when the
architecture is known.  We'd have to think about the best time to run
the hook, but I'm sure something reasonable could be worked out.

Re: [PATCH] Fixup convenience variables on endian switch

[Daniel Jacobowitz]

On Thu, Nov 17, 2005 at 10:58:51AM -0800, Jim Blandy wrote:
> On 11/17/05, Andrew STUBBS <andrew.stubbs@st.com> wrote:
> > The problem I am actually trying to solve is that we have addresses and
> > such set up by a script that is sourced before the endian of the target
> > they will be used with is known.
> 
> So you only really need to preserve convenience variables whose types
> are builtin types, and don't go away when symbol tables are reloaded. 
> Wouldn't it be simpler just to have clear_internalvars only clear
> variables whose types belong to objfiles?

I think you've switched patches - this is about updating variables on
an endianness switch, not clearing them when we reload.  I think that
one way or another, we ought to preserve the values or discard the
convenience variables; leaving them corrupted fails my "can I explain
this behavior to a user" test.

> Or you could define a hook that runs a user-defined command when the
> architecture changes.  Then your script could define a command that
> sets up your variables, and have GDB run that command when the
> architecture is known.  We'd have to think about the best time to run
> the hook, but I'm sure something reasonable could be worked out.

Just seems nasty.  Why not preserve things that we know how to
preserve, and clear anything we don't?  We know how to preserve
scalars by reading them into a LONGEST, and floats by reading them into
a DOUBLEST.  Structures require complicated recursion and unions are
intractable.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] Fixup convenience variables on endian switch

[Andrew STUBBS]

Daniel Jacobowitz wrote:
>>So you only really need to preserve convenience variables whose types
>>are builtin types, and don't go away when symbol tables are reloaded. 
>>Wouldn't it be simpler just to have clear_internalvars only clear
>>variables whose types belong to objfiles?

Unless I am mistaken even builtin types have to be refreshed. It is a 
long time since I did this work, but I'm sure they are not as simple as 
you make out. At least, they didn't use to be ... this work originally 
comes from a 5.3 baseline.

> I think you've switched patches - this is about updating variables on
> an endianness switch, not clearing them when we reload.  I think that
> one way or another, we ought to preserve the values or discard the
> convenience variables; leaving them corrupted fails my "can I explain
> this behavior to a user" test.

The current behaviour is to leave them corrupted.

>>Or you could define a hook that runs a user-defined command when the
>>architecture changes.  Then your script could define a command that
>>sets up your variables, and have GDB run that command when the
>>architecture is known.  We'd have to think about the best time to run
>>the hook, but I'm sure something reasonable could be worked out.

This doesn't actually seem easier - I have already done the work one way 
and it is totally reliable for builtin types (once configured for each 
host anyway). It also makes the user interface considerably less 
friendly and then there's there's Daniel's test to be passed - the user 
will say "why can't it Just Work?"

> Just seems nasty.  Why not preserve things that we know how to
> preserve, and clear anything we don't?  We know how to preserve
> scalars by reading them into a LONGEST, and floats by reading them into
> a DOUBLEST.  Structures require complicated recursion and unions are
> intractable.

I would prefer we didn't actually clear the value - we currently get it 
back if the endian switches again.

The more I think about this the more I think it might be nice to 
preserve the value of difficult types in the background, should the 
endian switch back (or type return from the dead), and present the value 
to the user as a message '$v = void <wrong endian>' (or '$v = void <type 
unknown>').

Just a thought ....

Andrew

Re: [PATCH] Fixup convenience variables on endian switch

[Jim Blandy]

On 11/17/05, Daniel Jacobowitz <drow@false.org> wrote:
> On Thu, Nov 17, 2005 at 10:58:51AM -0800, Jim Blandy wrote:
> > On 11/17/05, Andrew STUBBS <andrew.stubbs@st.com> wrote:
> > > The problem I am actually trying to solve is that we have addresses and
> > > such set up by a script that is sourced before the endian of the target
> > > they will be used with is known.
> >
> > So you only really need to preserve convenience variables whose types
> > are builtin types, and don't go away when symbol tables are reloaded.
> > Wouldn't it be simpler just to have clear_internalvars only clear
> > variables whose types belong to objfiles?
>
> I think you've switched patches - this is about updating variables on
> an endianness switch, not clearing them when we reload.

No, it's popping up a level to figure out how to best address the
original problem.

In light of your stories about losing values, and given Andrew's
description of the troubles that motivated him, I'd agree that simply
always preserving the values of convenience variables that we know how
to preserve is the way to go.