[RFA] mips32_next_pc overdue cleanup

[RFA] mips32_next_pc overdue cleanup

[Daniel Jacobowitz]

This patch is half style cleanups and half obvious fixes, but there's so
many I'd appreciate more eyes.  According to the masks in the opcodes
directory, these changes are rigt, and what we had before can't possibly
work.  Especially the fact that we would never, ever predict a BNE as taken
before these changes convinced me that they really were correct and this
code had never been particularly tested.  Comments?

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

2001-07-04  Daniel Jacobowitz  <drow@mvista.com>
	* mips-tdep.c (mips32_op): Correct offset.
	(itype_op): Likewise.
	(itype_rs): Fix formatting.
	(itype_immediate): Fix formatting.
	(jtype_op): Correct offset.
	(jtype_target): Fix formatting.
	(rtype_op): Correct offset.
	(rtype_rs): Fix formatting.
	(rtype_rt): Likewise.
	(rtype_rd): Likewise.
	(rtype_shamt): Likewise.
	(rtype_funct): Likewise.

	(mips32_next_pc):  Fix formatting and comments.  Recognize
	coprocessor 1 branches.  Check the correct field for BLT family
	branches.  Use itype_rt instead of itype_rs for the second register
	of a BNE or BNEL branch.  Move (unreachable) default case.

Index: gdb/mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.54
diff -u -r1.54 mips-tdep.c
--- mips-tdep.c	2001/06/16 20:00:24	1.54
+++ mips-tdep.c	2001/07/04 18:15:03
@@ -669,21 +669,21 @@
 
 
 /* These the fields of 32 bit mips instructions */
-#define mips32_op(x) (x >> 25)
-#define itype_op(x) (x >> 25)
-#define itype_rs(x) ((x >> 21)& 0x1f)
+#define mips32_op(x) (x >> 26)
+#define itype_op(x) (x >> 26)
+#define itype_rs(x) ((x >> 21) & 0x1f)
 #define itype_rt(x) ((x >> 16) & 0x1f)
-#define itype_immediate(x) ( x & 0xffff)
+#define itype_immediate(x) (x & 0xffff)
 
-#define jtype_op(x) (x >> 25)
-#define jtype_target(x) ( x & 0x03fffff)
+#define jtype_op(x) (x >> 26)
+#define jtype_target(x) (x & 0x03ffffff)
 
-#define rtype_op(x) (x >>25)
-#define rtype_rs(x) ((x>>21) & 0x1f)
-#define rtype_rt(x) ((x>>16)  & 0x1f)
-#define rtype_rd(x) ((x>>11) & 0x1f)
-#define rtype_shamt(x) ((x>>6) & 0x1f)
-#define rtype_funct(x) (x & 0x3f )
+#define rtype_op(x) (x >> 26)
+#define rtype_rs(x) ((x >> 21) & 0x1f)
+#define rtype_rt(x) ((x >> 16) & 0x1f)
+#define rtype_rd(x) ((x >> 11) & 0x1f)
+#define rtype_shamt(x) ((x >> 6) & 0x1f)
+#define rtype_funct(x) (x & 0x3f)
 
 static CORE_ADDR
 mips32_relative_offset (unsigned long inst)
@@ -706,25 +706,39 @@
   unsigned long inst;
   int op;
   inst = mips_fetch_instruction (pc);
