[PATCH] m32c-tdep.c: Add virtual_frame_pointer function

[PATCH] m32c-tdep.c: Add virtual_frame_pointer function

[Corinna Vinschen]

Hi,

the below patch adds a virtual_frame_pointer function to m32c-tdep.c,
to avoid that legacy_virtual_frame_pointer is called from tracepoint.c,
function encode_actions().  The legacy function either expects a 
DEPRECATED_FP_REGNUM function, or it expects SP_REGNUM <= NUM_REGS,
which is not the case for the m32c code which includes serious
register banking.  The patch avoids all GDB internal errors in the
gdb.trace testsuite and allows to return PASSes for all these testcases,
except a single one (FAIL: gdb.trace/packetlen.exp: setup collect actions).

Ok to apply?


Thanks,
Corinna


	* m32c-tdep.c (m32c_virtual_frame_pointer): New function.
	(m32c_gdbarch_init): Add set_gdbarch_virtual_frame_pointer call.


Index: m32c-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/m32c-tdep.c,v
retrieving revision 1.1
diff -u -p -r1.1 m32c-tdep.c
--- m32c-tdep.c	20 Apr 2006 23:18:48 -0000	1.1
+++ m32c-tdep.c	28 Jun 2006 13:20:38 -0000
@@ -2475,6 +2475,14 @@ m32c_m16c_pointer_to_address (struct typ
   return ptr;
 }
 
+void
+m32c_virtual_frame_pointer (CORE_ADDR pc,
+			    int *frame_regnum,
+			    LONGEST *frame_offset)
+{
+  *frame_regnum = SP_REGNUM;
+  *frame_offset = 0;
+}
 
 \f
 /* Initialization.  */
@@ -2539,6 +2547,8 @@ m32c_gdbarch_init (struct gdbarch_info i
   /* Trampolines.  */
   set_gdbarch_skip_trampoline_code (arch, m32c_skip_trampoline_code);
 
+  set_gdbarch_virtual_frame_pointer (arch, m32c_virtual_frame_pointer);
+
   return arch;
 }
 

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [PATCH] m32c-tdep.c: Add virtual_frame_pointer function

[Mark Kettenis]

> Date: Wed, 28 Jun 2006 15:29:18 +0200
> From: Corinna Vinschen <vinschen@redhat.com>
> 
> Hi,
> 
> the below patch adds a virtual_frame_pointer function to m32c-tdep.c,
> to avoid that legacy_virtual_frame_pointer is called from tracepoint.c,
> function encode_actions().  The legacy function either expects a 
> DEPRECATED_FP_REGNUM function, or it expects SP_REGNUM <= NUM_REGS,
> which is not the case for the m32c code which includes serious
> register banking.  The patch avoids all GDB internal errors in the
> gdb.trace testsuite and allows to return PASSes for all these testcases,
> except a single one (FAIL: gdb.trace/packetlen.exp: setup collect actions).
> 
> Ok to apply?

You probably should change legacy_virtual_frame_pointer to check for
SP_REGNUM < NUM_REGS + NUM_PSEUDO_REGS instead.

Re: [PATCH] m32c-tdep.c: Add virtual_frame_pointer function

[DJ Delorie]

> You probably should change legacy_virtual_frame_pointer to check for
> SP_REGNUM < NUM_REGS + NUM_PSEUDO_REGS instead.

Note that m32c has two stack pointers, and the one that's used depends
on the CPU flags.  This is separate from the flag that determines
which register bank is in use.

Re: [PATCH] m32c-tdep.c: Add virtual_frame_pointer function

[Daniel Jacobowitz]

On Wed, Jun 28, 2006 at 11:29:41PM +0200, Mark Kettenis wrote:
> > Date: Wed, 28 Jun 2006 15:29:18 +0200
> > From: Corinna Vinschen <vinschen@redhat.com>
> > 
> > Hi,
> > 
> > the below patch adds a virtual_frame_pointer function to m32c-tdep.c,
> > to avoid that legacy_virtual_frame_pointer is called from tracepoint.c,
> > function encode_actions().  The legacy function either expects a 
> > DEPRECATED_FP_REGNUM function, or it expects SP_REGNUM <= NUM_REGS,
> > which is not the case for the m32c code which includes serious
> > register banking.  The patch avoids all GDB internal errors in the
> > gdb.trace testsuite and allows to return PASSes for all these testcases,
> > except a single one (FAIL: gdb.trace/packetlen.exp: setup collect actions).
> > 
> > Ok to apply?
> 
> You probably should change legacy_virtual_frame_pointer to check for
> SP_REGNUM < NUM_REGS + NUM_PSEUDO_REGS instead.

