[RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Joel Brobecker]

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

Hello,

We have noticed that GDB skips too much prologue when inserting
a breakpoint on a given function. I will provide the source code
below, but it's not very important to understand the cause of the
problem. Here is an extract:

     12       procedure print is
     13          A : Integer;
     14       begin
     15          A := I;
     16          null;
     17       end print;

Right now, when we insert a breakpoint on function ``print''
(which encoded name is
``generics__test_generics__other_instance__print.0''), we end
up placing the breakpoint at line 17, but the expected location
was at line 15:

    (gdb) b "generics__test_generics__other_instance__print.0"
    Breakpoint 1 at 0x100112e0: file generics.adb, line 17.

A quick look at the assembly code for function ``print'' shows
what causes skip_prologue() to go too far:

    
    Line 12:
        <print.0+0>:        stw     r31,-4(r1)
        <print.0+4>:        stwu    r1,-48(r1)
        <print.0+8>:        mr      r31,r1
        <print.0+12>:       stw     r11,24(r31)
    Line 15:
        <print.0+16>:       li      r0,12
        <print.0+20>:       stw     r0,28(r31)
    Line 17:
        <print.0+24>:       lwz     r1,0(r1)
        <print.0+28>:       lwz     r31,-4(r1)
        <print.0+32>:       blr

The first problem is with the insn at +16 (li [...]). If part of the
prologue, this instruction should be part of a pair of instructions
saving vector registers. This is not the case here. However,
skip_prologue() doesn't check that, and instead considers by default
that this insn is part of the prologue (ie "prev_insn_was_prologue_insn"
remains set to 1). So the first change I made was to *not* consider this
instruction as part of the prologue unless the next one is part of the
prologue. Normally, you would rather check that the next instruction is
even one of the instructions that come as a pair with that "li"
instruction. But this is not necessary in our case, because we're not
trying to tag each instruction individually, we're trying to find the
upper bound of the prologue. So long as some later instruction is
recognized as part of the prologue (whether it be part of that
instruction pair or not), we need to update our upper bound accordingly.
On the contrary, if none of the following instructions we scan belong
to the prologue, then we should not include that instruction in the
function prologue. Hence my first change (I hope I was clear enough?).

The second problem is with the following instruction: "stw r0,28(r31)".
The prologue analyzer tags this instruction as being part of the
prologue, but this is incorrect, I believe. According to the PPC
ABI document I have, only r3 to r10 are used for parameter passing:

        For PowerPC, up to eight words are passed in general purpose
        registers, loaded sequentially into general purpose registers r3
        through r10.

Unfortunately, skip_prologue() detects the "stw Rx, NUM(R31)" sequence,
but forgot to check the register number. Here, it's the scratch register
zero, so the instruction should not be considered part of the prologue
either.

The same applies to instructions involving saving floating point
arguments passed via registers: Only f2 to f8 are used for that
purpose.

So my second change was to create a new function extracted from
skip_prologue() named store_parameter_on_stack_prologue_insn_p(),
and then improve it to also check for register numbers.

2004-04-02  Joel Brobecker  <brobecker@gnat.com>

        * rs6000-tdep.c (store_parameter_on_stack_prologue_insn_p):
        New function, an improved version of some code extracted from
        skip_prologue().
        (skip_prologue): Use store_parameter_on_stack_prologue_insn_p()
        to detect instructions saving a parameter on the stack.
        Do not mark "li rx, SIMM" instructions as part of the prologue,
        unless the following instruction is also part of the prologue.

Tested on ppc-aix-5.1. This fixes several FAILs and XFAILs as well as
our problem:

In gdb.base:
    condbreak.exp: run until breakpoint at marker1              XFAIL PASS
    condbreak.exp: run until breakpoint at marker2              XFAIL PASS
    ena-dis-br.exp: continue to auto-disabled break marker2     XFAIL PASS
    ena-dis-br.exp: continue to break marker1                   XFAIL PASS
    ena-dis-br.exp: continue to break marker1, 2nd time         XFAIL PASS
    funcargs.exp: continue to call5b                            FAIL  PASS
    funcargs.exp: print *stp                                    FAIL  PASS
    funcargs.exp: print st                                      FAIL  PASS
    funcargs.exp: print un                                      FAIL  PASS
    funcargs.exp: run to call5a                                 FAIL  PASS

I also get some improved results in gdb.threads, and it seems related to
breakpointing, but I am always cautious with varying results in that
part of the testsuite, and I haven't had time to study them.

OK to apply?

Thanks,
-- 
Joel

Here is the entire Ada code we used to reproduce the problem. To build
them, place the sources in a file, say sources.ada, and then do:

        % gnatchop sources.ada
        % gnatmake -g generics_main

<<
package body Generics is

   procedure Test_Generics is
      package other_instance is new the_generic(12);
   begin
      other_instance.print;
      null;
   end Test_Generics;

   package body the_generic is

      procedure print is
         A : Integer;
      begin
         A := I;
         null;
      end print;
   end the_generic;

end Generics;
package Generics is

   generic
      i : integer;
   package the_generic is
      procedure print;
   end the_generic;

   procedure Test_Generics;

end Generics;
with generics;
procedure generics_main is
begin
    generics.test_generics;
