Fix for prologue processing on PowerPC

Fix for prologue processing on PowerPC

[Nikola Prica]

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

After analyzing dump of ppc program, whose crash occurred after 
watchdog_force_here () function, GDB couldn't print full back trace 
because GDB couldn't unwind PC from the watchdog fucntion.

The problem is introduced with the following patch:

https://sourceware.org/ml/gdb-patches/2008-08/msg00245.html

In function skip_prologue(), shifted lr_reg makes below condition always 
false because non-shifted lr_reg value is expected to be checked.

    else if (lr_reg >= 0 &&
         /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */
         (((op & 0xffff0000) == (lr_reg | 0xf8010000)) ||
         /* stw Rx, NUM(r1) */
         ((op & 0xffff0000) == (lr_reg | 0x90010000)) ||
         /* stwu Rx, NUM(r1) */
         ((op & 0xffff0000) == (lr_reg | 0x94010000))))

Before this fix unwinding was able to work because it relied on unwind
directives or on some of the next frames to find PC. Problem came with
watchdog_force_here() function which didn't contain unwind directives.

I wasn't able to produce test case that would show improvements for end
user. I suppose that changes would be visible if watchdog event was 
called, but I don't have valid ppc board to try this. I have tried this 
code on simple test case with few functions in back trace. The back 
trace is printed correctly with and without this fix, but the difference 
between those two runs is that the body of the upper condition was 
visited with this patch. After visiting the body there was no need to 
look for PC counter in next frames nor to use unwind directives.

[-- Attachment #2: PowerPC-fix-for-prologue-processing.patch --]
[-- Type: text/x-patch, Size: 1733 bytes --]

From 93a406efd79552eba7fc55ed3b7f293cdcf324ef Mon Sep 17 00:00:00 2001
From: Prica <nprica@rt-rk.com>
Date: Tue, 19 Sep 2017 17:52:35 +0200
Subject: [PATCH] PowerPC, fix for prologue processing

   One of conditions in skip_prologue() is never visited because it
   expects non shifted `lr_reg`. That condition is supposed to set PC
   offset. When body of this condition is visited PC offset is set and
   there will be no need to look for it in next frames nor to use frame
   unwind directives.

gdb/ChangeLog:
        *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg
         and assign shifted lr_reg to fdata->lr_register when lr_reg is
         set.
---
 gdb/rs6000-tdep.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 95b2ca7..7f64901 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1652,11 +1652,14 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
 
 	     remember just the first one, but skip over additional
 	     ones.  */
-	  if (lr_reg == -1)
-	    lr_reg = (op & 0x03e00000) >> 21;
-          if (lr_reg == 0)
-            r0_contains_arg = 0;
-	  continue;
+    if (lr_reg == -1)
+      {
+        lr_reg = (op & 0x03e00000);
+        fdata->lr_register = lr_reg >> 21;
+      }
+    if (lr_reg == 0)
+      r0_contains_arg = 0;
+    continue;
 	}
       else if ((op & 0xfc1fffff) == 0x7c000026)
 	{			/* mfcr Rx */
@@ -2178,9 +2181,6 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
     }
 #endif /* 0 */
 
-  if (pc == lim_pc && lr_reg >= 0)
-    fdata->lr_register = lr_reg;
-
   fdata->offset = -fdata->offset;
   return last_prologue_pc;
 }
-- 
2.7.4

Re: Fix for prologue processing on PowerPC

[Kevin Buettner]

On Fri, 22 Sep 2017 14:11:51 +0200
Nikola Prica <nikola.prica@rt-rk.com> wrote:

>    One of conditions in skip_prologue() is never visited because it
>    expects non shifted `lr_reg`. That condition is supposed to set PC
>    offset. When body of this condition is visited PC offset is set and
>    there will be no need to look for it in next frames nor to use frame
>    unwind directives.
> 
> gdb/ChangeLog:
>         *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg
>          and assign shifted lr_reg to fdata->lr_register when lr_reg is
>          set.
> ---
>  gdb/rs6000-tdep.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 95b2ca7..7f64901 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1652,11 +1652,14 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
>  
>  	     remember just the first one, but skip over additional
>  	     ones.  */
> -	  if (lr_reg == -1)
> -	    lr_reg = (op & 0x03e00000) >> 21;
> -          if (lr_reg == 0)
> -            r0_contains_arg = 0;
> -	  continue;
> +    if (lr_reg == -1)
> +      {
> +        lr_reg = (op & 0x03e00000);
> +        fdata->lr_register = lr_reg >> 21;
> +      }
> +    if (lr_reg == 0)
> +      r0_contains_arg = 0;
> +    continue;
>  	}
>        else if ((op & 0xfc1fffff) == 0x7c000026)
>  	{			/* mfcr Rx */
> @@ -2178,9 +2181,6 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
>      }
>  #endif /* 0 */
>  
> -  if (pc == lim_pc && lr_reg >= 0)
> -    fdata->lr_register = lr_reg;
> -
>    fdata->offset = -fdata->offset;
>    return last_prologue_pc;
>  }
> -- 
> 2.7.4
> 

Hi Nikola,

The current version of the code causes fdata->lr_register to be set
only when lr_reg has been previously set ( >= 0) and when pc == lim_pc.

I.e. it won't be set when the loop scanning the prologue is exited early.
Just looking over that loop, there are a number of break statements which
might cause an early loop exit.

My concern about your patch is that by setting fdata->lr_register
within the loop, fdata->lr_register could end up having a value which
we don't want it to have for one of those early loop exits when pc != lim_pc.

That said, I have not done an especially deep analysis of this matter.
If you think that fdata->offset should be set for all of those other
conditions too, then...  Well, I'm willing to consider this proposition.
But I'd like to see some analysis of why this is so.

Otherwise, it seems to me that the patch below (which I have not
tested) might fix the bug that you found, but not perturb the logic
regarding pc != lim_pc ?  If it looks okay to you, please try it out
on your test case (and regression test it using the testsuite too). 
If it's okay, then go ahead and commit it with a suitably adjusted
ChangeLog entry.

Thanks,

Kevin

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 95b2ca7..86c0915 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1653,7 +1653,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
 	     remember just the first one, but skip over additional
 	     ones.  */
 	  if (lr_reg == -1)
-	    lr_reg = (op & 0x03e00000) >> 21;
+	    lr_reg = op & 0x03e00000;
           if (lr_reg == 0)
             r0_contains_arg = 0;
 	  continue;
@@ -2179,7 +2179,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
 #endif /* 0 */
 
   if (pc == lim_pc && lr_reg >= 0)
-    fdata->lr_register = lr_reg;
+    fdata->lr_register = lr_reg >> 21;
 
   fdata->offset = -fdata->offset;
   return last_prologue_pc;

[PING]Fix for prologue processing on PowerPC

[Nikola Prica]

Can someone please take a look into this?

Thanks,

Nikola Prica

On 22.09.2017. 14:11, Nikola Prica wrote:
> After analyzing dump of ppc program, whose crash occurred after 
> watchdog_force_here () function, GDB couldn't print full back trace 
> because GDB couldn't unwind PC from the watchdog fucntion.
> 
> The problem is introduced with the following patch:
> 
> https://sourceware.org/ml/gdb-patches/2008-08/msg00245.html
> 
> In function skip_prologue(), shifted lr_reg makes below condition always 
> false because non-shifted lr_reg value is expected to be checked.
> 
>     else if (lr_reg >= 0 &&
>          /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */
>          (((op & 0xffff0000) == (lr_reg | 0xf8010000)) ||
>          /* stw Rx, NUM(r1) */
>          ((op & 0xffff0000) == (lr_reg | 0x90010000)) ||
>          /* stwu Rx, NUM(r1) */
>          ((op & 0xffff0000) == (lr_reg | 0x94010000))))
> 
> Before this fix unwinding was able to work because it relied on unwind
> directives or on some of the next frames to find PC. Problem came with
> watchdog_force_here() function which didn't contain unwind directives.
> 
> I wasn't able to produce test case that would show improvements for end
> user. I suppose that changes would be visible if watchdog event was 
> called, but I don't have valid ppc board to try this. I have tried this 
> code on simple test case with few functions in back trace. The back 
> trace is printed correctly with and without this fix, but the difference 
> between those two runs is that the body of the upper condition was 
> visited with this patch. After visiting the body there was no need to 
> look for PC counter in next frames nor to use unwind directives.

Re: [PING]Fix for prologue processing on PowerPC

[Kevin Buettner]

See:

https://sourceware.org/ml/gdb-patches/2017-10/msg00098.html

Kevin

On Thu, 26 Oct 2017 12:02:19 +0200
Nikola Prica <nikola.prica@rt-rk.com> wrote:

> Can someone please take a look into this?
> 
> Thanks,
> 
> Nikola Prica
> 
> On 22.09.2017. 14:11, Nikola Prica wrote:
> > After analyzing dump of ppc program, whose crash occurred after 
> > watchdog_force_here () function, GDB couldn't print full back trace 
> > because GDB couldn't unwind PC from the watchdog fucntion.
> > 
> > The problem is introduced with the following patch:
> > 
> > https://sourceware.org/ml/gdb-patches/2008-08/msg00245.html
> > 
> > In function skip_prologue(), shifted lr_reg makes below condition always 
> > false because non-shifted lr_reg value is expected to be checked.
> > 
> >     else if (lr_reg >= 0 &&
> >          /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */
> >          (((op & 0xffff0000) == (lr_reg | 0xf8010000)) ||
> >          /* stw Rx, NUM(r1) */
> >          ((op & 0xffff0000) == (lr_reg | 0x90010000)) ||
> >          /* stwu Rx, NUM(r1) */
> >          ((op & 0xffff0000) == (lr_reg | 0x94010000))))
> > 
> > Before this fix unwinding was able to work because it relied on unwind
> > directives or on some of the next frames to find PC. Problem came with
> > watchdog_force_here() function which didn't contain unwind directives.
> > 
> > I wasn't able to produce test case that would show improvements for end
> > user. I suppose that changes would be visible if watchdog event was 
> > called, but I don't have valid ppc board to try this. I have tried this 
> > code on simple test case with few functions in back trace. The back 
> > trace is printed correctly with and without this fix, but the difference 
> > between those two runs is that the body of the upper condition was 
> > visited with this patch. After visiting the body there was no need to 
> > look for PC counter in next frames nor to use unwind directives.  

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Nikola Prica]

Hi Kevin,

lr_reg could be also set to -2 in part of code which is reachable after 
shifting removal.

       /* Invalidate lr_reg, but don't set it to -1.
          That would mean that it had never been set.  */
       lr_reg = -2;