-  if ((inst & 0xe0000000) != 0)	/* Not a special, junp or branch instruction */
+  if ((inst & 0xe0000000) != 0)	/* Not a special, jump or branch instruction */
     {
-      if ((inst >> 27) == 5)	/* BEQL BNEZ BLEZL BGTZE , bits 0101xx */
+      if (itype_op (inst) >> 2 == 5)
+				/* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */
 	{
-	  op = ((inst >> 25) & 0x03);
+	  op = (itype_op (inst) & 0x03);
 	  switch (op)
 	    {
-	    case 0:
-	      goto equal_branch;	/* BEQL   */
-	    case 1:
-	      goto neq_branch;	/* BNEZ   */
-	    case 2:
-	      goto less_branch;	/* BLEZ   */
-	    case 3:
-	      goto greater_branch;	/* BGTZ */
+	    case 0:		/* BEQL */
+	      goto equal_branch;
+	    case 1:		/* BNEL */
+	      goto neq_branch;
+	    case 2:		/* BLEZL */
+	      goto less_branch;
+	    case 3:		/* BGTZ */
+	      goto greater_branch;
 	    default:
 	      pc += 4;
 	    }
 	}
+      else if (itype_op (inst) == 17 && itype_rs (inst) == 8)
+				/* BC1F, BC1FL, BC1T, BC1TL: 010001 01000 */
+	{
+	  int tf = itype_rt (inst) & 0x01;
+	  int cnum = itype_rt (inst) >> 2;
+	  int fcrcs = read_signed_register (FCRCS_REGNUM);
+	  int cond = ((fcrcs >> 24) & 0x0e) | ((fcrcs >> 23) & 0x01);
+
+	  if (((cond >> cnum) & 0x01) == tf)
+	    pc += mips32_relative_offset (inst) + 4;
+	  else
+	    pc += 8;
+	}
       else
 	pc += 4;		/* Not a branch, next instruction is easy */
     }
@@ -732,7 +746,7 @@
     {				/* This gets way messy */
 
       /* Further subdivide into SPECIAL, REGIMM and other */
-      switch (op = ((inst >> 26) & 0x07))	/* extract bits 28,27,26 */
+      switch (op = itype_op (inst) & 0x07)	/* extract bits 28,27,26 */
 	{
 	case 0:		/* SPECIAL */
 	  op = rtype_funct (inst);
@@ -747,15 +761,15 @@
 	      pc += 4;
 	    }
 
-	  break;		/* end special */
+	  break;	/* end SPECIAL */
 	case 1:		/* REGIMM */
 	  {
-	    op = jtype_op (inst);	/* branch condition */
-	    switch (jtype_op (inst))
+	    op = itype_rt (inst);	/* branch condition */
+	    switch (op)
 	      {
 	      case 0:		/* BLTZ */
-	      case 2:		/* BLTXL */
-	      case 16:		/* BLTZALL */
+	      case 2:		/* BLTZL */
+	      case 16:		/* BLTZAL */
 	      case 18:		/* BLTZALL */
 	      less_branch:
 		if (read_signed_register (itype_rs (inst)) < 0)
@@ -763,7 +777,7 @@
 		else
 		  pc += 8;	/* after the delay slot */
 		break;
-	      case 1:		/* GEZ */
+	      case 1:		/* BGEZ */
 	      case 3:		/* BGEZL */
 	      case 17:		/* BGEZAL */
 	      case 19:		/* BGEZALL */
@@ -773,19 +787,19 @@
 		else
 		  pc += 8;	/* after the delay slot */
 		break;
-		/* All of the other intructions in the REGIMM catagory */
+		/* All of the other instructions in the REGIMM category */
 	      default:
 		pc += 4;
 	      }
 	  }
-	  break;		/* end REGIMM */
+	  break;	/* end REGIMM */
 	case 2:		/* J */
 	case 3:		/* JAL */
 	  {
 	    unsigned long reg;
 	    reg = jtype_target (inst) << 2;
+	    /* Upper four bits get never changed... */
 	    pc = reg + ((pc + 4) & 0xf0000000);
-	    /* Whats this mysterious 0xf000000 adjustment ??? */
 	  }
 	  break;
 	  /* FIXME case JALX : */
@@ -796,7 +810,7 @@
 	    /* Add 1 to indicate 16 bit mode - Invert ISA mode */
 	  }
 	  break;		/* The new PC will be alternate mode */
-	case 4:		/* BEQ , BEQL */
+	case 4:		/* BEQ, BEQL */
 	equal_branch:
 	  if (read_signed_register (itype_rs (inst)) ==
 	      read_signed_register (itype_rt (inst)))
@@ -804,15 +818,15 @@
 	  else
 	    pc += 8;
 	  break;