end generics_main;
>>

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

Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.191
diff -u -p -r1.191 rs6000-tdep.c
--- rs6000-tdep.c	1 Apr 2004 21:00:59 -0000	1.191
+++ rs6000-tdep.c	1 Apr 2004 23:03:50 -0000
@@ -420,6 +420,69 @@ refine_prologue_limit (CORE_ADDR pc, COR
 }
 
 
+/* Return nonzero if the given instruction OP can be part of the prologue
+   of a function that saves a parameter on the stack.  FRAMEP should be
+   set if one of the previous instructions in the function has set the
+   Frame Pointer.  */
+
+static int
+store_parameter_on_stack_prologue_insn_p (unsigned long op, int framep)
+{
+  /* Move parameters from argument registers to temporary register.  */
+  if ((op & 0xfc0007fe) == 0x7c000378)         /* mr(.)  Rx,Ry */
+    {
+      /* Rx must be scratch register r0.  */
+      const int rx_regno = (op >> 16) & 31;
+      /* Ry: Only r3 - r10 are used for parameter passing.  */
+      const int ry_regno = GET_SRC_REG (op);
+
+      return (rx_regno == 0 && ry_regno >= 3 && ry_regno <= 10);
+    }
+
+  /* Save a General Purpose Register on stack.  */
+
+  if ((op & 0xfc1f0003) == 0xf8010000 ||       /* std  Rx,NUM(r1) */
+      (op & 0xfc1f0000) == 0xd8010000)         /* stfd Rx,NUM(r1) */
+    {
+      /* Rx: Only r3 - r10 are used for parameter passing.  */
+      const int rx_regno = GET_SRC_REG (op);
+
+      return (rx_regno >= 3 && rx_regno <= 10);
+    }
+           
+  /* Save a General Purpose Register on stack via the Frame Pointer.  */
+
+  if (framep &&
+      ((op & 0xfc1f0000) == 0x901f0000 ||     /* st rx,NUM(r31) */
+       (op & 0xfc1f0000) == 0x981f0000 ||     /* stb Rx,NUM(r31) */
+       (op & 0xfc1f0000) == 0xd81f0000))      /* stfd Rx,NUM(r31) */
+    {
+      /* Rx: Only r3 - r10 are used for parameter passing.  */
+      const int rx_regno = GET_SRC_REG (op);
+
+      return (rx_regno >= 3 && rx_regno <= 10);
+    }
+
+  if ((op & 0xfc1f0000) == 0xfc010000)         /* frsp, fp?,NUM(r1) */
+    {
+      /* Only f2 - f8 are used for parameter passing.  */
+      const int src_regno = GET_SRC_REG (op);
+
+      return (src_regno >= 2 && src_regno <= 8);
+    }
+
+  if (framep && ((op & 0xfc1f0000) == 0xfc1f0000))  /* frsp, fp?,NUM(r31) */
+    {
+      /* Only f2 - f8 are used for parameter passing.  */
+      const int src_regno = GET_SRC_REG (op);
+
+      return (src_regno >= 2 && src_regno <= 8);
+    }
+
+  /* Not an insn that saves a parameter on stack.  */
+  return 0;
+}
+
 static CORE_ADDR
 skip_prologue (CORE_ADDR pc, CORE_ADDR lim_pc, struct rs6000_framedata *fdata)
 {
@@ -701,26 +764,7 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
 	  /* store parameters in stack */
 	}
       /* Move parameters from argument registers to temporary register.  */
-      else if ((op & 0xfc0007fe) == 0x7c000378 &&	/* mr(.)  Rx,Ry */
-               (((op >> 21) & 31) >= 3) &&              /* R3 >= Ry >= R10 */
-               (((op >> 21) & 31) <= 10) &&
-               (((op >> 16) & 31) == 0)) /* Rx: scratch register r0 */
-        {
-          continue;
-        }
-      else if ((op & 0xfc1f0003) == 0xf8010000 ||	/* std rx,NUM(r1) */
-	       (op & 0xfc1f0000) == 0xd8010000 ||	/* stfd Rx,NUM(r1) */
-	       (op & 0xfc1f0000) == 0xfc010000)		/* frsp, fp?,NUM(r1) */
-	{
-	  continue;
-
-	  /* store parameters in stack via frame pointer */
-	}
-      else if (framep &&
-	       ((op & 0xfc1f0000) == 0x901f0000 ||     /* st rx,NUM(r31) */
-                (op & 0xfc1f0000) == 0x981f0000 ||     /* stb Rx,NUM(r31) */
-		(op & 0xfc1f0000) == 0xd81f0000 ||     /* stfd Rx,NUM(r31) */
-		(op & 0xfc1f0000) == 0xfc1f0000))      /* frsp, fp?,NUM(r31) */
+      else if (store_parameter_on_stack_prologue_insn_p (op, framep))
         {
 	  continue;
 
@@ -791,6 +835,11 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
 	{
 	  li_found_pc = pc;
 	  vr_saved_offset = SIGNED_SHORT (op);
+
+          /* This insn by itself is not part of the prologue, unless
+             if part of the pair of insns mentioned above. So do not
+             record this insn as part of the prologue yet.  */
+          prev_insn_was_prologue_insn = 0;
 	}
       /* Store vector register S at (r31+r0) aligned to 16 bytes.  */      
       /* 011111 sssss 11111 00000 00111001110 */

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Jim Blandy]