This part of the code which depends of non shifted lr_reg, and the part 
where shifting is removed are only two places where lr_reg is changed. 
As so, I've added last condition to set fdata->lr_register on -1 if 
lim_pc is not reached.

If it seems fine now could you pleas commit it because I don't have 
rights to do it.

Thanks,

Nikola Prica


From: Prica <nprica@rt-rk.com>
Date: Thu, 9 Nov 2017 13:10:48 +0100
Subject: Fix for prologue processing on PowerPC

One of conditions in skip_prologue() is never visited because it
expects non shifted `lr_reg`.  That condition is supposed to set PC
offset.  When body of this condition is visited PC offset is set and
there will be no need to look for it in next frames nor to use frame
unwind directives.

gdb/ChangeLog:

	*rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg
   	and assign shifted lr_reg to fdata->lr_register when lr_reg is
   	set. If iteration do not hit lim_pc lr_register is set as -1.
---
  gdb/rs6000-tdep.c | 13 ++++++++-----
  1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 6c44995..6f05ef5 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR 
pc, CORE_ADDR lim_pc,
  	     remember just the first one, but skip over additional
  	     ones.  */
  	  if (lr_reg == -1)
-	    lr_reg = (op & 0x03e00000) >> 21;
-          if (lr_reg == 0)
-            r0_contains_arg = 0;
+      {
+        lr_reg = (op & 0x03e00000);
+        fdata->lr_register = lr_reg >> 21;
+        if (lr_reg == 0)
+          r0_contains_arg = 0;
+      }
  	  continue;
  	}
        else if ((op & 0xfc1fffff) == 0x7c000026)
@@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR 
pc, CORE_ADDR lim_pc,
      }
  #endif /* 0 */

-  if (pc == lim_pc && lr_reg >= 0)
-    fdata->lr_register = lr_reg;
+  if (pc != lim_pc)
+    fdata->lr_register = -1;

    fdata->offset = -fdata->offset;
    return last_prologue_pc;
-- 
2.7.4

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[pedromfc]

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

Hello Nikola, Kevin,

Thank you for providing these patches.

I tested both patches (Nicola's second patch and Kevin's patch) on 
ppc64le, and there were no regressions, except for some additional 
expected failures in gdb.threads/attach-many-short-lived-threads.exp.

Some comments:

- Nikola's patch moves "if(lr_reg == 0)/r0_contains_arg = 0;" to within 
the first if. This is useful for the case when a second mflr Rx with Rx 
!= 0 is detected. Previously this would cause r0_contains_arg to be 
reset, despite Rx not being 0. However, if the second mflr has Rx == 0, 
it would make sense to reset r0_contains_arg (and this would work 
accidentally before the patch, assuming the first mflr also had Rx == 
0). Maybe the best solution here is to always check the Rx contained in 
the opcode and clear r0_contains_arg if it is 0, regardless of lr_reg, 
or leave it as before since it's a separate issue.

- Kevin's patch assigns lr_reg = op & 0x03e00000, but lr_reg is an int, 
and op is an unsigned long. Will the unshifted reg always fit in a int?

- I think it's more clear to only set lr_register when needed (pc 
reaches the limit), as opposed to resetting it to -1 if pc didn't reach 
the limit.

- I wasn't able to directly apply Nikola's patch, I did so manually.

I've also attached a reproducer (prologue.c/foo.S) to check if the 
patches fixed the issue. I had to alter a generated assembly file so 
that mflr would use a register other than R0 (in which case the old code 
does work).

gcc -g0 -O0 -o prologue prologue.c foo.S

(gdb) file prologue
Reading symbols from prologue...done.
(gdb) break bar
Breakpoint 1 at 0x100005b8
(gdb) run

Starting program: /home/pedromfc/prologue

Before the patch:

Breakpoint 1, 0x00000000100005b8 in bar ()
(gdb) bt
#0  0x00000000100005b8 in bar ()
#1  0x0000000010000644 in foo ()
Backtrace stopped: frame did not save the PC

After (both patches):

Breakpoint 1, 0x00000000100005b8 in bar ()
(gdb) bt
#0  0x00000000100005b8 in bar ()
#1  0x0000000010000644 in foo ()
#2  0x00000000100005f8 in main ()

(gdb) info f 1
Stack frame at 0x7fffffffeee0:
  pc = 0x10000644 in foo; saved pc = 0x100005f8

Thanks!
Pedro

On 11/09/2017 04:15 PM, Nikola Prica wrote:
> Hi Kevin,
>
> lr_reg could be also set to -2 in part of code which is reachable 
> after shifting removal.
>
>       /* Invalidate lr_reg, but don't set it to -1.
>          That would mean that it had never been set.  */
>       lr_reg = -2;
>
> This part of the code which depends of non shifted lr_reg, and the 
> part where shifting is removed are only two places where lr_reg is 
> changed. As so, I've added last condition to set fdata->lr_register on 
> -1 if lim_pc is not reached.
>
> If it seems fine now could you pleas commit it because I don't have 
> rights to do it.
>
> Thanks,
>
> Nikola Prica
>
>
> From: Prica <nprica@rt-rk.com>
> Date: Thu, 9 Nov 2017 13:10:48 +0100
> Subject: Fix for prologue processing on PowerPC
>
> One of conditions in skip_prologue() is never visited because it
> expects non shifted `lr_reg`.  That condition is supposed to set PC
> offset.  When body of this condition is visited PC offset is set and
> there will be no need to look for it in next frames nor to use frame
> unwind directives.
>
> gdb/ChangeLog:
>
>     *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg
>       and assign shifted lr_reg to fdata->lr_register when lr_reg is
>       set. If iteration do not hit lim_pc lr_register is set as -1.
> ---
>  gdb/rs6000-tdep.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 6c44995..6f05ef5 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, 
> CORE_ADDR pc, CORE_ADDR lim_pc,
>           remember just the first one, but skip over additional
>           ones.  */
>        if (lr_reg == -1)
> -        lr_reg = (op & 0x03e00000) >> 21;
> -          if (lr_reg == 0)
> -            r0_contains_arg = 0;
> +      {
> +        lr_reg = (op & 0x03e00000);
> +        fdata->lr_register = lr_reg >> 21;
> +        if (lr_reg == 0)
> +          r0_contains_arg = 0;
> +      }
>        continue;
>      }
>        else if ((op & 0xfc1fffff) == 0x7c000026)
> @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, 
> CORE_ADDR pc, CORE_ADDR lim_pc,
>      }
>  #endif /* 0 */
>
> -  if (pc == lim_pc && lr_reg >= 0)
> -    fdata->lr_register = lr_reg;
> +  if (pc != lim_pc)
> +    fdata->lr_register = -1;
>
>    fdata->offset = -fdata->offset;
>    return last_prologue_pc;


[-- Attachment #2: prologue.c --]
[-- Type: text/x-csrc, Size: 74 bytes --]

int bar()
{
  return 0;
}

int foo();

int main(void)
{
  return foo();
}

[-- Attachment #3: foo.S --]
[-- Type: text/plain, Size: 486 bytes --]

	.file	"foo.c"
	.abiversion 2
	.section	".text"
	.align 2
	.globl foo
	.type	foo, @function
foo:
.LCF0:
0:	addis 2,12,.TOC.-.LCF0@ha
	addi 2,2,.TOC.-.LCF0@l
	.localentry	foo,.-foo
	mflr 3
	std 3,16(1)
	std 31,-8(1)
	stdu 1,-112(1)
	mr 31,1
	bl bar
	nop
	mr 9,3
	mr 3,9
	addi 1,31,112
	ld 0,16(1)
	mtlr 0
	ld 31,-8(1)
	blr
	.long 0
	.byte 0,0,0,1,128,1,0,1
	.size	foo,.-foo
	.ident	"GCC: (SUSE Linux) 7.2.1 20170901 [gcc-7-branch revision 251580]"
	.section	.note.GNU-stack,"",@progbits

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Nikola Prica]

Hello Pedro and Kevi,

I've created and tested example based on your example. Thank you for that.

 > - I think it's more clear to only set lr_register when needed (pc
 > reaches the limit), as opposed to resetting it to -1 if pc didn't reach
 > the limit.

The body of condition that becomes visitable after this patch 
invalidates lr_reg by setting it to -2 before reaching the limit.

       else if (lr_reg >= 0 &&
                /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */
                (((op & 0xffff0000) == (lr_reg | 0xf8010000)) ||
                 /* stw Rx, NUM(r1) */
                 ((op & 0xffff0000) == (lr_reg | 0x90010000)) ||
                 /* stwu Rx, NUM(r1) */
                 ((op & 0xffff0000) == (lr_reg | 0x94010000))))
         {       /* where Rx == lr */
           fdata->lr_offset = offset;
           fdata->nosavedpc = 0;
           /* Invalidate lr_reg, but don't set it to -1.
              That would mean that it had never been set.  */
           lr_reg = -2;
	  ...

Thanks,

Nikola

 From 9aaddf9670d9f4cb7f088499febd1fa9c6a7076c Mon Sep 17 00:00:00 2001
From: Prica <nprica@rt-rk.com>
Date: Tue, 19 Dec 2017 14:29:09 +0100
Subject: [PATCH] Fix for prologue processing on PowerPc

One of conditions in skip_prologue() is never visited because it
expects non shifted `lr_reg`.  That condtition is supposed to set PC
offset.  When body of this condition is visited PC offset is set and
there will be no need to look for it in next frames nor to use frame
unwind directives.

gdb/ChangeLog:
   *rs600-tdep.c (skip_prologue): Remove shifting for lr_reg and assign
    shifted lr_reg to fdata->lr_register when lr_reg is set.  If
    iteration do not hit lim_pc lr_register is set as -1.
   *testsuite/gdb.arch/ppc-prologue-frame.s: New file.
   *testsuite/gdb.arch/ppc-prologue-frame.c: Likewise.
   *testsuite/gdb.arch/ppr-prologue-frame.exp: Likewise.
---
  gdb/rs6000-tdep.c                                 | 14 ++++---
  gdb/testsuite/gdb.arch/powerpc-prologue-frame.c   | 28 +++++++++++++
  gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 48 
+++++++++++++++++++++++
  gdb/testsuite/gdb.arch/powerpc-prologue-frame.s   | 40 +++++++++++++++++++
  4 files changed, 125 insertions(+), 5 deletions(-)
  create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
  create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
  create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.s

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 456dbcc..f0d2781 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1655,9 +1655,13 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR 
pc, CORE_ADDR lim_pc,
  	     remember just the first one, but skip over additional
  	     ones.  */
  	  if (lr_reg == -1)
-	    lr_reg = (op & 0x03e00000) >> 21;
-          if (lr_reg == 0)
-            r0_contains_arg = 0;
+      {
+        lr_reg = (op & 0x03e00000);
+        fdata->lr_register = lr_reg >> 21;
+      }
+    if (lr_reg == 0)
+      r0_contains_arg = 0;
+
  	  continue;
  	}
        else if ((op & 0xfc1fffff) == 0x7c000026)
