Fix for prologue processing on PowerPC

Message ID f90d189e-7ec5-34f9-c776-8af42a3c07a6@rt-rk.com
State New, archived
Headers

Commit Message

Nikola Prica Sept. 22, 2017, 12:11 p.m. UTC
  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.
  

Comments

Nikola Prica Oct. 26, 2017, 10:02 a.m. UTC | #1
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.
  
Kevin Buettner Nov. 8, 2017, 4:58 p.m. UTC | #2
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.
  

Patch

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