Joel Brobecker <brobecker@gnat.com> writes:
> We have noticed that GDB skips too much prologue when inserting
> a breakpoint on a given function. I will provide the source code
> below, but it's not very important to understand the cause of the
> problem. Here is an extract:
> 
>      12       procedure print is
>      13          A : Integer;
>      14       begin
>      15          A := I;
>      16          null;
>      17       end print;
> 
> Right now, when we insert a breakpoint on function ``print''
> (which encoded name is
> ``generics__test_generics__other_instance__print.0''), we end
> up placing the breakpoint at line 17, but the expected location
> was at line 15:
> 
>     (gdb) b "generics__test_generics__other_instance__print.0"
>     Breakpoint 1 at 0x100112e0: file generics.adb, line 17.
> 
> A quick look at the assembly code for function ``print'' shows
> what causes skip_prologue() to go too far:
> 
>     
>     Line 12:
>         <print.0+0>:        stw     r31,-4(r1)
>         <print.0+4>:        stwu    r1,-48(r1)
>         <print.0+8>:        mr      r31,r1
>         <print.0+12>:       stw     r11,24(r31)
>     Line 15:
>         <print.0+16>:       li      r0,12
>         <print.0+20>:       stw     r0,28(r31)
>     Line 17:
>         <print.0+24>:       lwz     r1,0(r1)
>         <print.0+28>:       lwz     r31,-4(r1)
>         <print.0+32>:       blr
> 
> The first problem is with the insn at +16 (li [...]). If part of the
> prologue, this instruction should be part of a pair of instructions
> saving vector registers. This is not the case here. However,
> skip_prologue() doesn't check that, and instead considers by default
> that this insn is part of the prologue (ie "prev_insn_was_prologue_insn"
> remains set to 1). So the first change I made was to *not* consider this
> instruction as part of the prologue unless the next one is part of the
> prologue. Normally, you would rather check that the next instruction is
> even one of the instructions that come as a pair with that "li"
> instruction. But this is not necessary in our case, because we're not
> trying to tag each instruction individually, we're trying to find the
> upper bound of the prologue. So long as some later instruction is
> recognized as part of the prologue (whether it be part of that
> instruction pair or not), we need to update our upper bound accordingly.
> On the contrary, if none of the following instructions we scan belong
> to the prologue, then we should not include that instruction in the
> function prologue. Hence my first change (I hope I was clear enough?).
> 
> The second problem is with the following instruction: "stw r0,28(r31)".
> The prologue analyzer tags this instruction as being part of the
> prologue, but this is incorrect, I believe. According to the PPC
> ABI document I have, only r3 to r10 are used for parameter passing:
> 
>         For PowerPC, up to eight words are passed in general purpose
>         registers, loaded sequentially into general purpose registers r3
>         through r10.
> 
> Unfortunately, skip_prologue() detects the "stw Rx, NUM(R31)" sequence,
> but forgot to check the register number. Here, it's the scratch register
> zero, so the instruction should not be considered part of the prologue
> either.

Here's a prologue I saw recently where an 'stX r0, NUM(r31)' really is
part of the prologue:

            .align 2
            .globl arg_passing_test2
            .type	arg_passing_test2, @function
    arg_passing_test2:
    .LFB107:
            .loc 1 62 0
            stwu 1,-64(1)
    .LCFI11:
            stw 31,60(1)
    .LCFI12:
            mr 31,1
    .LCFI13:
            mr 0,3
            evstdd 4,16(31)
            stw 5,24(31)
            stw 7,32(31)
            stw 8,36(31)
            stw 9,40(31)
            stb 0,8(31)
            lwz 11,0(1)
            lwz 31,-4(11)
            mr 1,11
            blr
    .LFE107:
            .size	arg_passing_test2, .-arg_passing_test2

In this case, the stX 0,N(31) is spilling an argument, even though r0
is not an argument register.  ('evstdd' is an E500 instruction that
is definitely an argument spill.)

Clearly, both your function and mine need to go into the test suite...

What if we did something like this?  It'd need to be combined with the 
rest of your change, I'm just sketching:

*** rs6000-tdep.c.~1.191.~	2004-03-29 16:45:15.000000000 -0500
--- rs6000-tdep.c	2004-04-02 16:11:26.000000000 -0500
***************
*** 441,446 ****
--- 441,447 ----
    int minimal_toc_loaded = 0;
    int prev_insn_was_prologue_insn = 1;
    int num_skip_non_prologue_insns = 0;
+   int r0_contains_argument = 0;
    const struct bfd_arch_info *arch_info = gdbarch_bfd_arch_info (current_gdbarch);
    struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
    
***************
*** 509,519 ****
--- 510,524 ----
  	     ones.  */
  	  if (lr_reg < 0)
  	    lr_reg = (op & 0x03e00000);
+           if (lr_reg == 0)
+             r0_contains_argument = 0;
  	  continue;
  	}
        else if ((op & 0xfc1fffff) == 0x7c000026)
  	{			/* mfcr Rx */
  	  cr_reg = (op & 0x03e00000);
+           if (cr_reg == 0)
+             r0_contains_argument = 0;
  	  continue;
  
  	}
***************
*** 560,565 ****
--- 565,571 ----
  				   for >= 32k frames */
  	  fdata->offset = (op & 0x0000ffff) << 16;
  	  fdata->frameless = 0;