Neither, I don't think!  Did this just turn up in a testsuite run, or
did you actually try it with tracepoints, Corinna?

The assertion seems correct to me.  The frame register number is going
to get sent to the remote target system.  We shouldn't ever do that for
pseudo registers, because the target probably doesn't know how to
generate their contents... otherwise they'd be real registers.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] m32c-tdep.c: Add virtual_frame_pointer function

[Corinna Vinschen]

On Jun 28 22:34, Daniel Jacobowitz wrote:
> On Wed, Jun 28, 2006 at 11:29:41PM +0200, Mark Kettenis wrote:
> > > Date: Wed, 28 Jun 2006 15:29:18 +0200
> > > From: Corinna Vinschen <XXXXXXXX@XXXXXX.XXX>
> > > 
> > > the below patch adds a virtual_frame_pointer function to m32c-tdep.c,
> > > to avoid that legacy_virtual_frame_pointer is called from tracepoint.c,
> > > [...]
> > 
> > You probably should change legacy_virtual_frame_pointer to check for
> > SP_REGNUM < NUM_REGS + NUM_PSEUDO_REGS instead.
> 
> Neither, I don't think!  Did this just turn up in a testsuite run, or
> did you actually try it with tracepoints, Corinna?

It fixed the testsuite run.  I assumed that the gdb.trace testsuite
is testing..., well, tracepoints.

> The assertion seems correct to me.  The frame register number is going
> to get sent to the remote target system.  We shouldn't ever do that for
> pseudo registers, because the target probably doesn't know how to
> generate their contents... otherwise they'd be real registers.

I see the point.  I rewrote the m32c_virtual_frame_pointer function to
return a valid register/offset pair from the banked fb resp. sp
registers.  Does that look ok?


Corinna


	* m32c-tdep.c (m32c_banked_register): New function.
	(m32c_banked_read): Use m32c_banked_register function to evaluate
	real register number.
	(m32c_banked_write): Ditto.
	(m32c_virtual_frame_pointer): New function.
	(m32c_gdbarch_init): Add set_gdbarch_virtual_frame_pointer call.


Index: m32c-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/m32c-tdep.c,v
retrieving revision 1.1
diff -u -p -r1.1 m32c-tdep.c
--- m32c-tdep.c	20 Apr 2006 23:18:48 -0000	1.1
+++ m32c-tdep.c	29 Jun 2006 10:46:38 -0000
@@ -340,6 +340,13 @@ m32c_read_flg (struct regcache *cache)
 }
 
 
+/* Evaluate the real register number of a banked register.  */
+m32c_banked_register (struct m32c_reg *reg, struct regcache *cache)
+{
+  return ((m32c_read_flg (cache) & reg->n) ? reg->ry : reg->rx);
+}
+
+
 /* Move the value of a banked register from CACHE to BUF.
    If the value of the 'flg' register in CACHE has any of the bits
    masked in REG->n set, then read REG->ry.  Otherwise, read
@@ -347,8 +354,7 @@ m32c_read_flg (struct regcache *cache)
 static void
 m32c_banked_read (struct m32c_reg *reg, struct regcache *cache, void *buf)
 {
-  struct m32c_reg *bank_reg
-    = ((m32c_read_flg (cache) & reg->n) ? reg->ry : reg->rx);
+  struct m32c_reg *bank_reg = m32c_banked_register (reg, cache);
   regcache_raw_read (cache, bank_reg->num, buf);
 }
 
@@ -360,8 +366,7 @@ m32c_banked_read (struct m32c_reg *reg, 
 static void
 m32c_banked_write (struct m32c_reg *reg, struct regcache *cache, void *buf)
 {
-  struct m32c_reg *bank_reg
-    = ((m32c_read_flg (cache) & reg->n) ? reg->ry : reg->rx);
+  struct m32c_reg *bank_reg = m32c_banked_register (reg, cache);
   regcache_raw_write (cache, bank_reg->num, (const void *) buf);
 }
 
@@ -2475,6 +2480,40 @@ m32c_m16c_pointer_to_address (struct typ
   return ptr;
 }
 
+void
+m32c_virtual_frame_pointer (CORE_ADDR pc,
+			    int *frame_regnum,
+			    LONGEST *frame_offset)
+{
+  char *name;
+  CORE_ADDR func_addr, func_end, sal_end;
+  struct m32c_prologue p;
+
+  struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
+  
+  if (!find_pc_partial_function (pc, &name, &func_addr, &func_end))
+    internal_error (__FILE__, __LINE__, _("No virtual frame pointer available"));
+
+  m32c_analyze_prologue (current_gdbarch, func_addr, pc, &p);
+  switch (p.kind)
+    {
+    case prologue_with_frame_ptr:
+      *frame_regnum = m32c_banked_register (tdep->fb, current_regcache)->num;
+      *frame_offset = p.frame_ptr_offset;
+      break;
+    case prologue_sans_frame_ptr:
+      *frame_regnum = m32c_banked_register (tdep->sp, current_regcache)->num;
+      *frame_offset = p.frame_size;
+      break;
+    default:
+      *frame_regnum = m32c_banked_register (tdep->sp, current_regcache)->num;
+      *frame_offset = 0;
+      break;
+    }
+  /* Sanity check */
+  if (*frame_regnum > NUM_REGS)
+    internal_error (__FILE__, __LINE__, _("No virtual frame pointer available"));
+}
 
 \f
 /* Initialization.  */
