[RFC/RFA/sparc] problem with prologue analyzer

[RFC/RFA/sparc] problem with prologue analyzer

[Joel Brobecker]

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

Hello,

Using break.exp, we have a function marker2 defined in break1.c as
follow (sic):

        int marker2 (a) int a; { return (1); }  /* set breakpoint 9 here */

Because the entire declaration is on one single line, the function
that skips prologue can not use the line number information from
debugging data (sparc32_skip_prologue()):

  /* This is the preferred method, find the end of the prologue by
     using the debugging information.  */
  if (find_pc_partial_function (start_pc, NULL, &func_start, &func_end))
    {
      sal = find_pc_line (func_start, 0);

      if (sal.end < func_end
          && start_pc <= sal.end)
        return sal.end;
    }

So sparc32_skip_prologue() fallsback to sparc_analyze_prologue().
Unfortunately, this function recognizes the prologue instructions
only up to the "save" instruction. But the prologue of a function
can contain store instructions that home the input registers into
their stack location. This is the case of our function marker2
above:

        (gdb) disass &marker2
        Dump of assembler code for function marker2:
        0x00010aa8 <marker2+0>: save  %sp, -112, %sp
        0x00010aac <marker2+4>: st  %i0, [ %fp + 0x44 ]
        0x00010ab0 <marker2+8>: mov  1, %g1
        0x00010ab4 <marker2+12>:        mov  %g1, %i0
        0x00010ab8 <marker2+16>:        nop 
        0x00010abc <marker2+20>:        ret 
        0x00010ac0 <marker2+24>:        restore 
        End of assembler dump.

A visible consequence of this problem is that GDB will insert
a breakpoint inside marker2 one instruction too earlier, and
hence just before parameter a has been homed. And that causes
the following FAIL in the GDB testsuite:

   (gdb) PASS: gdb.base/break.exp: run until file:function(1) breakpoint
   continue
   Continuing.
   720
   
   Breakpoint 2, 0x00010aac in marker2 (a=720) at break1.c:41
   41      int marker2 (a) int a; { return (1); }  /* set breakpoint 9 here */
   (gdb) FAIL: gdb.base/break.exp: run until quoted breakpoint

The value for parameter a is incorrect, it should be 43.

This test used to pass with 5.3. Doing a bit of archeology, I discovered
that the code analyzing problogues has been heavily rewritten at the end
of 2003, and that the piece of code that handles these store insns got
lost during one large code rewrite.

Assuming this was an accident, I put the code back more or less blindly.
I did exclude the part of the code that recognizes an instruction adding
and offset to sp, as I haven't seen evidences that this is needed, and
removed one if block that could only be executed in that case. But I'd
be happy to put the entire code back, if it is felt more appropriate.

2004-11-26  Joel Brobecker  <brobecker@gnat.com>

        * sparc-tdep.c (sparc_analyze_prologue): Recognize certain store
        instructions following the save instructions as part of the
        prologue.

Tested on sparc-solaris 2.8, with GCC (based on a 3.4.x backend).
Fixes:
  . break.exp: run until quoted breakpoint
    (the case I used to study the problem)
  . funcargs.exp: print *stp

Ok to apply?

Thanks,
-- 
Joel

[-- Attachment #2: prologue.diff --]
[-- Type: text/plain, Size: 3473 bytes --]

Index: sparc-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
retrieving revision 1.157
diff -u -p -r1.157 sparc-tdep.c
--- sparc-tdep.c	23 Nov 2004 18:59:13 -0000	1.157
+++ sparc-tdep.c	26 Nov 2004 22:12:15 -0000
@@ -80,6 +80,7 @@ struct regset;
 #define X_OP2(i) (((i) >> 22) & 0x7)
 #define X_IMM22(i) ((i) & 0x3fffff)
 #define X_OP3(i) (((i) >> 19) & 0x3f)
+#define X_RS1(i) (((i) >> 14) & 0x1f)
 #define X_I(i) (((i) >> 13) & 1)
 /* Sign extension macros.  */
 #define X_DISP22(i) ((X_IMM22 (i) ^ 0x200000) - 0x200000)
@@ -575,7 +576,59 @@ sparc_analyze_prologue (CORE_ADDR pc, CO
   if (X_OP (insn) == 2 && X_OP3 (insn) == 0x3c)
     {
       cache->frameless_p = 0;
-      return pc + offset + 4;
+      offset += 4;
+
+      insn = sparc_fetch_instruction (pc + offset);
+
+      while (1)
+        {
+          /* Recognize stores into the frame from the input registers.
+             This recognizes all non alternate stores of an input register,
+             into a location offset from the frame pointer between
+             +68 and +92.  */
+
+          /* The above will fail for arguments that are promoted 
+             (eg. shorts to ints or floats to doubles), because the compiler
+             will pass them in positive-offset frame space, but the prologue
+             will save them (after conversion) in negative frame space at an
+             unpredictable offset.  Therefore I am going to remove the 
+             restriction on the target-address of the save, on the theory
+             that any unbroken sequence of saves from input registers must
+             be part of the prologue.  In un-optimized code (at least), I'm
+             fairly sure that the compiler would emit SOME other instruction
+             (eg. a move or add) before emitting another save that is actually
+             a part of the function body.
+
+             Besides, the reserved stack space is different for SPARC64 anyway.
+
+             MVS  4/23/2000  */
+
+          if (X_OP (insn) == 3
+              && (X_OP3 (insn) & 0x3c)   == 4   /* Store, non-alternate.  */
+              && (X_RD (insn) & 0x18) == 0x18   /* Input register.  */
+              && X_I (insn)                     /* Immediate mode.  */
+              && X_RS1 (insn) == 30)            /* Off of frame pointer.  */
+            ; /* empty statement -- fall thru to end of loop */
+          else if ((gdbarch_ptr_bit (current_gdbarch) == 64)
+                   && X_OP (insn) == 3
+                   && (X_OP3 (insn) & 0x3c) == 12       /* store, extended (64-bit) */
+                   && (X_RD (insn) & 0x18) == 0x18      /* input register */
+                   && X_I (insn)                        /* immediate mode */
+                   && X_RS1 (insn) == 30)               /* off of frame pointer */
+            ; /* empty statement -- fall thru to end of loop */
+          else if (X_OP (insn) == 3
+                   && (X_OP3 (insn) & 0x3c) == 36       /* store, floating-point */
+                   && X_I (insn)                        /* immediate mode */
+                   && X_RS1 (insn) == 30)               /* off of frame pointer */
+            ; /* empty statement -- fall thru to end of loop */
+          else
+            break;
+
+          offset += 4;
+          insn = sparc_fetch_instruction (pc + offset);
+        }
+
+      return pc + offset;
     }
 
   return pc;

Re: [RFC/RFA/sparc] problem with prologue analyzer

[Daniel Jacobowitz]

On Fri, Nov 26, 2004 at 02:34:10PM -0800, Joel Brobecker wrote:
> Hello,
> 
> Using break.exp, we have a function marker2 defined in break1.c as
> follow (sic):
> 
>         int marker2 (a) int a; { return (1); }  /* set breakpoint 9 here */
> 
> Because the entire declaration is on one single line, the function
> that skips prologue can not use the line number information from
> debugging data (sparc32_skip_prologue()):
> 
>   /* This is the preferred method, find the end of the prologue by
>      using the debugging information.  */
>   if (find_pc_partial_function (start_pc, NULL, &func_start, &func_end))
>     {
>       sal = find_pc_line (func_start, 0);
> 
>       if (sal.end < func_end
>           && start_pc <= sal.end)
>         return sal.end;
>     }

"Normally", when using GCC, there are two line number entries in the
debug information in this case.  Whether they've got the same line
number or not doesn't make a difference; the presence of two lets GDB
find the end of the prologue.  What does the debug information look
like?

> This test used to pass with 5.3. Doing a bit of archeology, I discovered
> that the code analyzing problogues has been heavily rewritten at the end
> of 2003, and that the piece of code that handles these store insns got
> lost during one large code rewrite.
> 
> Assuming this was an accident, I put the code back more or less blindly.
> I did exclude the part of the code that recognizes an instruction adding
> and offset to sp, as I haven't seen evidences that this is needed, and
> removed one if block that could only be executed in that case. But I'd
> be happy to put the entire code back, if it is felt more appropriate.

The rest of this I'm not qualified to review; Mark rewrote it, so maybe
he can.

-- 
Daniel Jacobowitz

Re: [RFC/RFA/sparc] problem with prologue analyzer

[Mark Kettenis]

   Date: Fri, 26 Nov 2004 14:34:10 -0800
   From: Joel Brobecker <brobecker@adacore.com>

      Breakpoint 2, 0x00010aac in marker2 (a=720) at break1.c:41
      41      int marker2 (a) int a; { return (1); }  /* set breakpoint 9 here */
      (gdb) FAIL: gdb.base/break.exp: run until quoted breakpoint

Hmm, the test PASSes for me on sparc-sun-solaris2.9 with GCC 3.2.3,
which generates essentially the same prologue code.

   This test used to pass with 5.3. Doing a bit of archeology, I discovered
   that the code analyzing problogues has been heavily rewritten at the end
   of 2003, and that the piece of code that handles these store insns got
   lost during one large code rewrite.

I left that bit of code out quite deliberately.  The idea was to
implement the minimum of prologue analysis code necessary to get the
frame unwinder working, and rely on debugging information for skipping
prologues (which the old code didn't do).  It appears that your
compiler isn't generating the proper debug info, or GDB isn't handling
that debug info properly.

Now if indeed GCC is generating bad debug info, and we accept that, it
makes sense to improve the prologue analyzer where it makes sense.  We
must realize that we probably will never get it completely right.  We
must also realize that there's a rather fundamental question here:
Should we be conservative with prologue skipping, risking the "wrong
arguments" problem you're seeing, or should we skip any code that is
likely to belong to the prologue, risking stopping after real code has
been executed.

Ultimately the "wrong arguments" case is a problem with incomplete
debug info, which might go away when GDB implements proper "multiple
location" support.  So I'm in favour of being a bit conservative here.

That said, I'm not against improving the prologue analyzer here.
However, as the comment that goes with the code that your patch buts
back in says, there are some problems with that code.  I think code
needs some polishing, splitting out the 32-bit and 64-bit cases.  I'll
try to come up with a somewhat better patch.

Mark

Re: [RFC/RFA/sparc] problem with prologue analyzer

[Mark Kettenis]

Joel, does this patch work for you?

Mark

Index: sparc-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
retrieving revision 1.157
diff -u -p -r1.157 sparc-tdep.c
--- sparc-tdep.c 23 Nov 2004 18:59:13 -0000 1.157
+++ sparc-tdep.c 29 Nov 2004 08:59:36 -0000
@@ -80,10 +80,12 @@ struct regset;
 #define X_OP2(i) (((i) >> 22) & 0x7)
 #define X_IMM22(i) ((i) & 0x3fffff)
 #define X_OP3(i) (((i) >> 19) & 0x3f)
+#define X_RS1(i) (((i) >> 14) & 0x1f)
 #define X_I(i) (((i) >> 13) & 1)
 /* Sign extension macros.  */
 #define X_DISP22(i) ((X_IMM22 (i) ^ 0x200000) - 0x200000)
 #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000)
+#define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000)
 
 /* Fetch the instruction at PC.  Instructions are always big-endian
    even if the processor operates in little-endian mode.  */
@@ -609,7 +611,36 @@ sparc32_skip_prologue (CORE_ADDR start_p
 	return sal.end;
     }
 
-  return sparc_analyze_prologue (start_pc, 0xffffffffUL, &cache);
+  start_pc = sparc_analyze_prologue (start_pc, 0xffffffffUL, &cache);
+
+  /* The psABI says that "Although the first 6 words of arguments
+     reside in registers, the standard stack frame reserves space for
+     them.".  It also suggests that a function may use that space to
+     "write incoming arguments 0 to 5" into that space, and that's
+     indeed what GCC seems to be doing.  In that case GCC will
+     generate debug information that points to the stack slots instead
+     of the registers, so we should consider the instructions that
+     write out these incoming arguments onto the stack.  Of course we
+     only need to do this if we have a stack frame.  */
+
+  while (!cache.frameless_p)
+    {
+      unsigned long insn = sparc_fetch_instruction (start_pc);
+
+      /* Recognize instructions that store incoming arguments in
+         %i0...%i5 into the corresponding stack slot.  */
+      if (X_OP (insn) == 3 && (X_OP3 (insn) & 0x3c) == 0x04 && X_I (insn)
+	  && (X_RD (insn) >= 24 && X_RD (insn) <= 29) && X_RS1 (insn) == 30
+	  && X_SIMM13 (insn) == 68 + (X_RD (insn) - 24) * 4)
+	{
+	  start_pc += 4;
+	  continue;
+	}
+
+      break;
+    }
+
+  return start_pc;
 }
 
 /* Normal frames.  */

Re: [RFC/RFA/sparc] problem with prologue analyzer

[Joel Brobecker]

Hello Mark,

> Joel, does this patch work for you?

Yes, I confirm the patch works well.

I also looked at the debugging information generated by GCC.
I found that GCC used to generate a new line number entry with
3.2.3 while it doesn't anymore with 3.4. 3.2.3 was using stabs
while 3.4 is now emitting dwarf2. This may have a relationship
with the line entry that disappeared.

Nevertheless, I think it would be very useful to commit your
patch.

Thanks!

> Index: sparc-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
> retrieving revision 1.157
> diff -u -p -r1.157 sparc-tdep.c
> --- sparc-tdep.c 23 Nov 2004 18:59:13 -0000 1.157
> +++ sparc-tdep.c 29 Nov 2004 08:59:36 -0000
> @@ -80,10 +80,12 @@ struct regset;
>  #define X_OP2(i) (((i) >> 22) & 0x7)
>  #define X_IMM22(i) ((i) & 0x3fffff)
>  #define X_OP3(i) (((i) >> 19) & 0x3f)
> +#define X_RS1(i) (((i) >> 14) & 0x1f)
>  #define X_I(i) (((i) >> 13) & 1)
>  /* Sign extension macros.  */
>  #define X_DISP22(i) ((X_IMM22 (i) ^ 0x200000) - 0x200000)
>  #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000)
> +#define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000)
>  
>  /* Fetch the instruction at PC.  Instructions are always big-endian
>     even if the processor operates in little-endian mode.  */
> @@ -609,7 +611,36 @@ sparc32_skip_prologue (CORE_ADDR start_p
>  	return sal.end;
>      }
>  
> -  return sparc_analyze_prologue (start_pc, 0xffffffffUL, &cache);
> +  start_pc = sparc_analyze_prologue (start_pc, 0xffffffffUL, &cache);
> +
> +  /* The psABI says that "Although the first 6 words of arguments
> +     reside in registers, the standard stack frame reserves space for
> +     them.".  It also suggests that a function may use that space to
> +     "write incoming arguments 0 to 5" into that space, and that's
> +     indeed what GCC seems to be doing.  In that case GCC will
> +     generate debug information that points to the stack slots instead
> +     of the registers, so we should consider the instructions that
> +     write out these incoming arguments onto the stack.  Of course we
> +     only need to do this if we have a stack frame.  */
> +
> +  while (!cache.frameless_p)
> +    {
> +      unsigned long insn = sparc_fetch_instruction (start_pc);
> +
> +      /* Recognize instructions that store incoming arguments in
> +         %i0...%i5 into the corresponding stack slot.  */
> +      if (X_OP (insn) == 3 && (X_OP3 (insn) & 0x3c) == 0x04 && X_I (insn)
> +	  && (X_RD (insn) >= 24 && X_RD (insn) <= 29) && X_RS1 (insn) == 30
> +	  && X_SIMM13 (insn) == 68 + (X_RD (insn) - 24) * 4)
> +	{
> +	  start_pc += 4;
> +	  continue;
> +	}
> +
> +      break;
> +    }
> +
> +  return start_pc;
>  }
>  
>  /* Normal frames.  */

-- 
Joel

Re: [RFC/RFA/sparc] problem with prologue analyzer

[Mark Kettenis]

   Date: Mon, 29 Nov 2004 11:17:13 -0800
   From: Joel Brobecker <brobecker@adacore.com>

   Hello Mark,

   > Joel, does this patch work for you?

   Yes, I confirm the patch works well.

Thanks,

Here's the complete patch with ChangeLog that I checked in.

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>
 
	* sparc-tdep.c (X_RS1, X_SIMM13): New macros.
	(sparc32_skip_prologue): Skip instructions that store arguments in
	registers into their corresponding stack slots.

Index: sparc-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
retrieving revision 1.157
diff -u -p -r1.157 sparc-tdep.c
--- sparc-tdep.c 23 Nov 2004 18:59:13 -0000 1.157
+++ sparc-tdep.c 29 Nov 2004 19:48:47 -0000
@@ -80,10 +80,12 @@ struct regset;
 #define X_OP2(i) (((i) >> 22) & 0x7)
 #define X_IMM22(i) ((i) & 0x3fffff)
 #define X_OP3(i) (((i) >> 19) & 0x3f)
+#define X_RS1(i) (((i) >> 14) & 0x1f)
 #define X_I(i) (((i) >> 13) & 1)
 /* Sign extension macros.  */
 #define X_DISP22(i) ((X_IMM22 (i) ^ 0x200000) - 0x200000)
 #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000)
+#define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000)
 
 /* Fetch the instruction at PC.  Instructions are always big-endian
    even if the processor operates in little-endian mode.  */
@@ -609,7 +611,36 @@ sparc32_skip_prologue (CORE_ADDR start_p
 	return sal.end;
     }
 
-  return sparc_analyze_prologue (start_pc, 0xffffffffUL, &cache);
+  start_pc = sparc_analyze_prologue (start_pc, 0xffffffffUL, &cache);
+
+  /* The psABI says that "Although the first 6 words of arguments
+     reside in registers, the standard stack frame reserves space for
+     them.".  It also suggests that a function may use that space to
+     "write incoming arguments 0 to 5" into that space, and that's
+     indeed what GCC seems to be doing.  In that case GCC will
+     generate debug information that points to the stack slots instead
+     of the registers, so we should consider the instructions that
+     write out these incoming arguments onto the stack.  Of course we
+     only need to do this if we have a stack frame.  */
+
+  while (!cache.frameless_p)
+    {
+      unsigned long insn = sparc_fetch_instruction (start_pc);
+
+      /* Recognize instructions that store incoming arguments in
+         %i0...%i5 into the corresponding stack slot.  */
+      if (X_OP (insn) == 3 && (X_OP3 (insn) & 0x3c) == 0x04 && X_I (insn)
+	  && (X_RD (insn) >= 24 && X_RD (insn) <= 29) && X_RS1 (insn) == 30
+	  && X_SIMM13 (insn) == 68 + (X_RD (insn) - 24) * 4)
+	{
+	  start_pc += 4;
+	  continue;
+	}
+
+      break;
+    }
+
+  return start_pc;
 }
 
 /* Normal frames.  */