+           r0_contains_argument = 0;
  	  continue;
  
  	}
***************
*** 568,573 ****
--- 574,580 ----
  				   lf of >= 32k frames */
  	  fdata->offset |= (op & 0x0000ffff);
  	  fdata->frameless = 0;
+           r0_contains_argument = 0;
  	  continue;
  
  	}
***************
*** 706,711 ****
--- 713,719 ----
                 (((op >> 21) & 31) <= 10) &&
                 (((op >> 16) & 31) == 0)) /* Rx: scratch register r0 */
          {
+           r0_contains_argument = 1;
            continue;
          }
        else if ((op & 0xfc1f0003) == 0xf8010000 ||	/* std rx,NUM(r1) */
***************
*** 717,725 ****
  	  /* store parameters in stack via frame pointer */
  	}
        else if (framep &&
! 	       ((op & 0xfc1f0000) == 0x901f0000 ||     /* st rx,NUM(r31) */
!                 (op & 0xfc1f0000) == 0x981f0000 ||     /* stb Rx,NUM(r31) */
! 		(op & 0xfc1f0000) == 0xd81f0000 ||     /* stfd Rx,NUM(r31) */
  		(op & 0xfc1f0000) == 0xfc1f0000))      /* frsp, fp?,NUM(r31) */
          {
  	  continue;
--- 725,737 ----
  	  /* store parameters in stack via frame pointer */
  	}
        else if (framep &&
! 	       ((((op & 0xfc1f0000) == 0x901f0000 ||   /* st rx,NUM(r31) */
!                   (op & 0xfc1f0000) == 0x981f0000 ||   /* stb Rx,NUM(r31) */
!                   (op & 0xfc1f0000) == 0xd81f0000) &&  /* stfd Rx,NUM(r31) */
!                  ((((op & 0x03e00000) >> 21) >= 3 &&   /* Rx is arg reg */
!                    ((op & 0x03e00000) >> 21) <= 10) || /* (r3..r10) */
!                   ((op & 0x03e00000) == 0              /* Rx is r0 */
!                    && r0_contains_argument))) ||       /* and r0 holds arg */
  		(op & 0xfc1f0000) == 0xfc1f0000))      /* frsp, fp?,NUM(r31) */
          {
  	  continue;
***************
*** 789,794 ****
--- 801,808 ----
        else if ((op & 0xffff0000) == 0x38000000         /* li r0, SIMM */
                 || (op & 0xffff0000) == 0x39c00000)     /* li r14, SIMM */
  	{
+           if ((op & 0xffff0000) == 0x38000000)
+             r0_contains_argument = 0;
  	  li_found_pc = pc;
  	  vr_saved_offset = SIGNED_SHORT (op);
  	}

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Kevin Buettner]

On 02 Apr 2004 16:14:16 -0500
Jim Blandy <jimb@redhat.com> wrote:

> Joel Brobecker <brobecker@gnat.com> writes:
>
> > The second problem is with the following instruction: "stw r0,28(r31)".
> > The prologue analyzer tags this instruction as being part of the
> > prologue, but this is incorrect, I believe. According to the PPC
> > ABI document I have, only r3 to r10 are used for parameter passing:
> > 
> >         For PowerPC, up to eight words are passed in general purpose
> >         registers, loaded sequentially into general purpose registers r3
> >         through r10.
> > 
> > Unfortunately, skip_prologue() detects the "stw Rx, NUM(R31)" sequence,
> > but forgot to check the register number. Here, it's the scratch register
> > zero, so the instruction should not be considered part of the prologue
> > either.
> 
> Here's a prologue I saw recently where an 'stX r0, NUM(r31)' really is
> part of the prologue:
> 
>             .align 2
>             .globl arg_passing_test2
>             .type	arg_passing_test2, @function
>     arg_passing_test2:
>     .LFB107:
>             .loc 1 62 0
>             stwu 1,-64(1)
>     .LCFI11:
>             stw 31,60(1)
>     .LCFI12:
>             mr 31,1
>     .LCFI13:
>             mr 0,3
>             evstdd 4,16(31)
>             stw 5,24(31)
>             stw 7,32(31)
>             stw 8,36(31)
>             stw 9,40(31)
>             stb 0,8(31)
>             lwz 11,0(1)
>             lwz 31,-4(11)
>             mr 1,11
>             blr
>     .LFE107:
>             .size	arg_passing_test2, .-arg_passing_test2
> 
> In this case, the stX 0,N(31) is spilling an argument, even though r0
> is not an argument register.  ('evstdd' is an E500 instruction that
> is definitely an argument spill.)

Do you have any idea why the compiler chose to arrange the code this way?
Why couldn't it have just done "stb 3, 8(31)" and ommitted the "mr 0, 3"
altogether?

