[RFA/alpha] Add handling of FP control insn in software-single step

[RFA/alpha] Add handling of FP control insn in software-single step

[Joel Brobecker]

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

Hello,

We noticed that alpha-tdep.c:alpha_next_pc() did have any support for
FP control (branch) instructions. So single-steping over instructions
such as fbeq was not always working properly since the target address
was not always following the potential jump.

The attached patch should fix this.

2005-05-17  Joel Brobecker  <brobecker@adacore.com>

        * alpha-tdep.c (fp_register_zero_p): New function.
        (fp_register_sign_bit): New function.
        (alpha_next_pc): Add support for floating-point control instructions.

Tested on alpha-tru64 5.1a with no regression. It fixes a problem with
some code but I can't contribute it, as it has been given to us by a
customer.

OK to apply?

Thanks,
-- 
Joel

[-- Attachment #2: alpha-tdep.c.diff --]
[-- Type: text/plain, Size: 2727 bytes --]

Index: alpha-tdep.c
===================================================================
RCS file: /nile.c/cvs/Dev/gdb/gdb-6.3/gdb/alpha-tdep.c,v
retrieving revision 1.3
diff -u -p -r1.3 alpha-tdep.c
--- alpha-tdep.c	1 Dec 2004 04:07:53 -0000	1.3
+++ alpha-tdep.c	17 May 2005 00:26:57 -0000
@@ -1357,6 +1357,26 @@ alpha_fill_fp_regs (int regno, void *f0_
 }
 
 \f
+
+/* Return nonzero if the given buffer holds a G_floating register
+   value of zero.  */
+   
+static int
+fp_register_zero_p (char *buf)
+{
+  return ((buf[1] & 0x0f) == 0 && buf[2] == 0 && buf[3] == 0
+          && buf[4] == 0 && buf[5] == 0 && buf[6] == 0 && buf[7] == 0);
+}
+
+/* Return the value of the sign bit for the G_floating register
+   value held in BUF.  */
+
+static int
+fp_register_sign_bit (char *buf)
+{
+  return (buf[0] & 0x80);
+}
+
 /* alpha_software_single_step() is called just before we want to resume
    the inferior, if we want to single-step it but there is no hardware
    or kernel single-step support (NetBSD on Alpha, for example).  We find
@@ -1372,6 +1392,7 @@ alpha_next_pc (CORE_ADDR pc)
   unsigned int op;
   int offset;
   LONGEST rav;
+  char reg[8];
 
   insn = alpha_read_insn (pc);
 
@@ -1400,8 +1421,9 @@ alpha_next_pc (CORE_ADDR pc)
 	  return (pc + 4 + offset);
 	}
 
+      regcache_cooked_read (current_regcache, (insn >> 21) & 0x1f, reg);
       /* Need to determine if branch is taken; read RA.  */
-      rav = (LONGEST) read_register ((insn >> 21) & 0x1f);
+      rav = extract_signed_integer (reg, 4);
       switch (op)
 	{
 	case 0x38:		/* BLBC */
@@ -1437,7 +1459,32 @@ alpha_next_pc (CORE_ADDR pc)
 	    goto branch_taken;
 	  break;
 
-	/* ??? Missing floating-point branches.  */
+        /* Floating point branches.  */
+        
+        case 0x31:              /* FBEQ */
+          if (fp_register_zero_p (reg))
+            goto branch_taken;
+          break;
+        case 0x36:              /* FBGE */
+          if (fp_register_sign_bit (reg) == 0 || fp_register_zero_p (reg))
+            goto branch_taken;
+          break;
+        case 0x37:              /* FBGT */
+          if (fp_register_sign_bit (reg) == 0 && ! fp_register_zero_p (reg))
+            goto branch_taken;
+          break;
+        case 0x33:              /* FBLE */
+          if (fp_register_sign_bit (reg) == 1 || fp_register_zero_p (reg))
+            goto branch_taken;
+          break;
+        case 0x32:              /* FBLT */
+          if (fp_register_sign_bit (reg) == 1 && ! fp_register_zero_p (reg))
+            goto branch_taken;
+          break;
+        case 0x35:              /* FBNE */
+          if (! fp_register_zero_p (reg))
+            goto branch_taken;
+          break;
 	}
     }
 

Re: [RFA/alpha] Add handling of FP control insn in software-single step

[Daniel Jacobowitz]

On Tue, May 17, 2005 at 12:34:00PM +1000, Joel Brobecker wrote:
> Tested on alpha-tru64 5.1a with no regression. It fixes a problem with
> some code but I can't contribute it, as it has been given to us by a
> customer.

Can you write a test for this in gdb.arch, using an asm to provide the
fbeq instruction that was causing a problem?


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA/alpha] Add handling of FP control insn in software-single step

[Joel Brobecker]

> On Tue, May 17, 2005 at 12:34:00PM +1000, Joel Brobecker wrote:
> > Tested on alpha-tru64 5.1a with no regression. It fixes a problem with
> > some code but I can't contribute it, as it has been given to us by a
> > customer.
> 
> Can you write a test for this in gdb.arch, using an asm to provide the
> fbeq instruction that was causing a problem?

Let me see what I can do. I tend to prefer tests written in C or Ada,
as it can be used on all platforms rather than just on alpha. But
perhaps the usage in this case is to prefer asm?

-- 
Joel

Re: [RFA/alpha] Add handling of FP control insn in software-single step

[Daniel Jacobowitz]

On Tue, May 17, 2005 at 12:50:48PM +1000, Joel Brobecker wrote:
> > On Tue, May 17, 2005 at 12:34:00PM +1000, Joel Brobecker wrote:
> > > Tested on alpha-tru64 5.1a with no regression. It fixes a problem with
> > > some code but I can't contribute it, as it has been given to us by a
> > > customer.
> > 
> > Can you write a test for this in gdb.arch, using an asm to provide the
> > fbeq instruction that was causing a problem?
> 
> Let me see what I can do. I tend to prefer tests written in C or Ada,
> as it can be used on all platforms rather than just on alpha. But
> perhaps the usage in this case is to prefer asm?

The feature you're implementing is specific to Alpha; think of it in
the nature of unit testing.

That's not to say a test for "single step over a floating point
compare-and-branch" would be a bad thing to have in the rest of the
testsuite.  But I'm sure you can imagine how hard it is to predict what
a compiler will generate.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFA/alpha] Add handling of FP control insn in software-single step

[Joel Brobecker]

> The feature you're implementing is specific to Alpha; think of it in
> the nature of unit testing.

That makes sense. Do you know if it is ok to import an assembly file
generated by GCC? Or do I have to embed the assembly into a C file?

-- 
Joel

Re: [RFA/alpha] Add handling of FP control insn in software-single step

[Mark Kettenis]

   Date: Tue, 17 May 2005 14:48:57 +1000
   From: Joel Brobecker <brobecker@adacore.com>

   > The feature you're implementing is specific to Alpha; think of it in
   > the nature of unit testing.

   That makes sense. Do you know if it is ok to import an assembly file
   generated by GCC? Or do I have to embed the assembly into a C file?

The latter is probably a bit easier, but I guess the former is ok too
if you can be reasonably sure that the .s file can be assembled with
gas too.

Take a look at the i386 files in gdb.arch/ for an example.

Mark

Re: [RFA/alpha] Add handling of FP control insn in software-single step

[Richard Henderson]

On Tue, May 17, 2005 at 12:34:00PM +1000, Joel Brobecker wrote:
> +fp_register_zero_p (char *buf)
> +{
> +  return ((buf[1] & 0x0f) == 0 && buf[2] == 0 && buf[3] == 0
> +          && buf[4] == 0 && buf[5] == 0 && buf[6] == 0 && buf[7] == 0);
> +}
...
> +        case 0x31:              /* FBEQ */
> +          if (fp_register_zero_p (reg))
> +            goto branch_taken;
> +          break;
> +        case 0x36:              /* FBGE */
> +          if (fp_register_sign_bit (reg) == 0 || fp_register_zero_p (reg))
> +            goto branch_taken;
> +          break;
> +        case 0x37:              /* FBGT */
> +          if (fp_register_sign_bit (reg) == 0 && ! fp_register_zero_p (reg))
> +            goto branch_taken;
> +          break;
> +        case 0x33:              /* FBLE */
> +          if (fp_register_sign_bit (reg) == 1 || fp_register_zero_p (reg))
> +            goto branch_taken;
> +          break;
> +        case 0x32:              /* FBLT */
> +          if (fp_register_sign_bit (reg) == 1 && ! fp_register_zero_p (reg))
> +            goto branch_taken;
> +          break;
> +        case 0x35:              /* FBNE */
> +          if (! fp_register_zero_p (reg))
> +            goto branch_taken;
> +          break;

NACK.  Why are you ignoring the entire exponent?  These are not the
operations that fp branches perform.  The tests are:

  zero		(reg & 0x7fff_ffff_ffff_ffff) == 0
  sign		(reg & 0x8000_0000_0000_0000) != 0

  fbeq		zero
  fbne		!zero
  fbge		!sign || zero
  fbgt		!sign && !zero
  fble		sign || zero
  fblt		sign && !zero

This is a nearly normal integer test, except for the special case
of 0x8000_0000_0000_0000, i.e. -0.0, which is treated as zero.


r~

Re: [RFA/alpha] Add handling of FP control insn in software-single step

[Richard Henderson]

On Tue, May 17, 2005 at 12:34:00PM +1000, Joel Brobecker wrote:
> +static int
> +fp_register_sign_bit (char *buf)
> +{
> +  return (buf[0] & 0x80);

Also, this is a big-endian test.  

> +      regcache_cooked_read (current_regcache, (insn >> 21) & 0x1f, reg);
>        /* Need to determine if branch is taken; read RA.  */
> -      rav = (LONGEST) read_register ((insn >> 21) & 0x1f);
> +      rav = extract_signed_integer (reg, 4);

And you're only extracting 4 bytes instead of 8.

So, basically, not one part of the patch is correct.


r~

Re: [RFA/alpha] Add handling of FP control insn in software-single step

[Joel Brobecker]

> And you're only extracting 4 bytes instead of 8.
> 
> So, basically, not one part of the patch is correct.

Arg. Now I'm ashamed.  Patch withdrawn for now, will post another one
with the fixes you suggest.

Thanks for the review,
-- 
Joel