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

[Jim Blandy <jimb@redhat.com>]

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);
  	}