> Clearly, both your function and mine need to go into the test suite...
> 
> What if we did something like this?  It'd need to be combined with the 
> rest of your change, I'm just sketching:
> 
> *** rs6000-tdep.c.~1.191.~	2004-03-29 16:45:15.000000000 -0500
> --- rs6000-tdep.c	2004-04-02 16:11:26.000000000 -0500
> ***************
> *** 441,446 ****
> --- 441,447 ----
>     int minimal_toc_loaded = 0;
>     int prev_insn_was_prologue_insn = 1;
>     int num_skip_non_prologue_insns = 0;
> +   int r0_contains_argument = 0;
>     const struct bfd_arch_info *arch_info = gdbarch_bfd_arch_info (current_gdbarch);
>     struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
>     
> ***************
> *** 509,519 ****
> --- 510,524 ----
>   	     ones.  */
>   	  if (lr_reg < 0)
>   	    lr_reg = (op & 0x03e00000);
> +           if (lr_reg == 0)
> +             r0_contains_argument = 0;

This special casing of r0 bothers me.  In your example above, what's to
prevent the compiler from using some other scratch register, e.g. r11 or
r12?  If it starts doing so, do we add more state variables to record this
fact?

Kevin

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Jim Blandy]

Kevin Buettner <kevinb@redhat.com> writes:
> > In this case, the stX 0,N(31) is spilling an argument, even though r0
> > is not an argument register.  ('evstdd' is an E500 instruction that
> > is definitely an argument spill.)
> 
> Do you have any idea why the compiler chose to arrange the code this way?
> Why couldn't it have just done "stb 3, 8(31)" and ommitted the "mr 0, 3"
> altogether?

I have no idea.  My attitude has always been that we just have to put
up with whatever junk we get.  GDB, by its nature, just has to cope
with whatever hair the upstream implementors come up with.

> > Clearly, both your function and mine need to go into the test suite...
> > 
> > What if we did something like this?  It'd need to be combined with the 
> > rest of your change, I'm just sketching:
> > 
> > *** rs6000-tdep.c.~1.191.~	2004-03-29 16:45:15.000000000 -0500
> > --- rs6000-tdep.c	2004-04-02 16:11:26.000000000 -0500
> > ***************
> > *** 441,446 ****
> > --- 441,447 ----
> >     int minimal_toc_loaded = 0;
> >     int prev_insn_was_prologue_insn = 1;
> >     int num_skip_non_prologue_insns = 0;
> > +   int r0_contains_argument = 0;
> >     const struct bfd_arch_info *arch_info = gdbarch_bfd_arch_info (current_gdbarch);
> >     struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
> >     
> > ***************
> > *** 509,519 ****
> > --- 510,524 ----
> >   	     ones.  */
> >   	  if (lr_reg < 0)
> >   	    lr_reg = (op & 0x03e00000);
> > +           if (lr_reg == 0)
> > +             r0_contains_argument = 0;
> 
> This special casing of r0 bothers me.  In your example above, what's to
> prevent the compiler from using some other scratch register, e.g. r11 or
> r12?  If it starts doing so, do we add more state variables to record this
> fact?

I guess so.

For what it's worth --- the analysis used in s390-tdep.c makes all
these problems just go away; it is able to recognize argument spills,
register saves, etc. done in almost any reasonable fashion.  Internal
to Red Hat, I've gotten the architecture-independent portions split
out; the arch-dependent code that's left over is pretty much just an
interpreter for prologue instructions.  In spare moments I've been
trying to put together enough platforms for PPC testing (Altivec!
E500!  AIX!  Embedded ABI!  Rah rah rah.) that this could be
approached with some sort of confidence.

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Joel Brobecker]

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

> In this case, the stX 0,N(31) is spilling an argument, even though r0
> is not an argument register.  ('evstdd' is an E500 instruction that
> is definitely an argument spill.)

Dough!

> Clearly, both your function and mine need to go into the test suite...

I can add a new testcase in Ada for the prologue we saw.

> What if we did something like this?  It'd need to be combined with the 
> rest of your change, I'm just sketching:

Attached is a revised version incorporating your changes. Could you
give it a shot against your function, and let me know if it works for
you? It works for me, and doesn't introduce any regression on our
powerpc-aix-5.1 machine.

2004-04-16  Joel Brobecker  <brobecker@gnat.com>

        * rs6000-tdep.c (store_param_on_stack_p): New function,
        an improved version of some code extracted from skip_prologue().
        (skip_prologue): Use store_param_on_stack_p() to detect
        instructions saving a parameter on the stack. Detect when r0
        is used to save a parameter.
        Do not mark "li rx, SIMM" instructions as part of the prologue,
        unless the following instruction is also part of the prologue.
        
I'll followup with a testcase soon.

Thanks,
-- 
Joel

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

Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.191
diff -u -p -r1.191 rs6000-tdep.c
--- rs6000-tdep.c	1 Apr 2004 21:00:59 -0000	1.191
+++ rs6000-tdep.c	17 Apr 2004 04:58:34 -0000
@@ -419,6 +419,76 @@ refine_prologue_limit (CORE_ADDR pc, COR
   return lim_pc;
 }
 
