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

[pedromfc <pedromfc at linux dot vnet dot ibm dot com>]

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;

int bar()
{
  return 0;
}

int foo();

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

Attachment: foo.S
Description: Text document