@@ -2539,6 +2578,8 @@ m32c_gdbarch_init (struct gdbarch_info i
   /* Trampolines.  */
   set_gdbarch_skip_trampoline_code (arch, m32c_skip_trampoline_code);
 
+  set_gdbarch_virtual_frame_pointer (arch, m32c_virtual_frame_pointer);
+
   return arch;
 }
 

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [PATCH] m32c-tdep.c: Add virtual_frame_pointer function

[Corinna Vinschen]

On Jun 29 12:51, Corinna Vinschen wrote:
> I see the point.  I rewrote the m32c_virtual_frame_pointer function to
> return a valid register/offset pair from the banked fb resp. sp
> registers.  Does that look ok?

Oops!  There was a copy/paste error in my patch, sorry about that.
New patch below.


Corinna


 	* m32c-tdep.c (m32c_banked_register): New function.
 	(m32c_banked_read): Use m32c_banked_register function to evaluate
 	real register number.
 	(m32c_banked_write): Ditto.
 	(m32c_virtual_frame_pointer): New function.
 	(m32c_gdbarch_init): Add set_gdbarch_virtual_frame_pointer call.
 
 
Index: m32c-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/m32c-tdep.c,v
retrieving revision 1.1
diff -u -p -r1.1 m32c-tdep.c
--- m32c-tdep.c	20 Apr 2006 23:18:48 -0000	1.1
+++ m32c-tdep.c	29 Jun 2006 13:07:25 -0000
@@ -340,6 +340,14 @@ m32c_read_flg (struct regcache *cache)
 }
 
 