+/* Return nonzero if the given instruction OP can be part of the prologue
+   of a function that saves a parameter on the stack.  FRAMEP should be
+   set if one of the previous instructions in the function has set the
+   Frame Pointer.  */
+
+static int
+store_param_on_stack_p (unsigned long op, int framep, int *r0_contains_arg)
+{
+  /* Move parameters from argument registers to temporary register.  */
+  if ((op & 0xfc0007fe) == 0x7c000378)         /* mr(.)  Rx,Ry */
+    {
+      /* Rx must be scratch register r0.  */
+      const int rx_regno = (op >> 16) & 31;
+      /* Ry: Only r3 - r10 are used for parameter passing.  */
+      const int ry_regno = GET_SRC_REG (op);
+
+      if (rx_regno == 0 && ry_regno >= 3 && ry_regno <= 10)
+        {
+          *r0_contains_arg = 1;
+          return 1;
+        }
+      else
+        return 0;
+    }
+
+  /* Save a General Purpose Register on stack.  */
+
+  if ((op & 0xfc1f0003) == 0xf8010000 ||       /* std  Rx,NUM(r1) */
+      (op & 0xfc1f0000) == 0xd8010000)         /* stfd Rx,NUM(r1) */
+    {
+      /* Rx: Only r3 - r10 are used for parameter passing.  */
+      const int rx_regno = GET_SRC_REG (op);
+
+      return (rx_regno >= 3 && rx_regno <= 10);
+    }
+           
+  /* Save a General Purpose Register on stack via the Frame Pointer.  */
+
+  if (framep &&
+      ((op & 0xfc1f0000) == 0x901f0000 ||     /* st rx,NUM(r31) */
+       (op & 0xfc1f0000) == 0x981f0000 ||     /* stb Rx,NUM(r31) */
+       (op & 0xfc1f0000) == 0xd81f0000))      /* stfd Rx,NUM(r31) */
+    {
+      /* Rx: Usually, only r3 - r10 are used for parameter passing.
+         However, the compiler sometimes uses r0 to hold an argument.  */
+      const int rx_regno = GET_SRC_REG (op);
+
+      return ((rx_regno >= 3 && rx_regno <= 10)
+              || (rx_regno == 0 && *r0_contains_arg));
+    }
+
+  if ((op & 0xfc1f0000) == 0xfc010000)         /* frsp, fp?,NUM(r1) */
+    {
+      /* Only f2 - f8 are used for parameter passing.  */
+      const int src_regno = GET_SRC_REG (op);
+
+      return (src_regno >= 2 && src_regno <= 8);
+    }
+
+  if (framep && ((op & 0xfc1f0000) == 0xfc1f0000))  /* frsp, fp?,NUM(r31) */
+    {
+      /* Only f2 - f8 are used for parameter passing.  */
+      const int src_regno = GET_SRC_REG (op);
+
+      return (src_regno >= 2 && src_regno <= 8);
+    }
+
+  /* Not an insn that saves a parameter on stack.  */
+  return 0;
+}
 
 static CORE_ADDR
 skip_prologue (CORE_ADDR pc, CORE_ADDR lim_pc, struct rs6000_framedata *fdata)
@@ -441,6 +511,7 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
   int minimal_toc_loaded = 0;
   int prev_insn_was_prologue_insn = 1;
   int num_skip_non_prologue_insns = 0;
+  int r0_contains_arg = 0;
   const struct bfd_arch_info *arch_info = gdbarch_bfd_arch_info (current_gdbarch);
   struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
   
@@ -509,11 +580,15 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
 	     ones.  */
 	  if (lr_reg < 0)
 	    lr_reg = (op & 0x03e00000);
+          if (lr_reg == 0)
+            r0_contains_arg = 0;
 	  continue;
 	}
       else if ((op & 0xfc1fffff) == 0x7c000026)
 	{			/* mfcr Rx */
 	  cr_reg = (op & 0x03e00000);
+          if (cr_reg == 0)
+            r0_contains_arg = 0;
 	  continue;
 
 	}
@@ -560,6 +635,7 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
 				   for >= 32k frames */
 	  fdata->offset = (op & 0x0000ffff) << 16;
 	  fdata->frameless = 0;
+          r0_contains_arg = 0;
 	  continue;
 
 	}
@@ -568,6 +644,7 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
 				   lf of >= 32k frames */
 	  fdata->offset |= (op & 0x0000ffff);
 	  fdata->frameless = 0;
+          r0_contains_arg = 0;
 	  continue;
 
 	}
@@ -701,26 +778,7 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
 	  /* store parameters in stack */
 	}
       /* Move parameters from argument registers to temporary register.  */
-      else if ((op & 0xfc0007fe) == 0x7c000378 &&	/* mr(.)  Rx,Ry */
-               (((op >> 21) & 31) >= 3) &&              /* R3 >= Ry >= R10 */
-               (((op >> 21) & 31) <= 10) &&
-               (((op >> 16) & 31) == 0)) /* Rx: scratch register r0 */
-        {
-          continue;
-        }
-      else if ((op & 0xfc1f0003) == 0xf8010000 ||	/* std rx,NUM(r1) */
-	       (op & 0xfc1f0000) == 0xd8010000 ||	/* stfd Rx,NUM(r1) */
-	       (op & 0xfc1f0000) == 0xfc010000)		/* frsp, fp?,NUM(r1) */
-	{
-	  continue;
-
-	  /* store parameters in stack via frame pointer */
-	}
-      else if (framep &&
-	       ((op & 0xfc1f0000) == 0x901f0000 ||     /* st rx,NUM(r31) */
-                (op & 0xfc1f0000) == 0x981f0000 ||     /* stb Rx,NUM(r31) */
-		(op & 0xfc1f0000) == 0xd81f0000 ||     /* stfd Rx,NUM(r31) */
-		(op & 0xfc1f0000) == 0xfc1f0000))      /* frsp, fp?,NUM(r31) */
+      else if (store_param_on_stack_p (op, framep, &r0_contains_arg))
         {
 	  continue;
 
@@ -789,8 +847,15 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
       else if ((op & 0xffff0000) == 0x38000000         /* li r0, SIMM */
                || (op & 0xffff0000) == 0x39c00000)     /* li r14, SIMM */
 	{
+          if ((op & 0xffff0000) == 0x38000000)
+            r0_contains_arg = 0;
 	  li_found_pc = pc;
 	  vr_saved_offset = SIGNED_SHORT (op);
+
+          /* This insn by itself is not part of the prologue, unless
+             if part of the pair of insns mentioned above. So do not
+             record this insn as part of the prologue yet.  */
+          prev_insn_was_prologue_insn = 0;
 	}
       /* Store vector register S at (r31+r0) aligned to 16 bytes.  */      
       /* 011111 sssss 11111 00000 00111001110 */

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Daniel Jacobowitz]