@@ -2180,8 +2184,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR 
pc, CORE_ADDR lim_pc,
      }
  #endif /* 0 */

-  if (pc == lim_pc && lr_reg >= 0)
-    fdata->lr_register = lr_reg;
+  if (pc != lim_pc)
+    fdata->lr_register = -1;

    fdata->offset = -fdata->offset;
    return last_prologue_pc;
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c 
b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
new file mode 100644
index 0000000..f59210a
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
@@ -0,0 +1,28 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2017 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see 
<http://www.gnu.org/licenses/>.  */
+
+int bar()
+{
+  return 0;
+}
+
+int foo();
+
+int main(void)
+{
+  return foo();
+}
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp 
b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
new file mode 100644
index 0000000..e90a8c1
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
@@ -0,0 +1,48 @@
+# Copyright 2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>
+
+if {![istarget "powerpc*"] } {
+    verbose "Skipping powerpc back trace test."
+    return
+}
+
+
+set main_testfile ppc-prolog-frame
+
+if {![istarget "powerpc*-*-*"]} then {
+    verbose "Skipping PowerPC instructions disassembly."
+    return -1
+}
+
+
+if {[gdb_compile \
+      [list ${srcdir}/${subdir}/$main_testfile.c 
${srcdir}/${subdir}/$main_testfile.S] \
+      [standard_output_file ${main_testfile}] \
+      executable {debug}] != ""} {
+    untested "failed to build $main_testfile"
+    return -1
+}
+
+
+clean_restart ${main_testfile}
+
+if ![runto bar] {
+  untested "could not run to bar"
+  return -1
+}
+
+gdb_test "bt" \
+         "#0 \[x0-9a-f\]* bar \\(\\) at .*#1 \[x0-9a-f in\]* foo \\(\\) 
at .*#2 \[x0-9a-f in\]* main \\(\\) at .*" \
+         "Backtrace to the main frame"
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s 
b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s
new file mode 100644
index 0000000..16cd7e2
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s
@@ -0,0 +1,40 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2017 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see 
<http://www.gnu.org/licenses/>.  */
+
+	.file	"foo.c"
+	.section	".text"
+	.align 2
+	.globl foo
+	.type	foo, @function
+foo:
+	stwu 1,-32(1)
+	mflr 3
+	stw 3,36(1)
+	stw 31,28(1)
+	mr 31,1
+	bl bar
+	mr 9,3
+	mr 3,9
+	addi 11,31,32
+	lwz 0,4(11)
+	mtlr 0
+	lwz 31,-4(11)
+	mr 1,11
+	blr
+	.size	foo,.-foo
+	.ident	"GCC: (Ubuntu 4.8.2-19ubuntu1) 4.8.2"
+	.section	.note.GNU-stack,"",@progbits
-- 


On 01.12.2017. 20:36, pedromfc wrote:
> Hello Nikola, Kevin,
> 
> Thank you for providing these patches.
> 
> I tested both patches (Nicola's second patch and Kevin's patch) on 
> ppc64le, and there were no regressions, except for some additional 
> expected failures in gdb.threads/attach-many-short-lived-threads.exp.
> 
> Some comments:
> 
> - Nikola's patch moves "if(lr_reg == 0)/r0_contains_arg = 0;" to within 
> the first if. This is useful for the case when a second mflr Rx with Rx 
> != 0 is detected. Previously this would cause r0_contains_arg to be 
> reset, despite Rx not being 0. However, if the second mflr has Rx == 0, 
> it would make sense to reset r0_contains_arg (and this would work 
> accidentally before the patch, assuming the first mflr also had Rx == 
> 0). Maybe the best solution here is to always check the Rx contained in 
> the opcode and clear r0_contains_arg if it is 0, regardless of lr_reg, 
> or leave it as before since it's a separate issue.
> 
> - Kevin's patch assigns lr_reg = op & 0x03e00000, but lr_reg is an int, 
> and op is an unsigned long. Will the unshifted reg always fit in a int?
> 
> - I think it's more clear to only set lr_register when needed (pc 
> reaches the limit), as opposed to resetting it to -1 if pc didn't reach 
> the limit.
> 
> - I wasn't able to directly apply Nikola's patch, I did so manually.
> 
> I've also attached a reproducer (prologue.c/foo.S) to check if the 
> patches fixed the issue. I had to alter a generated assembly file so 
> that mflr would use a register other than R0 (in which case the old code 
> does work).
> 
> gcc -g0 -O0 -o prologue prologue.c foo.S
> 
> (gdb) file prologue
> Reading symbols from prologue...done.
> (gdb) break bar
> Breakpoint 1 at 0x100005b8
> (gdb) run
> 
> Starting program: /home/pedromfc/prologue
> 
> Before the patch:
> 
> Breakpoint 1, 0x00000000100005b8 in bar ()
> (gdb) bt
> #0  0x00000000100005b8 in bar ()
> #1  0x0000000010000644 in foo ()
> Backtrace stopped: frame did not save the PC
> 
> After (both patches):
> 
> Breakpoint 1, 0x00000000100005b8 in bar ()
> (gdb) bt
> #0  0x00000000100005b8 in bar ()
> #1  0x0000000010000644 in foo ()
> #2  0x00000000100005f8 in main ()
> 
> (gdb) info f 1
> Stack frame at 0x7fffffffeee0:
>   pc = 0x10000644 in foo; saved pc = 0x100005f8
> 
> Thanks!
> Pedro
> 
> On 11/09/2017 04:15 PM, Nikola Prica wrote:
>> Hi Kevin,
>>
>> lr_reg could be also set to -2 in part of code which is reachable 
>> after shifting removal.
>>
>>       /* Invalidate lr_reg, but don't set it to -1.
>>          That would mean that it had never been set.  */
>>       lr_reg = -2;
>>
>> This part of the code which depends of non shifted lr_reg, and the 
>> part where shifting is removed are only two places where lr_reg is 
>> changed. As so, I've added last condition to set fdata->lr_register on 
>> -1 if lim_pc is not reached.
>>
>> If it seems fine now could you pleas commit it because I don't have 
>> rights to do it.
>>
>> Thanks,
>>
>> Nikola Prica
>>
>>
>> From: Prica <nprica@rt-rk.com>
>> Date: Thu, 9 Nov 2017 13:10:48 +0100
>> Subject: Fix for prologue processing on PowerPC
>>
>> One of conditions in skip_prologue() is never visited because it
>> expects non shifted `lr_reg`.  That condition is supposed to set PC
>> offset.  When body of this condition is visited PC offset is set and
>> there will be no need to look for it in next frames nor to use frame
>> unwind directives.
>>
>> gdb/ChangeLog:
>>
>>     *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg
>>       and assign shifted lr_reg to fdata->lr_register when lr_reg is
>>       set. If iteration do not hit lim_pc lr_register is set as -1.
>> ---
>>  gdb/rs6000-tdep.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
>> index 6c44995..6f05ef5 100644
>> --- a/gdb/rs6000-tdep.c
>> +++ b/gdb/rs6000-tdep.c
>> @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, 
>> CORE_ADDR pc, CORE_ADDR lim_pc,
>>           remember just the first one, but skip over additional
>>           ones.  */
>>        if (lr_reg == -1)
>> -        lr_reg = (op & 0x03e00000) >> 21;
>> -          if (lr_reg == 0)
>> -            r0_contains_arg = 0;
>> +      {
>> +        lr_reg = (op & 0x03e00000);
>> +        fdata->lr_register = lr_reg >> 21;
>> +        if (lr_reg == 0)
>> +          r0_contains_arg = 0;
>> +      }
>>        continue;
>>      }
>>        else if ((op & 0xfc1fffff) == 0x7c000026)
>> @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, 
>> CORE_ADDR pc, CORE_ADDR lim_pc,
>>      }
>>  #endif /* 0 */
>>
>> -  if (pc == lim_pc && lr_reg >= 0)
>> -    fdata->lr_register = lr_reg;
>> +  if (pc != lim_pc)
>> +    fdata->lr_register = -1;
>>
>>    fdata->offset = -fdata->offset;
>>    return last_prologue_pc;
> 

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Pedro Franco de Carvalho]

Hello Nikola,

>  > - I think it's more clear to only set lr_register when needed (pc
>  > reaches the limit), as opposed to resetting it to -1 if pc didn't reach
>  > the limit.
>
> The body of condition that becomes visitable after this patch 
> invalidates lr_reg by setting it to -2 before reaching the limit.

Sorry, I was referring to to fdata->lr_register. What I meant is that it
seems better to me to only set fdata->lr_register to lr_reg >> 21 at the end
of the function (as Kevin had suggested), as opposed to setting it to -1
at the end of the function, in this part:

> +  if (pc != lim_pc)
> +    fdata->lr_register = -1;

In any case, for this part to be correct, shouldn't the condition be the
full complement of the original condition?

That is,

if (pc != lim_pc || lr_reg < 0)

Which is the complement of:

if (pc == lim_pc && lr_reg >= 0)

Instead of:

> +  if (pc != lim_pc)

Previously if there was a mflr/stw sequence that invalidated lr_reg,
fdata->lr_register would be -1 when the function exited, even if pc ==
lim_pc (because of the second term of the condition).

Still, I think it's safer to keep fdata->lr_register == -1 and set it at
the end if needed.

I wasn't able to apply your patch directly, I think there are some
extra whitespaces around there.

And thank you for providing a testcase. There's just a few comments:

> +if {![istarget "powerpc*"] } {
> +    verbose "Skipping powerpc back trace test."
> +    return
> +}
> +
> +
> +set main_testfile ppc-prolog-frame
> +
> +if {![istarget "powerpc*-*-*"]} then {
> +    verbose "Skipping PowerPC instructions disassembly."
> +    return -1
> +}

There is an extra target check here. Also, because the assembly file
uses the 32-bit abi I think the target selection should be: "istarget
"powerpc-*-*"", this way it excludes 64-bit targets.

The main_testfile name doesn't matche the test file name. In any case, you can use the
standard_testfile macro like this:

standard_testfile .c .S

It will set $srcfile and $srcfile2 to powerpc-prologue-frame.c/.S,
respectively.

