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

[pedromfc <pedromfc@linux.vnet.ibm.com>]

[-- 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