On Fri, Apr 16, 2004 at 10:15:45PM -0700, Joel Brobecker wrote:
> > In this case, the stX 0,N(31) is spilling an argument, even though r0
> > is not an argument register.  ('evstdd' is an E500 instruction that
> > is definitely an argument spill.)
> 
> Dough!
> 
> > Clearly, both your function and mine need to go into the test suite...
> 
> I can add a new testcase in Ada for the prologue we saw.

I'd prefer that you add them to gdb.asm, unless it's likely to produce
strange prologues on other architectures.  That way you can test what
you're trying to test, instead of something different.  For instance,
once dwarf2 unwinding is switched on for PPC, an Ada testcase is likely
to not trigger most of the prologue scanning.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> On Fri, Apr 16, 2004 at 10:15:45PM -0700, Joel Brobecker wrote:
> > > In this case, the stX 0,N(31) is spilling an argument, even though r0
> > > is not an argument register.  ('evstdd' is an E500 instruction that
> > > is definitely an argument spill.)
> > 
> > Dough!
> > 
> > > Clearly, both your function and mine need to go into the test suite...
> > 
> > I can add a new testcase in Ada for the prologue we saw.
> 
> I'd prefer that you add them to gdb.asm, unless it's likely to produce
> strange prologues on other architectures.  That way you can test what
> you're trying to test, instead of something different.  For instance,
> once dwarf2 unwinding is switched on for PPC, an Ada testcase is likely
> to not trigger most of the prologue scanning.

I totally agree.  Prologue analysis failures are a major part of
stabilizing a GDB port, at least for me.  I've never actually dumped
the exact prologues I've got on hand into a test script, but it seems
like the obvious thing to do.  Is there some reason we don't already?

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Daniel Jacobowitz]

On Mon, Apr 19, 2004 at 01:11:56AM -0500, Jim Blandy wrote:
> 
> Daniel Jacobowitz <drow@false.org> writes:
> > On Fri, Apr 16, 2004 at 10:15:45PM -0700, Joel Brobecker wrote:
> > > > In this case, the stX 0,N(31) is spilling an argument, even though r0
> > > > is not an argument register.  ('evstdd' is an E500 instruction that
> > > > is definitely an argument spill.)
> > > 
> > > Dough!
> > > 
> > > > Clearly, both your function and mine need to go into the test suite...
> > > 
> > > I can add a new testcase in Ada for the prologue we saw.
> > 
> > I'd prefer that you add them to gdb.asm, unless it's likely to produce
> > strange prologues on other architectures.  That way you can test what
> > you're trying to test, instead of something different.  For instance,
> > once dwarf2 unwinding is switched on for PPC, an Ada testcase is likely
> > to not trigger most of the prologue scanning.
> 
> I totally agree.  Prologue analysis failures are a major part of
> stabilizing a GDB port, at least for me.  I've never actually dumped
> the exact prologues I've got on hand into a test script, but it seems
> like the obvious thing to do.  Is there some reason we don't already?

We do now :)  There's a collection of SH prologues, at least.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Joel Brobecker]

> I'd prefer that you add them to gdb.asm, unless it's likely to produce
> strange prologues on other architectures.

I looked at the gdb.asm subdirectory, and found only one test there:
asm-source.exp. It doesn't look like this testcase would be the correct
location where to add a test for this prologue.

So should I add a new testcase? This testcase would only be activated
for powerpc*-*-* targets.

In terms of the code, I would just dump the assembly code for the
function in question into an .s file. To perform the link, I'm tempted
between do it all in asm (just as we do in asm-source.exp), or see
if it is simpler if I use a C main...

All the testcase would do is: Build the executable, load it, and then
insert a breakpoint in my function.

Am I on the right track?

BTW: I can't find the collection of SH prologues that Daniel was
refering to...

Thanks,
-- 
Joel

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Daniel Jacobowitz]