-	case 5:		/* BNE , BNEL */
+	case 5:		/* BNE, BNEL */
 	neq_branch:
 	  if (read_signed_register (itype_rs (inst)) !=
-	      read_signed_register (itype_rs (inst)))
+	      read_signed_register (itype_rt (inst)))
 	    pc += mips32_relative_offset (inst) + 4;
 	  else
 	    pc += 8;
 	  break;
-	case 6:		/* BLEZ , BLEZL */
+	case 6:		/* BLEZ, BLEZL */
 	less_zero_branch:
 	  if (read_signed_register (itype_rs (inst) <= 0))
 	    pc += mips32_relative_offset (inst) + 4;
@@ -820,14 +834,13 @@
 	    pc += 8;
 	  break;
 	case 7:
-	greater_branch:	/* BGTZ BGTZL */
+	default:
+	greater_branch:	/* BGTZ, BGTZL */
 	  if (read_signed_register (itype_rs (inst) > 0))
 	    pc += mips32_relative_offset (inst) + 4;
 	  else
 	    pc += 8;
 	  break;
-	default:
-	  pc += 8;
 	}			/* switch */
     }				/* else */
   return pc;

Re: [RFA] mips32_next_pc overdue cleanup

[Eli Zaretskii]

On Wed, 4 Jul 2001, Daniel Jacobowitz wrote:

> According to the masks in the opcodes
> directory, these changes are rigt, and what we had before can't possibly
> work.  Especially the fact that we would never, ever predict a BNE as taken
> before these changes convinced me that they really were correct and this
> code had never been particularly tested.  Comments?

I'd appreciate a short test case for this.  I'd like to verify that
the previous stuff indeed didn't work and that the new stuff does.

Assuming that this code is run on SGI Irix, that is.

Re: [RFA] mips32_next_pc overdue cleanup

[Daniel Jacobowitz]

On Thu, Jul 05, 2001 at 09:46:32AM +0300, Eli Zaretskii wrote:
> 
> On Wed, 4 Jul 2001, Daniel Jacobowitz wrote:
> 
> > According to the masks in the opcodes
> > directory, these changes are rigt, and what we had before can't possibly
> > work.  Especially the fact that we would never, ever predict a BNE as taken
> > before these changes convinced me that they really were correct and this
> > code had never been particularly tested.  Comments?
> 
> I'd appreciate a short test case for this.  I'd like to verify that
> the previous stuff indeed didn't work and that the new stuff does.
> 
> Assuming that this code is run on SGI Irix, that is.

I'll try to make one.  I'm not immediately sure how.

It looks almost as if this code was for an unreleased port inside
Cygnus - anyone know if that's true?  I see active work on it in 1998
and 2000 in the ChangeLogs, and no mention of removing calls to it
- but nothing uses it at all.

Re: [RFA] mips32_next_pc overdue cleanup

[Andrew Cagney]

Eli wrote:

> According to the masks in the opcodes
>> directory, these changes are rigt, and what we had before can't possibly
>> work.  Especially the fact that we would never, ever predict a BNE as taken
>> before these changes convinced me that they really were correct and this
>> code had never been particularly tested.  Comments?
> 
> 
> I'd appreciate a short test case for this.  I'd like to verify that
> the previous stuff indeed didn't work and that the new stuff does.
> 
> Assuming that this code is run on SGI Irix, that is.


I'm wondering the same thing.

> I'll try to make one.  I'm not immediately sure how.
> 
> It looks almost as if this code was for an unreleased port inside
> Cygnus - anyone know if that's true?  I see active work on it in 1998
> and 2000 in the ChangeLogs, and no mention of removing calls to it
> - but nothing uses it at all.


I'll do some digging.

One thing, it is very easy for a software single step function to be 
implemented yet never tested - everything supports h/w singlestep 
now-a-days.

	Andrew

Re: [RFA] mips32_next_pc overdue cleanup

[Daniel Jacobowitz]

