PowerPC prologue analysis

PowerPC prologue analysis

[Aleksandar Ristovski]

Hello,

In the code, in rs6000-tdep.c around line 3334, there is a comment stating:

  /* if != -1, fdata.saved_gpr is the smallest number of saved_gpr.
     All gpr's from saved_gpr to gpr31 are saved.  */

I am, however, witnessing a function that appears to be saving r30, but not r31 (see the disassembly below). This, in turn, causes gdb to unwind r31 from a 'saved' area even though the area does not exist.

I am not very familiar with PowerPC ABI, but from what I gather reading the "function call" section, but can not see where is it stated that if r30 is saved, then r31 must be saved too? But again, I haven't studied the ABI very thoroughly and might be missing that line. 


Just for the reference, here is the disassembly of the function:

(gdb) disassemble foo
Dump of assembler code for function foo
0xfe346aa0 <foo+0>:    stwu    r1,-16(r1)
0xfe346aa4 <foo+4>:    mflr    r0
0xfe346aa8 <foo+8>:    bl      0xfe37ca18
0xfe346aac <foo+12>:   mr      r4,r3
0xfe346ab0 <foo+16>:   stw     r30,8(r1)
0xfe346ab4 <foo+20>:   mflr    r30
0xfe346ab8 <foo+24>:   li      r5,0
0xfe346abc <foo+28>:   li      r6,0
0xfe346ac0 <foo+32>:   stw     r0,20(r1)
0xfe346ac4 <foo+36>:   lwz     r3,-176(r30)
0xfe346ac8 <foo+40>:   bl      0xfe37d738
0xfe346acc <foo+44>:   lwz     r0,20(r1)
0xfe346ad0 <foo+48>:   lwz     r30,8(r1)
0xfe346ad4 <foo+52>:   addi    r1,r1,16
0xfe346ad8 <foo+56>:   mtlr    r0
0xfe346adc <foo+60>:   blr
End of assembler dump.


Thanks,

Aleksandar Ristovski
QNX Software Systems

Re: PowerPC prologue analysis

[Daniel Jacobowitz]

On Mon, Jul 28, 2008 at 04:09:54PM -0400, Aleksandar Ristovski wrote:
> Hello,
>
> In the code, in rs6000-tdep.c around line 3334, there is a comment stating:
>
>  /* if != -1, fdata.saved_gpr is the smallest number of saved_gpr.
>     All gpr's from saved_gpr to gpr31 are saved.  */
>
> I am, however, witnessing a function that appears to be saving r30, but not r31 (see the disassembly below). This, in turn, causes gdb to unwind r31 from a 'saved' area even though the area does not exist.
>
> I am not very familiar with PowerPC ABI, but from what I gather reading 
> the "function call" section, but can not see where is it stated that if 
> r30 is saved, then r31 must be saved too? But again, I haven't studied the 
> ABI very thoroughly and might be missing that line. 

Might want to look at this patch:

http://sourceware.org/ml/gdb-patches/2007-12/msg00111.html

-- 
Daniel Jacobowitz
CodeSourcery

Re: PowerPC prologue analysis

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Mon, Jul 28, 2008 at 04:09:54PM -0400, Aleksandar Ristovski wrote:
>> I am not very familiar with PowerPC ABI, but from what I gather reading 
>> the "function call" section, but can not see where is it stated that if 
>> r30 is saved, then r31 must be saved too? But again, I haven't studied the 
>> ABI very thoroughly and might be missing that line. 
> 
> Might want to look at this patch:
> 
> http://sourceware.org/ml/gdb-patches/2007-12/msg00111.html
> 

Thanks for the link! I briefly looked at the patch and it seems to address some of the things I am talking about (r30-r31 issue) but the comment still reads:

+     All gpr's from saved_gpr to gpr31 are saved (except during the
+     prologue).  */
 
Is that in the ABI? I would think that if it is, then the code I am looking at is not according to it (gcc issue or just me not understanding powerpc assembly?).

Re: PowerPC prologue analysis

[Daniel Jacobowitz]

On Mon, Jul 28, 2008 at 04:29:18PM -0400, Aleksandar Ristovski wrote:
> Thanks for the link! I briefly looked at the patch and it seems to address some of the things I am talking about (r30-r31 issue) but the comment still reads:
>
> +     All gpr's from saved_gpr to gpr31 are saved (except during the
> +     prologue).  */
>
> Is that in the ABI? I would think that if it is, then the code I am looking at is not according to it (gcc issue or just me not understanding powerpc assembly?).