> ${srcdir}/${subdir}/$main_testfile.S] \
> +      [standard_output_file ${main_testfile}] \
> +      executable {debug}] != ""} {

The option flag should be empty ({}), because we want to test the case
without debug information.

I tried the test and the regular expression didn't seem to match. This
is probably because of the "at" parts:

> +gdb_test "bt" \
> +         "#0 \[x0-9a-f\]* bar \\(\\) at .*#1 \[x0-9a-f in\]* foo \\(\\) 
> at .*#2 \[x0-9a-f in\]* main \\(\\) at .*" \

Does your gdb output "at" instead of in? I tried a recent version and it outputs "in".

I adapted one from the powerpc-prologue.exp test test that seemed to work:

gdb_test "bt" \
    "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \
    "Backtrace to the main frame"

It might be good to also remove unecessary info from the assembly file
for inclusion in the testcase, e.g. the .ident directive. I'm not sure
of what are the minimum necessary directives according to the abi, but
looking at another test, powerpc-stackless.exp, the corresponding
assembly source reads:

#include <ppc-asm.h>

FUNC_START(main)
        li      sp,0
        mtlr    sp
        blr
FUNC_END(main)

These macros could be useful here (in this case the extension should be
.S so that gcc will preprocess the file).

See below for a diff that incorporate these comments. Feel free to
include the changes in your patch.

Thanks,
Pedro

diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
new file mode 100644
index 0000000000..8697213934
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
@@ -0,0 +1,35 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2017 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <ppc-asm.h>
+
+FUNC_START(foo)
+	stwu 1,-32(1)
+	mflr 3
+	stw 3,36(1)
+	stw 31,28(1)
+	mr 31,1
+	bl bar
+	mr 9,3
+	mr 3,9
+	addi 11,31,32
+	lwz 0,4(11)
+	mtlr 0
+	lwz 31,-4(11)
+	mr 1,11
+	blr
+FUNC_END(foo)
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
new file mode 100644
index 0000000000..7a743aff8e
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
@@ -0,0 +1,31 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2017 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+bar (void)
+{
+  return 0;
+}
+
+int
+foo (void);
+
+int
+main (void)
+{
+  return foo ();
+}
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
new file mode 100644
index 0000000000..f124717169
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
@@ -0,0 +1,43 @@
+# Copyright 2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>
+
+# Test the PowerPC prologue analyzer when a register rx other than r0 is used
+# to save the link register in the mflr rx/stw rx sequence.
+
+if {![istarget "powerpc*-*-*"]} then {
+    verbose "Skipping PowerPC prologue tests."
+    return
+}
+
+standard_testfile .c .S
+
+set binfile [standard_output_file ${testfile}]
+
+if {[gdb_compile [list "${srcdir}/${subdir}/${srcfile}" "${srcdir}/${subdir}/${srcfile2}"] \
+	 "${binfile}" executable {}] != ""} {
+    untested "Failed to build ${binfile}"
+    return -1
+}
+
+clean_restart ${binfile}
+
+if ![runto bar] {
+  untested "could not run to bar"
+  return -1
+}
+
+gdb_test "bt" \
+    "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \
+    "Backtrace to the main frame"

Nikola Prica <nikola.prica@rt-rk.com> writes:

> Hello Pedro and Kevi,
>
> I've created and tested example based on your example. Thank you for that.
>
>  > - I think it's more clear to only set lr_register when needed (pc
>  > reaches the limit), as opposed to resetting it to -1 if pc didn't reach
>  > the limit.
>
> The body of condition that becomes visitable after this patch 
> invalidates lr_reg by setting it to -2 before reaching the limit.
>
>        else if (lr_reg >= 0 &&
>                 /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */
>                 (((op & 0xffff0000) == (lr_reg | 0xf8010000)) ||
>                  /* stw Rx, NUM(r1) */
>                  ((op & 0xffff0000) == (lr_reg | 0x90010000)) ||
>                  /* stwu Rx, NUM(r1) */
>                  ((op & 0xffff0000) == (lr_reg | 0x94010000))))
>          {       /* where Rx == lr */
>            fdata->lr_offset = offset;
>            fdata->nosavedpc = 0;
>            /* Invalidate lr_reg, but don't set it to -1.
>               That would mean that it had never been set.  */
>            lr_reg = -2;
> 	  ...
>
> Thanks,
>
> Nikola
>
>  From 9aaddf9670d9f4cb7f088499febd1fa9c6a7076c Mon Sep 17 00:00:00 2001
> From: Prica <nprica@rt-rk.com>
> Date: Tue, 19 Dec 2017 14:29:09 +0100
> Subject: [PATCH] Fix for prologue processing on PowerPc
>
> One of conditions in skip_prologue() is never visited because it
> expects non shifted `lr_reg`.  That condtition is supposed to set PC
> offset.  When body of this condition is visited PC offset is set and
> there will be no need to look for it in next frames nor to use frame
> unwind directives.
>
> gdb/ChangeLog:
>    *rs600-tdep.c (skip_prologue): Remove shifting for lr_reg and assign
>     shifted lr_reg to fdata->lr_register when lr_reg is set.  If
>     iteration do not hit lim_pc lr_register is set as -1.
>    *testsuite/gdb.arch/ppc-prologue-frame.s: New file.
>    *testsuite/gdb.arch/ppc-prologue-frame.c: Likewise.
>    *testsuite/gdb.arch/ppr-prologue-frame.exp: Likewise.
> ---
>   gdb/rs6000-tdep.c                                 | 14 ++++---
>   gdb/testsuite/gdb.arch/powerpc-prologue-frame.c   | 28 +++++++++++++
>   gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 48 
> +++++++++++++++++++++++
>   gdb/testsuite/gdb.arch/powerpc-prologue-frame.s   | 40 +++++++++++++++++++
>   4 files changed, 125 insertions(+), 5 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
>   create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
>   create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.s
>
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 456dbcc..f0d2781 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1655,9 +1655,13 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR 
> pc, CORE_ADDR lim_pc,
>   	     remember just the first one, but skip over additional
>   	     ones.  */
>   	  if (lr_reg == -1)
> -	    lr_reg = (op & 0x03e00000) >> 21;
> -          if (lr_reg == 0)
> -            r0_contains_arg = 0;
> +      {
> +        lr_reg = (op & 0x03e00000);
> +        fdata->lr_register = lr_reg >> 21;
> +      }
> +    if (lr_reg == 0)
> +      r0_contains_arg = 0;
> +
>   	  continue;
>   	}
>         else if ((op & 0xfc1fffff) == 0x7c000026)
> @@ -2180,8 +2184,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR 
> pc, CORE_ADDR lim_pc,
>       }
>   #endif /* 0 */
>
> -  if (pc == lim_pc && lr_reg >= 0)
> -    fdata->lr_register = lr_reg;
> +  if (pc != lim_pc)
> +    fdata->lr_register = -1;
>
>     fdata->offset = -fdata->offset;
>     return last_prologue_pc;
> diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c 
> b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
> new file mode 100644
> index 0000000..f59210a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
> @@ -0,0 +1,28 @@
> +/* This test is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.  */
> +
> +int bar()
> +{
> +  return 0;
> +}
> +
> +int foo();
> +
> +int main(void)
> +{
> +  return foo();
> +}
> diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp 
> b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
> new file mode 100644
> index 0000000..e90a8c1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
> @@ -0,0 +1,48 @@
> +# Copyright 2017 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>
> +
> +if {![istarget "powerpc*"] } {
> +    verbose "Skipping powerpc back trace test."
> +    return
> +}
> +
> +
> +set main_testfile ppc-prolog-frame
> +
> +if {![istarget "powerpc*-*-*"]} then {
> +    verbose "Skipping PowerPC instructions disassembly."
> +    return -1
> +}
> +
> +
> +if {[gdb_compile \
> +      [list ${srcdir}/${subdir}/$main_testfile.c 
> ${srcdir}/${subdir}/$main_testfile.S] \
> +      [standard_output_file ${main_testfile}] \
> +      executable {debug}] != ""} {
> +    untested "failed to build $main_testfile"
> +    return -1
> +}
> +
> +
> +clean_restart ${main_testfile}
> +
> +if ![runto bar] {
> +  untested "could not run to bar"
> +  return -1
> +}
> +
> +gdb_test "bt" \
> +         "#0 \[x0-9a-f\]* bar \\(\\) at .*#1 \[x0-9a-f in\]* foo \\(\\) 
> at .*#2 \[x0-9a-f in\]* main \\(\\) at .*" \
> +         "Backtrace to the main frame"
> diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s 
> b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s
> new file mode 100644
> index 0000000..16cd7e2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.s
> @@ -0,0 +1,40 @@
> +/* This test is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.  */
> +
> +	.file	"foo.c"
> +	.section	".text"
> +	.align 2
> +	.globl foo
> +	.type	foo, @function
> +foo:
> +	stwu 1,-32(1)
> +	mflr 3
> +	stw 3,36(1)
> +	stw 31,28(1)
> +	mr 31,1
> +	bl bar
> +	mr 9,3
> +	mr 3,9
> +	addi 11,31,32
> +	lwz 0,4(11)
> +	mtlr 0
> +	lwz 31,-4(11)
> +	mr 1,11
> +	blr
> +	.size	foo,.-foo
> +	.ident	"GCC: (Ubuntu 4.8.2-19ubuntu1) 4.8.2"
> +	.section	.note.GNU-stack,"",@progbits
> -- 
>
>
> On 01.12.2017. 20:36, pedromfc wrote:
>> Hello Nikola, Kevin,
>> 
>> Thank you for providing these patches.
>> 
>> I tested both patches (Nicola's second patch and Kevin's patch) on 
>> ppc64le, and there were no regressions, except for some additional 
>> expected failures in gdb.threads/attach-many-short-lived-threads.exp.
>> 
>> Some comments:
>> 
>> - Nikola's patch moves "if(lr_reg == 0)/r0_contains_arg = 0;" to within 
>> the first if. This is useful for the case when a second mflr Rx with Rx 
>> != 0 is detected. Previously this would cause r0_contains_arg to be 
>> reset, despite Rx not being 0. However, if the second mflr has Rx == 0, 
>> it would make sense to reset r0_contains_arg (and this would work 
>> accidentally before the patch, assuming the first mflr also had Rx == 
>> 0). Maybe the best solution here is to always check the Rx contained in 
>> the opcode and clear r0_contains_arg if it is 0, regardless of lr_reg, 
>> or leave it as before since it's a separate issue.
>> 
>> - Kevin's patch assigns lr_reg = op & 0x03e00000, but lr_reg is an int, 
>> and op is an unsigned long. Will the unshifted reg always fit in a int?
>> 
>> - I think it's more clear to only set lr_register when needed (pc 
>> reaches the limit), as opposed to resetting it to -1 if pc didn't reach 
>> the limit.
>> 
>> - I wasn't able to directly apply Nikola's patch, I did so manually.
>> 
>> I've also attached a reproducer (prologue.c/foo.S) to check if the 
>> patches fixed the issue. I had to alter a generated assembly file so 
>> that mflr would use a register other than R0 (in which case the old code 
>> does work).
>> 
>> gcc -g0 -O0 -o prologue prologue.c foo.S
>> 
>> (gdb) file prologue
>> Reading symbols from prologue...done.
>> (gdb) break bar
>> Breakpoint 1 at 0x100005b8
>> (gdb) run
>> 
>> Starting program: /home/pedromfc/prologue
>> 
>> Before the patch:
>> 
>> Breakpoint 1, 0x00000000100005b8 in bar ()
>> (gdb) bt
>> #0  0x00000000100005b8 in bar ()
>> #1  0x0000000010000644 in foo ()
>> Backtrace stopped: frame did not save the PC
>> 
>> After (both patches):
>> 
>> Breakpoint 1, 0x00000000100005b8 in bar ()
>> (gdb) bt
>> #0  0x00000000100005b8 in bar ()
>> #1  0x0000000010000644 in foo ()
>> #2  0x00000000100005f8 in main ()
>> 
>> (gdb) info f 1
>> Stack frame at 0x7fffffffeee0:
>>   pc = 0x10000644 in foo; saved pc = 0x100005f8
>> 
>> Thanks!
>> Pedro
>> 
>> On 11/09/2017 04:15 PM, Nikola Prica wrote:
>>> Hi Kevin,
>>>
>>> lr_reg could be also set to -2 in part of code which is reachable 
>>> after shifting removal.
>>>
>>>       /* Invalidate lr_reg, but don't set it to -1.
>>>          That would mean that it had never been set.  */
>>>       lr_reg = -2;
>>>
>>> This part of the code which depends of non shifted lr_reg, and the 
>>> part where shifting is removed are only two places where lr_reg is 
>>> changed. As so, I've added last condition to set fdata->lr_register on 
>>> -1 if lim_pc is not reached.
>>>
>>> If it seems fine now could you pleas commit it because I don't have 
>>> rights to do it.
>>>
>>> Thanks,
>>>
>>> Nikola Prica
>>>
>>>
>>> From: Prica <nprica@rt-rk.com>
>>> Date: Thu, 9 Nov 2017 13:10:48 +0100
>>> Subject: Fix for prologue processing on PowerPC
>>>
>>> One of conditions in skip_prologue() is never visited because it
>>> expects non shifted `lr_reg`.  That condition is supposed to set PC
>>> offset.  When body of this condition is visited PC offset is set and
>>> there will be no need to look for it in next frames nor to use frame
>>> unwind directives.
>>>
>>> gdb/ChangeLog:
>>>
>>>     *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg
>>>       and assign shifted lr_reg to fdata->lr_register when lr_reg is
>>>       set. If iteration do not hit lim_pc lr_register is set as -1.
>>> ---
>>>  gdb/rs6000-tdep.c | 13 ++++++++-----
>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
>>> index 6c44995..6f05ef5 100644
>>> --- a/gdb/rs6000-tdep.c
>>> +++ b/gdb/rs6000-tdep.c
>>> @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, 
>>> CORE_ADDR pc, CORE_ADDR lim_pc,
>>>           remember just the first one, but skip over additional
>>>           ones.  */
>>>        if (lr_reg == -1)
>>> -        lr_reg = (op & 0x03e00000) >> 21;
>>> -          if (lr_reg == 0)
>>> -            r0_contains_arg = 0;
>>> +      {
>>> +        lr_reg = (op & 0x03e00000);
>>> +        fdata->lr_register = lr_reg >> 21;
>>> +        if (lr_reg == 0)
>>> +          r0_contains_arg = 0;
>>> +      }
>>>        continue;
>>>      }
>>>        else if ((op & 0xfc1fffff) == 0x7c000026)
>>> @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, 
>>> CORE_ADDR pc, CORE_ADDR lim_pc,
>>>      }
>>>  #endif /* 0 */
>>>
>>> -  if (pc == lim_pc && lr_reg >= 0)
>>> -    fdata->lr_register = lr_reg;
>>> +  if (pc != lim_pc)
>>> +    fdata->lr_register = -1;
>>>
>>>    fdata->offset = -fdata->offset;
>>>    return last_prologue_pc;
>> 

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Nikola Prica]

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