On Thu, Jul 05, 2001 at 04:29:07PM -0400, Andrew Cagney wrote:
> Eli wrote:
> 
> > According to the masks in the opcodes
> >> directory, these changes are rigt, and what we had before can't possibly
> >> work.  Especially the fact that we would never, ever predict a BNE as taken
> >> before these changes convinced me that they really were correct and this
> >> code had never been particularly tested.  Comments?
> > 
> > 
> > I'd appreciate a short test case for this.  I'd like to verify that
> > the previous stuff indeed didn't work and that the new stuff does.
> > 
> > Assuming that this code is run on SGI Irix, that is.
> 
> 
> I'm wondering the same thing.
> 
> > I'll try to make one.  I'm not immediately sure how.
> > 
> > It looks almost as if this code was for an unreleased port inside
> > Cygnus - anyone know if that's true?  I see active work on it in 1998
> > and 2000 in the ChangeLogs, and no mention of removing calls to it
> > - but nothing uses it at all.
> 
> 
> I'll do some digging.

Thanks.  I'm curious.

Is there a CVS repository backed up somewhere which predates 1999, when
it seems the sourceware copy was created?  Makes it a little more
challenging to do the archaelogy.

> One thing, it is very easy for a software single step function to be 
> implemented yet never tested - everything supports h/w singlestep 
> now-a-days.

Everything except my target :)

The thing is, the (trivial) software single step function wasn't even
in mips-tdep.c.  Only all the complicated support routines for it were. 
The lossage isn't all that surprising, given that these functions had
no callers.


-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] mips32_next_pc overdue cleanup

[Andrew Cagney]

> Thanks.  I'm curious.
> 
> Is there a CVS repository backed up somewhere which predates 1999, when
> it seems the sourceware copy was created?  Makes it a little more
> challenging to do the archaelogy.


There is the archive of some of the old releases that Jason Molenda put 
together:

${sources-shadow}/pub/gdb/old-releases/

and Red Hat employees do occasionally act as a remote archive search 
engine (back to ~1991).  However, prior to that there is little.


>> One thing, it is very easy for a software single step function to be 
>> implemented yet never tested - everything supports h/w singlestep 
>> now-a-days.
> 
> 
> Everything except my target :)
> 
> The thing is, the (trivial) software single step function wasn't even
> in mips-tdep.c.  Only all the complicated support routines for it were. 
> The lossage isn't all that surprising, given that these functions had
> no callers.


Perhaphs -Wunused-function should be the next -Werror :-)

	Andrew

Re: [RFA] mips32_next_pc overdue cleanup

[Stan Shebs]

Daniel Jacobowitz wrote:
> 
> Is there a CVS repository backed up somewhere which predates 1999, when
> it seems the sourceware copy was created?  Makes it a little more
> challenging to do the archaelogy.

Cygnus's CVS repository goes back to 1991, but unfortunately it's
mixed in with a bunch of stuff that is confidential to this day.
So it can never be made publicly available.

One way to make more fine-grained information available is to put
out Cygnus' old progressive releases.  Those came out every 3 months
or so, so they provide snapshots of state in between FSF releases.

There were also frequent GDB snapshots in the mid-90s, put up on
Cygnus' ftp site.  Lots of sites downloaded them, dunno how many
copies are still to be found.

Stan

Re: [RFA] mips32_next_pc overdue cleanup

[Daniel Jacobowitz]

On Fri, Jul 06, 2001 at 01:22:35AM -0400, Andrew Cagney wrote:
> > This patch is half style cleanups and half obvious fixes, but there's so
> > many I'd appreciate more eyes.  According to the masks in the opcodes
> > directory, these changes are rigt, and what we had before can't possibly
> > work.  Especially the fact that we would never, ever predict a BNE as taken
> > before these changes convinced me that they really were correct and this
> > code had never been particularly tested.  Comments?
> 
> 
> Approved.

And committed.

> History?  It pre-dates my MIPS hacking :-)  Dig dig, ah, it started out 
> as part of some prototyping that wasn't finished  It was added in '98. 
> The WinCE GDB port ended up using it so I suspect it was made to mostly 
> work.  The main reason the code worked is because it didn't bother to 
> use the broken macros that were added at the same time as the original 
> code :-/

So I noticed :)  I'm guessing that it either got a breath of life in
2000, or someone just happened to notice a mistake and was inspired. 
Well, now it'll get some checking.

Now all the major patches for the Linux port are in!  A small style
tweak and a NEWS entry, and I consider it ready to commit.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer