Re: Fix for prologue processing on PowerPC

[Kevin Buettner <kevinb@redhat.com>]

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;