Hiello Pedro,

>>  > - I think it's more clear to only set lr_register when needed (pc
>>  > reaches the limit), as opposed to resetting it to -1 if pc didn't reach
>>  > the limit.
>>
>> The body of condition that becomes visitable after this patch 
>> invalidates lr_reg by setting it to -2 before reaching the limit.
> 
> Sorry, I was referring to to fdata->lr_register. What I meant is that it
> seems better to me to only set fdata->lr_register to lr_reg >> 21 at the end
> of the function (as Kevin had suggested), as opposed to setting it to -1
> at the end of the function, in this part:
> 

I didn't explain it right. The catch is that fdata->lr_register is set
by shifting lr_reg variable which is set only once and it can be
invalidated druing the loop iteration. For example, lets say that
firstly lr_reg is set to some value, then in some further loop iteration
the next code gets visited:

          else if (lr_reg >= 0 &&
             /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */
             (((op & 0xffff0000) == (lr_reg | 0xf8010000)) ||
        /* stw Rx, NUM(r1) */
        ((op & 0xffff0000) == (lr_reg | 0x90010000)) ||
        /* stwu Rx, NUM(r1) */
        ((op & 0xffff0000) == (lr_reg | 0x94010000))))
      { /* where Rx == lr */
        fdata->lr_offset = offset;
        fdata->nosavedpc = 0;
        /* Invalidate lr_reg, but don't set it to -1.
           That would mean that it had never been set.  */
        lr_reg = -2;

This part of code was previously never visited because of lr_reg
shifting. The last line in upper code overwrites value of lr_reg. If in
the furtherer iteration lim_pc would be reached, -2 value would be
shifted and set to fdata->lr_register and that is not what we want.


>
> Previously if there was a mflr/stw sequence that invalidated lr_reg,
> fdata->lr_register would be -1 when the function exited, even if pc ==
> lim_pc (because of the second term of the condition).
>


Previously if there was s mflr/stw sequence lr_reg would never be
invalidated (set to -2) because part of the code that does this
invalidation was never visitable due to shifting of lr_reg. Invalidation
is performed so that the upper code never gets visited again in same
iteration. lr_reg is set only once and it's value is used only in the
condition of upper part of the code. In other conditions it is only
checked whether it is set (different than -1). The comment where lr_reg
is changed from -1 to some value says the following:

"remember just the first one, but skip over additional ones."

Which means that we need to deal only with one value of lr_reg. And the
fdata->lr_register basically saves the register number that we get by
shifting lr_reg.

> 
> In any case, for this part to be correct, shouldn't the condition be the
> full complement of the original condition?
> 
> That is,
> 
> if (pc != lim_pc || lr_reg < 0)
> 
> Which is the complement of:
> 
> if (pc == lim_pc && lr_reg >= 0)
> 
> Instead of:
> 
>> +  if (pc != lim_pc)


I've changed this condition to following:

   if (pc != lim_pc && lr_reg != -1)

This is more appropriate condition because this condition requires
change of lr_reg.


> Still, I think it's safer to keep fdata->lr_register == -1 and set it at
> the end if needed.
> 


This could be achieved by using one more variable that would be backup
for lr_reg in case it is set to -2. But I think that this approach with
setting it to -1 is more appropriate than that?

Thanks,

Nikola Prica

[-- Attachment #2: 0001-PATCH-Fix-for-prologue-processing-on-PowerPc.patch --]
[-- Type: text/x-patch, Size: 6391 bytes --]

From d93689d414137926818429fe0910ad9a9e6ef0dd Mon Sep 17 00:00:00 2001
From: Prica <nprica@rt-rk.com>
Date: Tue, 19 Dec 2017 14:29:09 +0100
Subject: [PATCH] [PATCH] Fix for prologue processing on PowerPc

One of conditions in skip_prologue() is never visited because it
expects non shifted `lr_reg`.  That condtition is supposed to set PC
offset.  When body of this condition is visited PC offset is set and
there will be no need to look for it in next frames nor to use frame
unwind directives.

gdb/ChangeLog:
  *rs600-tdep.c (skip_prologue): Remove shifting for lr_reg and assign
   shifted lr_reg to fdata->lr_register when lr_reg is set.  If
   iteration do not hit lim_pc lr_register is set as -1.
  *testsuite/gdb.arch/ppc-prologue-frame.s: New file.
  *testsuite/gdb.arch/ppc-prologue-frame.c: Likewise.
  *testsuite/gdb.arch/ppr-prologue-frame.exp: Likewise.
---
 gdb/rs6000-tdep.c                                 | 14 ++++---
 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S   | 35 +++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c   | 28 ++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 47 +++++++++++++++++++++++
 4 files changed, 119 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 456dbcc..7bf4f60 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1655,9 +1655,13 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
 	     remember just the first one, but skip over additional
 	     ones.  */
 	  if (lr_reg == -1)
-	    lr_reg = (op & 0x03e00000) >> 21;
-          if (lr_reg == 0)
-            r0_contains_arg = 0;
+      {
+        lr_reg = (op & 0x03e00000);
+        fdata->lr_register = lr_reg >> 21;
+      }
+    if (lr_reg == 0)
+      r0_contains_arg = 0;
+
 	  continue;
 	}
       else if ((op & 0xfc1fffff) == 0x7c000026)
@@ -2180,8 +2184,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
     }
 #endif /* 0 */
 
-  if (pc == lim_pc && lr_reg >= 0)
-    fdata->lr_register = lr_reg;
+  if (pc != lim_pc && lr_reg != -1)
+    fdata->lr_register = -1;
 
   fdata->offset = -fdata->offset;
   return last_prologue_pc;
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
new file mode 100644
index 0000000..e30ca23
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
@@ -0,0 +1,35 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <ppc-asm.h>
+
+FUNC_START(foo)
+	stwu 1,-32(1)
+	mflr 3
+	stw 3,36(1)
+	stw 31,28(1)
+	mr 31,1
+	bl bar
+	mr 9,3
+	mr 3,9
+	addi 11,31,32
+	lwz 0,4(11)
+	mtlr 0
+	lwz 31,-4(11)
+	mr 1,11
+	blr
+FUNC_END(foo)
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
new file mode 100644
index 0000000..8cab6f2
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
@@ -0,0 +1,28 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int bar()
+{
+  return 0;
+}
+
+int foo();
+
+int main(void)
+{
+  return foo();
+}
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
new file mode 100644
index 0000000..4f988db
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
@@ -0,0 +1,47 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>
+
+if {![istarget "powerpc*"] } {
+    verbose "Skipping powerpc back trace test."
+    return
+}
+
+standard_testfile .c .S
+set binfile [standard_output_file ${testfile}]
+
+if {![istarget "powerpc-*-*"]} then {
+    verbose "Skipping PowerPC instructions disassembly."
+    return -1
+}
+
+
+if {[gdb_compile \
+      [list ${srcdir}/${subdir}/$srcfile ${srcdir}/${subdir}/$srcfile2] \
+      "${binfile}" executable {}] != ""} {
+    untested "failed to build $binfile"
+    return -1
+}
+
+
+clean_restart ${binfile}
+
+if ![runto bar] {
+  untested "could not run to bar"
+  return -1
+}
+
+gdb_test "bt" \
+         "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \
+         "Backtrace to the main frame"
-- 
2.7.4

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Pedro Franco de Carvalho]

Hello Nikola,

> This part of code was previously never visited because of lr_reg
> shifting. The last line in upper code overwrites value of lr_reg. If in
> the furtherer iteration lim_pc would be reached, -2 value would be
> shifted and set to fdata->lr_register and that is not what we want.

It could be visited when lr_reg == 0 (because in this case lr_reg >>
21 == lr_reg). I suspect this is a common case, when I used gcc
it usually generated the mflr/stw sequence using r0.

In the old code, even if the limit was reached, the fdata->lr_register
variable would not be set to lr_reg if lr_reg was previously invalidated
(set to -2), assuming mflr/stw used r0.

You are correct that previously, for any other register than r0, the
block that detects the "stw" part of the sequence would never be
reached, which would cause fdata->lr_register to be set, if lim_pc was
reached, even if LR was saved in the stack by the stw.

However, it seems to me that this was unintentional, and that the
original intention was to set fdata->lr_register only if
it was valid and not yet saved in the stack when skip_prologue was
called.

So I think it's safer to preserve this behavior, that is,
fdata->lr_register should be -1 at function exit if either the limit
wasn't reached or lr_reg was saved to the stack or never seen (which
would be reflected by -2 or -1, respectively).

Note that the comment in the definition of struct rs6000_framedata
specifices "if trustworthy".

    int lr_register;		/* register of saved lr, if trustworthy */

Here is an example. Take the sequence:

foo:
    stwu 1,-32(1)
    mflr 0
    nop
    ...

If you break at the nop, and run backtrace, skip_prologue will be called
to build the frame_cache and figure out where the link register was
saved so that the program counter from the previous stack frame can be
found.

In this case, skip_prologue will be called with lim_pc pointing to the
nop (which is where we are). Because r0 is not saved back to the
stack, and because in this case pc reaches lim_pc, fdata->lr_register is valid
and should hold lr_reg.

Take this other sequence:

foo:
    stwu 1,-32(1)
    mflr 0
    stw 0,36(1)
    li 0,-1
    nop

If we break at the nop, and call backtrace, LR will already have been
saved in the stack, so fdata->lr_offset should be valid, but not
fdata->lr_register. Note that it was overwritten by the li instruction,
so r0 can no longer be trusted to hold the value of LR.

I suspect the callers of skip_prologue will use lr_offset over
lr_register if both are set, but I think it's better to be conservative
and ensure fdata is valid.

>> Still, I think it's safer to keep fdata->lr_register == -1 and set it at
>> the end if needed.
>> 
>
>
> This could be achieved by using one more variable that would be backup
> for lr_reg in case it is set to -2. But I think that this approach with
> setting it to -1 is more appropriate than that?

It could be done without an additional variable by shifting lr_reg and
assigning the result to lr_register only at the end, like in
Kevin's suggestion.

I'm just not sure that the bitwise operations with
lr_reg are safe considering it is an int (e.g., is lr_reg | 0x90010000
well-defined?).

Thanks!
Pedro

Nikola Prica <nikola.prica@rt-rk.com> writes:

> Hiello Pedro,
>
>>>  > - I think it's more clear to only set lr_register when needed (pc
>>>  > reaches the limit), as opposed to resetting it to -1 if pc didn't reach
>>>  > the limit.
>>>
>>> The body of condition that becomes visitable after this patch 
>>> invalidates lr_reg by setting it to -2 before reaching the limit.
>> 
>> Sorry, I was referring to to fdata->lr_register. What I meant is that it
>> seems better to me to only set fdata->lr_register to lr_reg >> 21 at the end
>> of the function (as Kevin had suggested), as opposed to setting it to -1
>> at the end of the function, in this part:
>> 
>
> I didn't explain it right. The catch is that fdata->lr_register is set
> by shifting lr_reg variable which is set only once and it can be
> invalidated druing the loop iteration. For example, lets say that
> firstly lr_reg is set to some value, then in some further loop iteration
> the next code gets visited:
>
>           else if (lr_reg >= 0 &&
>              /* std Rx, NUM(r1) || stdu Rx, NUM(r1) */
>              (((op & 0xffff0000) == (lr_reg | 0xf8010000)) ||
>         /* stw Rx, NUM(r1) */
>         ((op & 0xffff0000) == (lr_reg | 0x90010000)) ||
>         /* stwu Rx, NUM(r1) */
>         ((op & 0xffff0000) == (lr_reg | 0x94010000))))
>       { /* where Rx == lr */
>         fdata->lr_offset = offset;
>         fdata->nosavedpc = 0;
>         /* Invalidate lr_reg, but don't set it to -1.
>            That would mean that it had never been set.  */
>         lr_reg = -2;
>
> This part of code was previously never visited because of lr_reg
> shifting. The last line in upper code overwrites value of lr_reg. If in
> the furtherer iteration lim_pc would be reached, -2 value would be
> shifted and set to fdata->lr_register and that is not what we want.
>
>
>>
>> Previously if there was a mflr/stw sequence that invalidated lr_reg,
>> fdata->lr_register would be -1 when the function exited, even if pc ==
>> lim_pc (because of the second term of the condition).
>>
>
>
> Previously if there was s mflr/stw sequence lr_reg would never be
> invalidated (set to -2) because part of the code that does this
> invalidation was never visitable due to shifting of lr_reg. Invalidation
> is performed so that the upper code never gets visited again in same
> iteration. lr_reg is set only once and it's value is used only in the
> condition of upper part of the code. In other conditions it is only
> checked whether it is set (different than -1). The comment where lr_reg
> is changed from -1 to some value says the following:
>
> "remember just the first one, but skip over additional ones."
>
> Which means that we need to deal only with one value of lr_reg. And the
> fdata->lr_register basically saves the register number that we get by
> shifting lr_reg.
>
>> 
>> In any case, for this part to be correct, shouldn't the condition be the
>> full complement of the original condition?
>> 
>> That is,
>> 
>> if (pc != lim_pc || lr_reg < 0)
>> 
>> Which is the complement of:
>> 
>> if (pc == lim_pc && lr_reg >= 0)
>> 
>> Instead of:
>> 
>>> +  if (pc != lim_pc)
>
>
> I've changed this condition to following:
>
>    if (pc != lim_pc && lr_reg != -1)
>
> This is more appropriate condition because this condition requires
> change of lr_reg.
>
>
>> Still, I think it's safer to keep fdata->lr_register == -1 and set it at
>> the end if needed.
>> 
>
>
> This could be achieved by using one more variable that would be backup
> for lr_reg in case it is set to -2. But I think that this approach with
> setting it to -1 is more appropriate than that?
>
> Thanks,
>
> Nikola Prica
> From d93689d414137926818429fe0910ad9a9e6ef0dd Mon Sep 17 00:00:00 2001
> From: Prica <nprica@rt-rk.com>
> Date: Tue, 19 Dec 2017 14:29:09 +0100
> Subject: [PATCH] [PATCH] Fix for prologue processing on PowerPc
>
> One of conditions in skip_prologue() is never visited because it
> expects non shifted `lr_reg`.  That condtition is supposed to set PC
> offset.  When body of this condition is visited PC offset is set and
> there will be no need to look for it in next frames nor to use frame
> unwind directives.
>
> gdb/ChangeLog:
>   *rs600-tdep.c (skip_prologue): Remove shifting for lr_reg and assign
>    shifted lr_reg to fdata->lr_register when lr_reg is set.  If
>    iteration do not hit lim_pc lr_register is set as -1.
>   *testsuite/gdb.arch/ppc-prologue-frame.s: New file.
>   *testsuite/gdb.arch/ppc-prologue-frame.c: Likewise.
>   *testsuite/gdb.arch/ppr-prologue-frame.exp: Likewise.
> ---
>  gdb/rs6000-tdep.c                                 | 14 ++++---
>  gdb/testsuite/gdb.arch/powerpc-prologue-frame.S   | 35 +++++++++++++++++
>  gdb/testsuite/gdb.arch/powerpc-prologue-frame.c   | 28 ++++++++++++++
>  gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 47 +++++++++++++++++++++++
>  4 files changed, 119 insertions(+), 5 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
>
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 456dbcc..7bf4f60 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1655,9 +1655,13 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
>  	     remember just the first one, but skip over additional
>  	     ones.  */
>  	  if (lr_reg == -1)
> -	    lr_reg = (op & 0x03e00000) >> 21;
> -          if (lr_reg == 0)
> -            r0_contains_arg = 0;
> +      {
> +        lr_reg = (op & 0x03e00000);
> +        fdata->lr_register = lr_reg >> 21;
> +      }
> +    if (lr_reg == 0)
> +      r0_contains_arg = 0;
> +
>  	  continue;
>  	}
>        else if ((op & 0xfc1fffff) == 0x7c000026)
> @@ -2180,8 +2184,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
>      }
>  #endif /* 0 */
>
> -  if (pc == lim_pc && lr_reg >= 0)
> -    fdata->lr_register = lr_reg;
> +  if (pc != lim_pc && lr_reg != -1)
> +    fdata->lr_register = -1;
>
>    fdata->offset = -fdata->offset;
>    return last_prologue_pc;
> diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
> new file mode 100644
> index 0000000..e30ca23
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
> @@ -0,0 +1,35 @@
> +/* This test is part of GDB, the GNU debugger.
> +
> +   Copyright 2018 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <ppc-asm.h>
> +
> +FUNC_START(foo)
> +	stwu 1,-32(1)
> +	mflr 3
> +	stw 3,36(1)
> +	stw 31,28(1)
> +	mr 31,1
> +	bl bar
> +	mr 9,3
> +	mr 3,9
> +	addi 11,31,32
> +	lwz 0,4(11)
> +	mtlr 0
> +	lwz 31,-4(11)
> +	mr 1,11
> +	blr
> +FUNC_END(foo)
> diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
> new file mode 100644
> index 0000000..8cab6f2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
> @@ -0,0 +1,28 @@
> +/* This test is part of GDB, the GNU debugger.
> +
> +   Copyright 2018 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int bar()
> +{
> +  return 0;
> +}
> +
> +int foo();
> +
> +int main(void)
> +{
> +  return foo();
> +}
> diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
> new file mode 100644
> index 0000000..4f988db
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
> @@ -0,0 +1,47 @@
> +# Copyright 2018 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>
> +
> +if {![istarget "powerpc*"] } {
> +    verbose "Skipping powerpc back trace test."
> +    return
> +}
> +
> +standard_testfile .c .S
> +set binfile [standard_output_file ${testfile}]
> +
> +if {![istarget "powerpc-*-*"]} then {
> +    verbose "Skipping PowerPC instructions disassembly."
> +    return -1
> +}
> +
> +
> +if {[gdb_compile \
> +      [list ${srcdir}/${subdir}/$srcfile ${srcdir}/${subdir}/$srcfile2] \
> +      "${binfile}" executable {}] != ""} {
> +    untested "failed to build $binfile"
> +    return -1
> +}
> +
> +
> +clean_restart ${binfile}
> +
> +if ![runto bar] {
> +  untested "could not run to bar"
> +  return -1
> +}
> +
> +gdb_test "bt" \
> +         "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \
> +         "Backtrace to the main frame"
> -- 
> 2.7.4

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Nikola Prica]

Hi Pedro,

I understand it now. Thank you for explanation.

> I'm just not sure that the bitwise operations with
> lr_reg are safe considering it is an int (e.g., is lr_reg | 0x90010000
> well-defined?).
> 

Regarding that lr_reg is changed to valid value only when it is
previously set -1 and that it is changed like lr_reg = (op & 0x03e00000)
it means that it only takes bits between 21-26. I would say that it is
safe. I suppose that it is set to int for easier way of verifying its
validity.

Thanks,

Nikola Prica

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Nikola Prica]

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

Hi Pedro,

I've just seen that changed version of patch was not attached.

Best regards,

Nikola Prica

[-- Attachment #2: 0001-Fix-for-prologue-processing-on-PowerPC.patch --]
[-- Type: text/x-patch, Size: 1763 bytes --]

From 3564cd3be620bb39ca615919148fa0d949284d35 Mon Sep 17 00:00:00 2001
From: Prica <nprica@rt-rk.com>
Date: Thu, 9 Nov 2017 13:10:48 +0100
Subject: [PATCH] Fix for prologue processing on PowerPC

  One of conditions in skip_prologue() is never visited because it
  expects non shifted `lr_reg`. That condition is supposed to set PC
  offset. When body of this condition is visited PC offset is set and
  there will be no need to look for it in next frames nor to use frame
  unwind directives.

  gdb/ChangeLog:
  	*rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg
  	and assign shifted lr_reg to fdata->lr_register when lr_reg is
  	set. If iteration do not hit lim_pc lr_register is set as -1.
---
 gdb/rs6000-tdep.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 6c44995..6f05ef5 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
 	     remember just the first one, but skip over additional
 	     ones.  */
 	  if (lr_reg == -1)
