[rfc] Re-parse display expressions if architecture changes

[rfc] Re-parse display expressions if architecture changes

[Ulrich Weigand]

Hello,

I've run into a problem caused by the architecture being stored in a parsed
expression structure.  This shows up if you use a display expression that
depends on architecture features like register numbers, e.g.
  display/i $pc
and subsequently the current architecture changes.  This could happen e.g.
by connecting to a remote target that provides a target description with
extra registers, or in a multi-architecture scenario like Cell/B.E. when
control is passed to code using a different architecture.

I think the expectation with a display statement like the above should
clearly be that whenever the inferior stops, the current PC is displayed
according to whatever architecture is currently in force.

The patch below implements this by ensuring the expression is re-parsed
if the currnet architecture is different from the one the expression
was originally parsed in.

Tested on s390(x)-linux in conjunction with the "32-bit application
with 64-bit register set" patch.

Any comments?  I'm planning to apply this next week.

Bye,
Ulrich


ChangeLog:

	* printcmd.c: Include "arch-utils.h".
	(do_one_display): Re-parse expression if current architecture changed.


diff -urNp gdb-orig/gdb/printcmd.c gdb-head/gdb/printcmd.c
--- gdb-orig/gdb/printcmd.c	2009-12-18 17:27:13.000000000 +0100
+++ gdb-head/gdb/printcmd.c	2009-12-21 15:07:50.000000000 +0100
@@ -49,6 +49,7 @@
 #include "solib.h"
 #include "parser-defs.h"
 #include "charset.h"
+#include "arch-utils.h"
 
 #ifdef TUI
 #include "tui/tui.h"		/* For tui_active et.al.   */
@@ -1597,7 +1598,15 @@ do_one_display (struct display *d)
   if (d->enabled_p == 0)
     return;
 
-  if (d->exp == NULL)
+  /* The expression carries the architecture that was used at parse time.
+     This is a problem if the expression depends on architecture features
+     (e.g. register numbers), and the current architecture is now different.
+     For example, a display statement like "display/i $pc" is expected to
+     display the PC register of the current architecture, not the arch at
+     the time the display command was given.  Therefore, we re-parse the
+     expression if the current architecture has changed.  */
+  if (d->exp == NULL
+      || d->exp->gdbarch != get_current_arch ())
     {
       volatile struct gdb_exception ex;
       TRY_CATCH (ex, RETURN_MASK_ALL)
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Re-parse display expressions if architecture changes

[Tom Tromey]

>>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:

Ulrich> The patch below implements this by ensuring the expression is
Ulrich> re-parsed if the currnet architecture is different from the one
Ulrich> the expression was originally parsed in.

This seems like a reasonable idea to me.

Ulrich> Any comments?  I'm planning to apply this next week.

I think this needs to xfree the old expression to avoid a memory leak.

Tom

Re: [rfc] Re-parse display expressions if architecture changes

[Ulrich Weigand]

Tom Tromey wrote:
> >>>>> "Ulrich" == Ulrich Weigand <uweigand@de.ibm.com> writes:
> 
> Ulrich> The patch below implements this by ensuring the expression is
> Ulrich> re-parsed if the currnet architecture is different from the one
> Ulrich> the expression was originally parsed in.
> 
> This seems like a reasonable idea to me.
> 
> Ulrich> Any comments?  I'm planning to apply this next week.
> 
> I think this needs to xfree the old expression to avoid a memory leak.

Oops, thanks for noticing this.  Updated patch below.

Bye,
Ulrich


ChangeLog:

	* printcmd.c: Include "arch-utils.h".
	(do_one_display): Re-parse expression if current architecture changed.


diff -urNp gdb-orig/gdb/printcmd.c gdb-head/gdb/printcmd.c
--- gdb-orig/gdb/printcmd.c	2009-12-18 17:27:13.000000000 +0100
+++ gdb-head/gdb/printcmd.c	2009-12-22 14:23:27.000000000 +0100
@@ -49,6 +49,7 @@
 #include "solib.h"
 #include "parser-defs.h"
 #include "charset.h"
+#include "arch-utils.h"
 
 #ifdef TUI
 #include "tui/tui.h"		/* For tui_active et.al.   */
@@ -1597,6 +1598,20 @@ do_one_display (struct display *d)
   if (d->enabled_p == 0)
     return;
 
+  /* The expression carries the architecture that was used at parse time.
+     This is a problem if the expression depends on architecture features
+     (e.g. register numbers), and the current architecture is now different.
+     For example, a display statement like "display/i $pc" is expected to
+     display the PC register of the current architecture, not the arch at
+     the time the display command was given.  Therefore, we re-parse the
+     expression if the current architecture has changed.  */
+  if (d->exp != NULL && d->exp->gdbarch != get_current_arch ())
+    {
+      xfree (d->exp);
+      d->exp = NULL;
+      d->block = NULL;
+    }
+
   if (d->exp == NULL)
     {
       volatile struct gdb_exception ex;

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Re-parse display expressions if architecture changes

[Ulrich Weigand]

> ChangeLog:
> 
> 	* printcmd.c: Include "arch-utils.h".
> 	(do_one_display): Re-parse expression if current architecture changed.

I've checked this in now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com