+/* Evaluate the real register number of a banked register.  */
+static struct m32c_reg *
+m32c_banked_register (struct m32c_reg *reg, struct regcache *cache)
+{
+  return ((m32c_read_flg (cache) & reg->n) ? reg->ry : reg->rx);
+}
+
+
 /* Move the value of a banked register from CACHE to BUF.
    If the value of the 'flg' register in CACHE has any of the bits
    masked in REG->n set, then read REG->ry.  Otherwise, read
@@ -347,8 +355,7 @@ m32c_read_flg (struct regcache *cache)
 static void
 m32c_banked_read (struct m32c_reg *reg, struct regcache *cache, void *buf)
 {
-  struct m32c_reg *bank_reg
-    = ((m32c_read_flg (cache) & reg->n) ? reg->ry : reg->rx);
+  struct m32c_reg *bank_reg = m32c_banked_register (reg, cache);
   regcache_raw_read (cache, bank_reg->num, buf);
 }
 
@@ -360,8 +367,7 @@ m32c_banked_read (struct m32c_reg *reg, 
 static void
 m32c_banked_write (struct m32c_reg *reg, struct regcache *cache, void *buf)
 {
-  struct m32c_reg *bank_reg
-    = ((m32c_read_flg (cache) & reg->n) ? reg->ry : reg->rx);
+  struct m32c_reg *bank_reg = m32c_banked_register (reg, cache);
   regcache_raw_write (cache, bank_reg->num, (const void *) buf);
 }
 
@@ -2475,6 +2481,40 @@ m32c_m16c_pointer_to_address (struct typ
   return ptr;
 }
 
+void
+m32c_virtual_frame_pointer (CORE_ADDR pc,
+			    int *frame_regnum,
+			    LONGEST *frame_offset)
+{
+  char *name;
+  CORE_ADDR func_addr, func_end, sal_end;
+  struct m32c_prologue p;
+
+  struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
+  
+  if (!find_pc_partial_function (pc, &name, &func_addr, &func_end))
+    internal_error (__FILE__, __LINE__, _("No virtual frame pointer available"));
+
+  m32c_analyze_prologue (current_gdbarch, func_addr, pc, &p);
+  switch (p.kind)
+    {
+    case prologue_with_frame_ptr:
+      *frame_regnum = m32c_banked_register (tdep->fb, current_regcache)->num;
+      *frame_offset = p.frame_ptr_offset;
+      break;
+    case prologue_sans_frame_ptr:
+      *frame_regnum = m32c_banked_register (tdep->sp, current_regcache)->num;
+      *frame_offset = p.frame_size;
+      break;
+    default:
+      *frame_regnum = m32c_banked_register (tdep->sp, current_regcache)->num;
+      *frame_offset = 0;
+      break;
+    }
+  /* Sanity check */
+  if (*frame_regnum > NUM_REGS)
+    internal_error (__FILE__, __LINE__, _("No virtual frame pointer available"));
+}
 
 \f
 /* Initialization.  */
@@ -2539,6 +2579,8 @@ m32c_gdbarch_init (struct gdbarch_info i
   /* Trampolines.  */
   set_gdbarch_skip_trampoline_code (arch, m32c_skip_trampoline_code);
 
+  set_gdbarch_virtual_frame_pointer (arch, m32c_virtual_frame_pointer);
+
   return arch;
 }
 

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [PATCH] m32c-tdep.c: Add virtual_frame_pointer function

[Daniel Jacobowitz]

On Thu, Jun 29, 2006 at 03:08:44PM +0200, Corinna Vinschen wrote:
> On Jun 29 12:51, Corinna Vinschen wrote:
> > I see the point.  I rewrote the m32c_virtual_frame_pointer function to
> > return a valid register/offset pair from the banked fb resp. sp
> > registers.  Does that look ok?
> 
> Oops!  There was a copy/paste error in my patch, sorry about that.
> New patch below.
> 
> 
> Corinna
> 
> 
>  	* m32c-tdep.c (m32c_banked_register): New function.
>  	(m32c_banked_read): Use m32c_banked_register function to evaluate
>  	real register number.
>  	(m32c_banked_write): Ditto.
>  	(m32c_virtual_frame_pointer): New function.
>  	(m32c_gdbarch_init): Add set_gdbarch_virtual_frame_pointer call.

Yes, this looks fine to me.  Thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] m32c-tdep.c: Add virtual_frame_pointer function

[Corinna Vinschen]

On Jul 12 15:13, Daniel Jacobowitz wrote:
> On Thu, Jun 29, 2006 at 03:08:44PM +0200, Corinna Vinschen wrote:
> >  	* m32c-tdep.c (m32c_banked_register): New function.
> >  	(m32c_banked_read): Use m32c_banked_register function to evaluate
> >  	real register number.
> >  	(m32c_banked_write): Ditto.
> >  	(m32c_virtual_frame_pointer): New function.
> >  	(m32c_gdbarch_init): Add set_gdbarch_virtual_frame_pointer call.
> 
> Yes, this looks fine to me.  Thanks.

Thanks, applied.


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

Re: [PATCH] m32c-tdep.c: Add virtual_frame_pointer function

[Daniel Jacobowitz]

On Thu, Jun 29, 2006 at 12:51:16PM +0200, Corinna Vinschen wrote:
> It fixed the testsuite run.  I assumed that the gdb.trace testsuite
> is testing..., well, tracepoints.

It's got two modes.  If the stub doesn't support tracepoints it does a
bunch of testing of the GDB commands; if it does, it actually tests
tracepoints a bit.  So which you were testing depends on whether your
stub supported tracepoints or not.

-- 
Daniel Jacobowitz
CodeSourcery