-	    lr_reg = (op & 0x03e00000) >> 21;
-          if (lr_reg == 0)
-            r0_contains_arg = 0;
+      {
+        lr_reg = (op & 0x03e00000);
+        fdata->lr_register = lr_reg >> 21;
+        if (lr_reg == 0)
+          r0_contains_arg = 0;
+      }
 	  continue;
 	}
       else if ((op & 0xfc1fffff) == 0x7c000026)
@@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
     }
 #endif /* 0 */
 
-  if (pc == lim_pc && lr_reg >= 0)
-    fdata->lr_register = lr_reg;
+  if (pc != lim_pc)
+    fdata->lr_register = -1;
 
   fdata->offset = -fdata->offset;
   return last_prologue_pc;
-- 
2.7.4

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Pedro Franco de Carvalho]

Nikola Prica <nikola.prica@rt-rk.com> writes:

> Hi Pedro,
>
> I've just seen that changed version of patch was not attached.
>
> Best regards,
>
> Nikola Prica
> From 3564cd3be620bb39ca615919148fa0d949284d35 Mon Sep 17 00:00:00 2001
> From: Prica <nprica@rt-rk.com>
> Date: Thu, 9 Nov 2017 13:10:48 +0100
> Subject: [PATCH] Fix for prologue processing on PowerPC
>
>   One of conditions in skip_prologue() is never visited because it
>   expects non shifted `lr_reg`. That condition is supposed to set PC
>   offset. When body of this condition is visited PC offset is set and
>   there will be no need to look for it in next frames nor to use frame
>   unwind directives.
>
>   gdb/ChangeLog:
>   	*rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg
>   	and assign shifted lr_reg to fdata->lr_register when lr_reg is
>   	set. If iteration do not hit lim_pc lr_register is set as -1.
> ---
>  gdb/rs6000-tdep.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 6c44995..6f05ef5 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
>  	     remember just the first one, but skip over additional
>  	     ones.  */
>  	  if (lr_reg == -1)
> -	    lr_reg = (op & 0x03e00000) >> 21;
> -          if (lr_reg == 0)
> -            r0_contains_arg = 0;
> +      {
> +        lr_reg = (op & 0x03e00000);
> +        fdata->lr_register = lr_reg >> 21;
> +        if (lr_reg == 0)
> +          r0_contains_arg = 0;
> +      }
>  	  continue;
>  	}
>        else if ((op & 0xfc1fffff) == 0x7c000026)
> @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
>      }
>  #endif /* 0 */
>
> -  if (pc == lim_pc && lr_reg >= 0)
> -    fdata->lr_register = lr_reg;
> +  if (pc != lim_pc)
> +    fdata->lr_register = -1;
>
>    fdata->offset = -fdata->offset;
>    return last_prologue_pc;
> -- 
> 2.7.4

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Pedro Franco de Carvalho]

Hello Nikola,

Is that the version you meant to send? That looks like the
same as this one:

https://sourceware.org/ml/gdb-patches/2017-11/msg00207.html

Thanks,
Pedro

Nikola Prica <nikola.prica@rt-rk.com> writes:

> Hi Pedro,
>
> I've just seen that changed version of patch was not attached.
>
> Best regards,
>
> Nikola Prica
> From 3564cd3be620bb39ca615919148fa0d949284d35 Mon Sep 17 00:00:00 2001
> From: Prica <nprica@rt-rk.com>
> Date: Thu, 9 Nov 2017 13:10:48 +0100
> Subject: [PATCH] Fix for prologue processing on PowerPC
>
>   One of conditions in skip_prologue() is never visited because it
>   expects non shifted `lr_reg`. That condition is supposed to set PC
>   offset. When body of this condition is visited PC offset is set and
>   there will be no need to look for it in next frames nor to use frame
>   unwind directives.
>
>   gdb/ChangeLog:
>   	*rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg
>   	and assign shifted lr_reg to fdata->lr_register when lr_reg is
>   	set. If iteration do not hit lim_pc lr_register is set as -1.
> ---
>  gdb/rs6000-tdep.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 6c44995..6f05ef5 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1655,9 +1655,12 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
>  	     remember just the first one, but skip over additional
>  	     ones.  */
>  	  if (lr_reg == -1)
> -	    lr_reg = (op & 0x03e00000) >> 21;
> -          if (lr_reg == 0)
> -            r0_contains_arg = 0;
> +      {
> +        lr_reg = (op & 0x03e00000);
> +        fdata->lr_register = lr_reg >> 21;
> +        if (lr_reg == 0)
> +          r0_contains_arg = 0;
> +      }
>  	  continue;
>  	}
>        else if ((op & 0xfc1fffff) == 0x7c000026)
> @@ -2180,8 +2183,8 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
>      }
>  #endif /* 0 */
>
> -  if (pc == lim_pc && lr_reg >= 0)
> -    fdata->lr_register = lr_reg;
> +  if (pc != lim_pc)
> +    fdata->lr_register = -1;
>
>    fdata->offset = -fdata->offset;
>    return last_prologue_pc;
> -- 
> 2.7.4

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Nikola Prica]

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

Hi Pedro,

Thank you for your review and patience. I'm really grateful.

Can someone commit this patch?

Thanks,

Nikola

[-- Attachment #2: 0001-Fix-for-prologue-processing-on-PowerPc.patch --]
[-- Type: text/x-patch, Size: 6127 bytes --]

From 4b32b0441739d7daa931b13029f1c8215a4b1f4e Mon Sep 17 00:00:00 2001
From: Prica <nprica@rt-rk.com>
Date: Thu, 11 Jan 2018 14:49:15 +0100
Subject: [PATCH] Fix for prologue processing on PowerPc

One of conditions in skip_prologue() was never visited if there was mflr
instruction that moves the link register to a register different thant r0.
This condition expects non shifted value of `lr_reg`. Previously offset
of link register was never saved for registers different than r0.

gdb/ChangeLog:

2018-01-27 Nikola Prica  <nikola.prica@rt-rk.com>

        *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg and
         assign shifted lr_reg to fdata->lr_register when lr_reg is set.

gdb/testsuite/ChangeLog:

2018-01-27 Nikola Prica  <nikola.prica@rt-rk.com>

        *gdb.arch/ppc-prologue-frame.s: New file.
        *gdb.arch/ppc-prologue-frame.c: Likewise.
        *gdb.arch/ppr-prologue-frame.exp: Likewise.
---
 gdb/rs6000-tdep.c                                 |  4 +--
 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S   | 35 +++++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c   | 28 ++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 41 +++++++++++++++++++++++
 4 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index e5a265d..da5182e 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1654,7 +1654,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
 	     remember just the first one, but skip over additional
 	     ones.  */
 	  if (lr_reg == -1)
-	    lr_reg = (op & 0x03e00000) >> 21;
+	    lr_reg = (op & 0x03e00000);
           if (lr_reg == 0)
             r0_contains_arg = 0;
 	  continue;
@@ -2180,7 +2180,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
 #endif /* 0 */
 
   if (pc == lim_pc && lr_reg >= 0)
-    fdata->lr_register = lr_reg;
+    fdata->lr_register = lr_reg >> 21;
 
   fdata->offset = -fdata->offset;
   return last_prologue_pc;
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
new file mode 100644
index 0000000..e30ca23
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
@@ -0,0 +1,35 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <ppc-asm.h>
+
+FUNC_START(foo)
+	stwu 1,-32(1)
+	mflr 3
+	stw 3,36(1)
+	stw 31,28(1)
+	mr 31,1
+	bl bar
+	mr 9,3
+	mr 3,9
+	addi 11,31,32
+	lwz 0,4(11)
+	mtlr 0
+	lwz 31,-4(11)
+	mr 1,11
+	blr
+FUNC_END(foo)
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
new file mode 100644
index 0000000..8cab6f2
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
@@ -0,0 +1,28 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int bar()
+{
+  return 0;
+}
+
+int foo();
+
+int main(void)
+{
+  return foo();
+}
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
new file mode 100644
index 0000000..d26314b
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
@@ -0,0 +1,41 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>
+
+if {![istarget "powerpc-*-*"] } {
+    verbose "Skipping powerpc back trace test."
+    return
+}
+
+standard_testfile .c .S
+set binfile [standard_output_file ${testfile}]
+
+if {[gdb_compile \
+      [list ${srcdir}/${subdir}/$srcfile ${srcdir}/${subdir}/$srcfile2] \
+      "${binfile}" executable {}] != ""} {
+    untested "failed to build $binfile"
+    return -1
+}
+
+
+clean_restart ${binfile}
+
+if ![runto bar] {
+  untested "could not run to bar"
+  return -1
+}
+
+gdb_test "bt" \
+         "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \
+         "Backtrace to the main frame"
-- 
2.7.4

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Pedro Franco de Carvalho]