Sorry, I don't know which.

-- 
Daniel Jacobowitz
CodeSourcery

Re: PowerPC prologue analysis

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Mon, Jul 28, 2008 at 04:29:18PM -0400, Aleksandar Ristovski wrote:
>> Thanks for the link! I briefly looked at the patch and it seems to address some of the things I am talking about (r30-r31 issue) but the comment still reads:
>>
>> +     All gpr's from saved_gpr to gpr31 are saved (except during the
>> +     prologue).  */
>>
>> Is that in the ABI? I would think that if it is, then the code I am looking at is not according to it (gcc issue or just me not understanding powerpc assembly?).
> 
> Sorry, I don't know which.
> 

There seems to be no such statement in the PowerPC ABI "all gpr's from saved_gpr to gpr31 are saved" - non-volatile registers do not need to be stored in this manner. For example, a function may save r29 but not r30 and r31. However PPC prologue analysis in gdb will assume there is r31 saved as well which will make unwind_register(r31) fail (fetch bogus value). (Note: I am using gcc 4.2.3).

Another assumption made in gdb code is that if multiple registers are saved by the prologue, they will be saved in the ascending index order - I am not sure this is a requirement stated in the ABI either. (Is it?) I believe there could be cases where registers are saved in different order, e.g. r30, r28, r29, etc... Hopefully this doesn't happen in practise.

I think your patch with gpr_mask covers the first case (not all registers saved) but the second issue (if real issue) is still not handled.

Re: PowerPC prologue analysis

[Aleksandar Ristovski]

Aleksandar Ristovski wrote:
> Daniel Jacobowitz wrote:
>> On Mon, Jul 28, 2008 at 04:29:18PM -0400, Aleksandar Ristovski wrote:
>>> Thanks for the link! I briefly looked at the patch and it seems to 
>>> address some of the things I am talking about (r30-r31 issue) but the 
>>> comment still reads:
>>>
>>> +     All gpr's from saved_gpr to gpr31 are saved (except during the
>>> +     prologue).  */
>>>
>>> Is that in the ABI? I would think that if it is, then the code I am 
>>> looking at is not according to it (gcc issue or just me not 
>>> understanding powerpc assembly?).
>>
>> Sorry, I don't know which.
>>
> 
> There seems to be no such statement in the PowerPC ABI "all gpr's from 
> saved_gpr to gpr31 are saved" - non-volatile registers do not need to be 
> stored in this manner. For example, a function may save r29 but not r30 
> and r31. However PPC prologue analysis in gdb will assume there is r31 
> saved as well which will make unwind_register(r31) fail (fetch bogus 
> value). (Note: I am using gcc 4.2.3).
> 
> Another assumption made in gdb code is that if multiple registers are 
> saved by the prologue, they will be saved in the ascending index order - 
> I am not sure this is a requirement stated in the ABI either. (Is it?) I 
> believe there could be cases where registers are saved in different 
> order, e.g. r30, r28, r29, etc... Hopefully this doesn't happen in 
> practise.
> 
> I think your patch with gpr_mask covers the first case (not all 
> registers saved) but the second issue (if real issue) is still not handled.


And for the reference: Daniel's patch ( http://sourceware.org/ml/gdb-patches/2007-12/msg00111.html ) does solve the problem with not all registers saved.

Why is the patch not in HEAD gdb?

Re: PowerPC prologue analysis

[Daniel Jacobowitz]

On Tue, Jul 29, 2008 at 11:42:29AM -0400, Aleksandar Ristovski wrote:
> And for the reference: Daniel's patch (
> http://sourceware.org/ml/gdb-patches/2007-12/msg00111.html ) does
> solve the problem with not all registers saved.
>
> Why is the patch not in HEAD gdb?

There's a couple of questions in the posting and I never figured out
the answers to them.  I'll be back to them at some point.

-- 
Daniel Jacobowitz
CodeSourcery

Re: PowerPC prologue analysis

[Aleksandar Ristovski]

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

Daniel Jacobowitz wrote:
> On Tue, Jul 29, 2008 at 11:42:29AM -0400, Aleksandar Ristovski wrote:
>> And for the reference: Daniel's patch (
>> http://sourceware.org/ml/gdb-patches/2007-12/msg00111.html ) does
>> solve the problem with not all registers saved.
>>
>> Why is the patch not in HEAD gdb?
> 
> There's a couple of questions in the posting and I never figured out
> the answers to them.  I'll be back to them at some point.
> 

May I suggest that you break the patch in two patches: one dealing with not-all-registers-saved (i.e. gpr_mask) and the rest?

I extracted the gpr_mask part here with the testsuite changes and I believe this part of the patch is safe to apply. 

Attached: Fragment extracted from Daniel's patch.




[-- Attachment #2: rs6000-tdep.c.Daniel-gpr_mask-part.diff --]
[-- Type: text/plain, Size: 2154 bytes --]

Index: gdb/rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.318
diff -u -p -r1.318 rs6000-tdep.c
--- gdb/rs6000-tdep.c	15 Jul 2008 18:32:06 -0000	1.318
+++ gdb/rs6000-tdep.c	29 Jul 2008 16:27:53 -0000
@@ -117,6 +117,7 @@ struct rs6000_framedata
 				   by which we decrement sp to allocate
 				   the frame */
     int saved_gpr;		/* smallest # of saved gpr */
+    unsigned int gpr_mask;	/* Each bit is an individual saved GPR.  */
     int saved_fpr;		/* smallest # of saved fpr */
     int saved_vr;               /* smallest # of saved vr */
     int saved_ev;               /* smallest # of saved ev */
@@ -1275,6 +1276,10 @@ skip_prologue (struct gdbarch *gdbarch, 
 	{
 
 	  reg = GET_SRC_REG (op);
+	  if ((op & 0xfc1f0000) == 0xbc010000)
+	    fdata->gpr_mask |= ~((1U << reg) - 1);
+	  else
+	    fdata->gpr_mask |= 1U << reg;
 	  if (fdata->saved_gpr == -1 || fdata->saved_gpr > reg)
 	    {
 	      fdata->saved_gpr = reg;
@@ -1678,11 +1683,15 @@ skip_prologue (struct gdbarch *gdbarch, 
 
       else
 	{
+	  unsigned int all_mask = ~((1U << fdata->saved_gpr) - 1);
+
 	  /* Not a recognized prologue instruction.
 	     Handle optimizer code motions into the prologue by continuing
 	     the search if we have no valid frame yet or if the return
-	     address is not yet saved in the frame.  */
-	  if (fdata->frameless == 0 && fdata->nosavedpc == 0)
+	     address is not yet saved in the frame.  Also skip instructions
+	     if some of the GPRs expected to be saved are not yet saved.  */
+	  if (fdata->frameless == 0 && fdata->nosavedpc == 0
+	      && (fdata->gpr_mask & all_mask) == all_mask)
 	    break;
 
 	  if (op == 0x4e800020		/* blr */
@@ -2573,7 +2582,8 @@ rs6000_frame_cache (struct frame_info *t
       CORE_ADDR gpr_addr = cache->base + fdata.gpr_offset;
       for (i = fdata.saved_gpr; i < ppc_num_gprs; i++)
 	{
-	  cache->saved_regs[tdep->ppc_gp0_regnum + i].addr = gpr_addr;
+	  if (fdata.gpr_mask & (1U << i))
+	    cache->saved_regs[tdep->ppc_gp0_regnum + i].addr = gpr_addr;
 	  gpr_addr += wordsize;
 	}
     }

[-- Attachment #3: gdb_arch_powerpc-prologue.exp.Daniel-gpr_mask-part.diff --]
[-- Type: text/plain, Size: 745 bytes --]

Index: gdb/testsuite/gdb.arch/powerpc-prologue.exp
===================================================================
--- gdb/testsuite/gdb.arch/powerpc-prologue.exp	(revision 197)
+++ gdb/testsuite/gdb.arch/powerpc-prologue.exp	(working copy)
@@ -64,7 +64,7 @@ gdb_test "backtrace 10" \
 	"backtrace in PIC"
 
 gdb_test "info frame" \
-	".*Saved registers:.*r30 at.*r31 at.*pc at.*lr at.*" \
+	".*Saved registers:.*r30 at.*pc at.*lr at.*" \
 	"saved registers in PIC"
 
 # Testcase for scheduled prologue.
@@ -84,5 +84,5 @@ gdb_test "backtrace 10" \
 	"backtrace in optimized"
 
 gdb_test "info frame" \
-	".*Saved registers:.*r30 at.*r31 at.*pc at.*lr at.*" \
+	".*Saved registers:.*r30 at.*pc at.*lr at.*" \
 	"saved registers in optimized"