On Mon, Apr 19, 2004 at 10:53:39AM -0700, Joel Brobecker wrote:
> > I'd prefer that you add them to gdb.asm, unless it's likely to produce
> > strange prologues on other architectures.
> 
> I looked at the gdb.asm subdirectory, and found only one test there:
> asm-source.exp. It doesn't look like this testcase would be the correct
> location where to add a test for this prologue.
> 
> So should I add a new testcase? This testcase would only be activated
> for powerpc*-*-* targets.
> 
> In terms of the code, I would just dump the assembly code for the
> function in question into an .s file. To perform the link, I'm tempted
> between do it all in asm (just as we do in asm-source.exp), or see
> if it is simpler if I use a C main...
> 
> All the testcase would do is: Build the executable, load it, and then
> insert a breakpoint in my function.
> 
> Am I on the right track?
> 
> BTW: I can't find the collection of SH prologues that Daniel was
> refering to...

That's because I was confused - I meant gdb.arch.  Take a look at
gdb1431.exp.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Jim Blandy]

Joel Brobecker <brobecker@gnat.com> writes:

> > I'd prefer that you add them to gdb.asm, unless it's likely to produce
> > strange prologues on other architectures.
> 
> I looked at the gdb.asm subdirectory, and found only one test there:
> asm-source.exp. It doesn't look like this testcase would be the correct
> location where to add a test for this prologue.
> 
> So should I add a new testcase? This testcase would only be activated
> for powerpc*-*-* targets.
> 
> In terms of the code, I would just dump the assembly code for the
> function in question into an .s file. To perform the link, I'm tempted
> between do it all in asm (just as we do in asm-source.exp), or see
> if it is simpler if I use a C main...
> 
> All the testcase would do is: Build the executable, load it, and then
> insert a breakpoint in my function.
> 
> Am I on the right track?

Well, that's what I had in mind.  One file for each architecture,
packed full of functions with interesting prologues.  The tests would
just set breakpoints on each of them and check that they get set at
the right distance from the entry point.

My test case uses E500-specific instructions.  I could rewrite it so
it didn't, but the prologue analyzer does have E500-specific code, so
it needs to be tested anyway.  So I'd probably need a separate test
file.


> BTW: I can't find the collection of SH prologues that Daniel was
> refering to...

Me neither.

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Jim Blandy]

Jim Blandy <jimb@redhat.com> writes:

> Joel Brobecker <brobecker@gnat.com> writes:
> 
> > > I'd prefer that you add them to gdb.asm, unless it's likely to produce
> > > strange prologues on other architectures.
> > 
> > I looked at the gdb.asm subdirectory, and found only one test there:
> > asm-source.exp. It doesn't look like this testcase would be the correct
> > location where to add a test for this prologue.
> > 
> > So should I add a new testcase? This testcase would only be activated
> > for powerpc*-*-* targets.
> > 
> > In terms of the code, I would just dump the assembly code for the
> > function in question into an .s file. To perform the link, I'm tempted
> > between do it all in asm (just as we do in asm-source.exp), or see
> > if it is simpler if I use a C main...
> > 
> > All the testcase would do is: Build the executable, load it, and then
> > insert a breakpoint in my function.
> > 
> > Am I on the right track?
> 
> Well, that's what I had in mind.  One file for each architecture,
> packed full of functions with interesting prologues.  The tests would
> just set breakpoints on each of them and check that they get set at
> the right distance from the entry point.
> 
> My test case uses E500-specific instructions.  I could rewrite it so
> it didn't, but the prologue analyzer does have E500-specific code, so
> it needs to be tested anyway.  So I'd probably need a separate test
> file.

Okay, I see now that i386-prologue.c actually contains top-level 'asm'
statements that explicitly spell out the instruction sequence to test.
I had jumped to the conclusion that these were just C functions for
which some compiler at some point in the past had generated prologues
that confused GDB.  Doing things the way i386-prologue.c does makes
sense to me.

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Jim Blandy]

> > Attached is a revised version incorporating your changes. Could you
> > give it a shot against your function, and let me know if it works for
> > you? It works for me, and doesn't introduce any regression on our
> > powerpc-aix-5.1 machine.
> > 
> > 2004-04-16  Joel Brobecker  <brobecker@gnat.com>
> > 
> >         * rs6000-tdep.c (store_param_on_stack_p): New function,
> >         an improved version of some code extracted from skip_prologue().
> >         (skip_prologue): Use store_param_on_stack_p() to detect
> >         instructions saving a parameter on the stack. Detect when r0
> >         is used to save a parameter.
> >         Do not mark "li rx, SIMM" instructions as part of the prologue,
> >         unless the following instruction is also part of the prologue.
> >         
> > I'll followup with a testcase soon.

I've finally been able to give this a shot, and it works fine on my
prologue, too.  So I have no objections to the patch.  Thanks for your
patience, Joel!

I've got a test script now for PPC E500 prologues, which I'll post
under separate cover.

Re: [RFA] Fix small problems in rs6000-tdep.c:skip_prologue()

[Joel Brobecker]

> > > > 2004-04-16  Joel Brobecker  <brobecker@gnat.com>
> > > > 
> > > >         * rs6000-tdep.c (store_param_on_stack_p): New function,
> > > >         an improved version of some code extracted from skip_prologue().
> > > >         (skip_prologue): Use store_param_on_stack_p() to detect
> > > >         instructions saving a parameter on the stack. Detect when r0
> > > >         is used to save a parameter.
> > > >         Do not mark "li rx, SIMM" instructions as part of the prologue,
> > > >         unless the following instruction is also part of the prologue.
> 
> Joel, your revised patch is approved.

Thanks a lot. I've checked the patch in.

-- 
Joel