Looks good to me. Just two details for the testsuite changelog,
the filenames in the entry don't match the actual filenames, and there's
a space between the "*" and the entries.

I tested the patch with ppc64le and ppc32, and it works. No regressions.

Thanks!
Pedro

Nikola Prica <nikola.prica@rt-rk.com> writes:

> Hi Pedro,
>
> Thank you for your review and patience. I'm really grateful.
>
> Can someone commit this patch?
>
> Thanks,
>
> Nikola
> From 4b32b0441739d7daa931b13029f1c8215a4b1f4e Mon Sep 17 00:00:00 2001
> From: Prica <nprica@rt-rk.com>
> Date: Thu, 11 Jan 2018 14:49:15 +0100
> Subject: [PATCH] Fix for prologue processing on PowerPc
>
> One of conditions in skip_prologue() was never visited if there was mflr
> instruction that moves the link register to a register different thant r0.
> This condition expects non shifted value of `lr_reg`. Previously offset
> of link register was never saved for registers different than r0.
>
> gdb/ChangeLog:
>
> 2018-01-27 Nikola Prica  <nikola.prica@rt-rk.com>
>
>         *rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg and
>          assign shifted lr_reg to fdata->lr_register when lr_reg is set.
>
> gdb/testsuite/ChangeLog:
>
> 2018-01-27 Nikola Prica  <nikola.prica@rt-rk.com>
>
>         *gdb.arch/ppc-prologue-frame.s: New file.
>         *gdb.arch/ppc-prologue-frame.c: Likewise.
>         *gdb.arch/ppr-prologue-frame.exp: Likewise.
> ---
>  gdb/rs6000-tdep.c                                 |  4 +--
>  gdb/testsuite/gdb.arch/powerpc-prologue-frame.S   | 35 +++++++++++++++++++
>  gdb/testsuite/gdb.arch/powerpc-prologue-frame.c   | 28 ++++++++++++++++
>  gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 41 +++++++++++++++++++++++
>  4 files changed, 106 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
>  create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
>
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index e5a265d..da5182e 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1654,7 +1654,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
>  	     remember just the first one, but skip over additional
>  	     ones.  */
>  	  if (lr_reg == -1)
> -	    lr_reg = (op & 0x03e00000) >> 21;
> +	    lr_reg = (op & 0x03e00000);
>            if (lr_reg == 0)
>              r0_contains_arg = 0;
>  	  continue;
> @@ -2180,7 +2180,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
>  #endif /* 0 */
>
>    if (pc == lim_pc && lr_reg >= 0)
> -    fdata->lr_register = lr_reg;
> +    fdata->lr_register = lr_reg >> 21;
>
>    fdata->offset = -fdata->offset;
>    return last_prologue_pc;
> diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
> new file mode 100644
> index 0000000..e30ca23
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
> @@ -0,0 +1,35 @@
> +/* This test is part of GDB, the GNU debugger.
> +
> +   Copyright 2018 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <ppc-asm.h>
> +
> +FUNC_START(foo)
> +	stwu 1,-32(1)
> +	mflr 3
> +	stw 3,36(1)
> +	stw 31,28(1)
> +	mr 31,1
> +	bl bar
> +	mr 9,3
> +	mr 3,9
> +	addi 11,31,32
> +	lwz 0,4(11)
> +	mtlr 0
> +	lwz 31,-4(11)
> +	mr 1,11
> +	blr
> +FUNC_END(foo)
> diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
> new file mode 100644
> index 0000000..8cab6f2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
> @@ -0,0 +1,28 @@
> +/* This test is part of GDB, the GNU debugger.
> +
> +   Copyright 2018 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int bar()
> +{
> +  return 0;
> +}
> +
> +int foo();
> +
> +int main(void)
> +{
> +  return foo();
> +}
> diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
> new file mode 100644
> index 0000000..d26314b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
> @@ -0,0 +1,41 @@
> +# Copyright 2018 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>
> +
> +if {![istarget "powerpc-*-*"] } {
> +    verbose "Skipping powerpc back trace test."
> +    return
> +}
> +
> +standard_testfile .c .S
> +set binfile [standard_output_file ${testfile}]
> +
> +if {[gdb_compile \
> +      [list ${srcdir}/${subdir}/$srcfile ${srcdir}/${subdir}/$srcfile2] \
> +      "${binfile}" executable {}] != ""} {
> +    untested "failed to build $binfile"
> +    return -1
> +}
> +
> +
> +clean_restart ${binfile}
> +
> +if ![runto bar] {
> +  untested "could not run to bar"
> +  return -1
> +}
> +
> +gdb_test "bt" \
> +         "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \
> +         "Backtrace to the main frame"
> -- 
> 2.7.4

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Ulrich Weigand]

Pedro Franco de Carvalho wrote:

> Looks good to me. Just two details for the testsuite changelog,
> the filenames in the entry don't match the actual filenames, and there's
> a space between the "*" and the entries.
> 
> I tested the patch with ppc64le and ppc32, and it works. No regressions.
> 
> Thanks!
> Pedro
> 
> Nikola Prica <nikola.prica@rt-rk.com> writes:
> 
> > Hi Pedro,
> >
> > Thank you for your review and patience. I'm really grateful.
> >
> > Can someone commit this patch?


The patch looks good to me as well.  Thanks for the review, Pedro!

I've checked this is now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Nikola Prica]

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

Thank you for your review.

Best regards,

Nikola

[-- Attachment #2: 0001-Fix-for-prologue-processing-on-PowerPc.patch --]
[-- Type: text/x-patch, Size: 6142 bytes --]

From d6d907b16d7e7d2a9ee7f147a0d0a1166cf557ae Mon Sep 17 00:00:00 2001
From: Prica <nprica@rt-rk.com>
Date: Thu, 11 Jan 2018 14:49:15 +0100
Subject: [PATCH] Fix for prologue processing on PowerPc

One of conditions in skip_prologue() was never visited if there was mflr
instruction that moves the link register to a register different thant r0.
This condition expects non shifted value of `lr_reg`. Previously offset
of link register was never saved for registers different than r0.

gdb/ChangeLog:

2018-02-01 Nikola Prica  <nikola.prica@rt-rk.com>

        * rs6000-tdep.c (skip_prologue): Remove shifting for lr_reg and
        assign shifted lr_reg to fdata->lr_register when lr_reg is set.

gdb/testsuite/ChangeLog:

2018-02-01 Nikola Prica  <nikola.prica@rt-rk.com>

        * gdb.arch/powerpc-prologue-frame.s: New file.
        * gdb.arch/powerpc-prologue-frame.c: Likewise.
        * gdb.arch/poweroc-prologue-frame.exp: Likewise.
---
 gdb/rs6000-tdep.c                                 |  4 +--
 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S   | 35 +++++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c   | 28 ++++++++++++++++
 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 41 +++++++++++++++++++++++
 4 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
 create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index e5a265d..da5182e 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1654,7 +1654,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
 	     remember just the first one, but skip over additional
 	     ones.  */
 	  if (lr_reg == -1)
-	    lr_reg = (op & 0x03e00000) >> 21;
+	    lr_reg = (op & 0x03e00000);
           if (lr_reg == 0)
             r0_contains_arg = 0;
 	  continue;
@@ -2180,7 +2180,7 @@ skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc, CORE_ADDR lim_pc,
 #endif /* 0 */
 
   if (pc == lim_pc && lr_reg >= 0)
-    fdata->lr_register = lr_reg;
+    fdata->lr_register = lr_reg >> 21;
 
   fdata->offset = -fdata->offset;
   return last_prologue_pc;
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
new file mode 100644
index 0000000..e30ca23
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S
@@ -0,0 +1,35 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <ppc-asm.h>
+
+FUNC_START(foo)
+	stwu 1,-32(1)
+	mflr 3
+	stw 3,36(1)
+	stw 31,28(1)
+	mr 31,1
+	bl bar
+	mr 9,3
+	mr 3,9
+	addi 11,31,32
+	lwz 0,4(11)
+	mtlr 0
+	lwz 31,-4(11)
+	mr 1,11
+	blr
+FUNC_END(foo)
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
new file mode 100644
index 0000000..8cab6f2
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c
@@ -0,0 +1,28 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int bar()
+{
+  return 0;
+}
+
+int foo();
+
+int main(void)
+{
+  return foo();
+}
diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
new file mode 100644
index 0000000..d26314b
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp
@@ -0,0 +1,41 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>
+
+if {![istarget "powerpc-*-*"] } {
+    verbose "Skipping powerpc back trace test."
+    return
+}
+
+standard_testfile .c .S
+set binfile [standard_output_file ${testfile}]
+
+if {[gdb_compile \
+      [list ${srcdir}/${subdir}/$srcfile ${srcdir}/${subdir}/$srcfile2] \
+      "${binfile}" executable {}] != ""} {
+    untested "failed to build $binfile"
+    return -1
+}
+
+
+clean_restart ${binfile}
+
+if ![runto bar] {
+  untested "could not run to bar"
+  return -1
+}
+
+gdb_test "bt" \
+         "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \
+         "Backtrace to the main frame"
-- 
2.7.4

Re: [PING][PATCH] Fix for prologue processing on PowerPC

[Yao Qi]

On Tue, Dec 19, 2017 at 3:57 PM, Nikola Prica <nikola.prica@rt-rk.com> wrote:
> Hello Pedro and Kevi,
>
> I've created and tested example based on your example. Thank you for that.
>

gdb.arch/powerpc-prologue-frame.exp is useful, but it is only run on powerpc
target.  I personally prefer using gdb unit test or self test to test
powerpc prologue
analyser can stop at the the right instruction after scanning a series of
instructions.  See aarch64-tdep.c:aarch64_analyze_prologue_test.

-- 
Yao (齐尧)