Message ID | 8bf0014c-e83c-5988-4d06-173572f21186@rt-rk.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 13318 invoked by alias); 9 Nov 2017 18:15:35 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 5484 invoked by uid 89); 9 Nov 2017 18:15:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=HTo:U*kevinb, rights X-HELO: mail.rt-rk.com Received: from mx2.rt-rk.com (HELO mail.rt-rk.com) (89.216.37.149) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 09 Nov 2017 18:15:15 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.rt-rk.com (Postfix) with ESMTP id 234AF1A4575; Thu, 9 Nov 2017 19:15:10 +0100 (CET) Received: from [10.10.13.98] (rtrkw512-lin.domain.local [10.10.13.98]) by mail.rt-rk.com (Postfix) with ESMTPSA id D861E1A239C; Thu, 9 Nov 2017 19:15:09 +0100 (CET) Subject: Re: [PING][PATCH] Fix for prologue processing on PowerPC To: Kevin Buettner <kevinb@redhat.com> Cc: gdb-patches@sourceware.org, "Ananthakrishna Sowda (asowda)" <asowda@cisco.com>, "Ivan Baev (ibaev)" <ibaev@cisco.com>, 'Nemanja Popov' <nemanja.popov@rt-rk.com>, Djordje Todorovic <Djordje.Todorovic@rt-rk.com> References: <f90d189e-7ec5-34f9-c776-8af42a3c07a6@rt-rk.com> <e3274bee-eb2d-3706-3af0-59f28c9c0899@rt-rk.com> <20171108095850.394a48ca@pinnacle.lan> From: Nikola Prica <nikola.prica@rt-rk.com> Message-ID: <8bf0014c-e83c-5988-4d06-173572f21186@rt-rk.com> Date: Thu, 9 Nov 2017 19:15:06 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171108095850.394a48ca@pinnacle.lan> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit |
Commit Message
Nikola Prica
Nov. 9, 2017, 6:15 p.m. UTC
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(-)
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;
Comments
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; .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
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; }