From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18540 invoked by alias); 10 Jan 2018 17:26:02 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 18530 invoked by uid 89); 10 Jan 2018 17:26:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.6 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=trustworthy X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 10 Jan 2018 17:25:59 +0000 Received: from pps.filterd (m0098420.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w0AHNpgE090615 for ; Wed, 10 Jan 2018 12:25:57 -0500 Received: from e33.co.us.ibm.com (e33.co.us.ibm.com [32.97.110.151]) by mx0b-001b2d01.pphosted.com with ESMTP id 2fdny4c3g7-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 10 Jan 2018 12:25:57 -0500 Received: from localhost by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 10 Jan 2018 10:25:56 -0700 Received: from b03cxnp07029.gho.boulder.ibm.com (9.17.130.16) by e33.co.us.ibm.com (192.168.1.133) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 10 Jan 2018 10:25:52 -0700 Received: from b03ledav006.gho.boulder.ibm.com (b03ledav006.gho.boulder.ibm.com [9.17.130.237]) by b03cxnp07029.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id w0AHPqJp11600290; Wed, 10 Jan 2018 10:25:52 -0700 Received: from b03ledav006.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id E1DD5C604F; Wed, 10 Jan 2018 10:25:51 -0700 (MST) Received: from pedro.localdomain (unknown [9.80.218.239]) by b03ledav006.gho.boulder.ibm.com (Postfix) with ESMTP id 6BF52C603E; Wed, 10 Jan 2018 10:25:51 -0700 (MST) Received: by pedro.localdomain (Postfix, from userid 1000) id 7BF5C3C564E; Wed, 10 Jan 2018 15:22:34 -0200 (-02) From: Pedro Franco de Carvalho To: Nikola Prica , Kevin Buettner Cc: gdb-patches@sourceware.org, "Ananthakrishna Sowda \(asowda\)" , "Ivan Baev \(ibaev\)" , "'Nemanja Popov'" , Djordje Todorovic , Ulrich.Weigand@de.ibm.com Subject: Re: [PING][PATCH] Fix for prologue processing on PowerPC In-Reply-To: References: <20171108095850.394a48ca@pinnacle.lan> <8bf0014c-e83c-5988-4d06-173572f21186@rt-rk.com> <7ba16b14-9384-34d9-937e-531a2192842a@linux.vnet.ibm.com> <87608p4dgr.fsf@linux.vnet.ibm.com> Date: Wed, 10 Jan 2018 17:26:00 -0000 MIME-Version: 1.0 Content-Type: text/plain X-TM-AS-GCONF: 00 x-cbid: 18011017-0008-0000-0000-0000092698D8 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00008355; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000245; SDB=6.00973059; UDB=6.00493007; IPR=6.00753009; BA=6.00005772; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00018964; XFM=3.00000015; UTC=2018-01-10 17:25:54 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 18011017-0009-0000-0000-0000457E6715 Message-Id: <87r2qx4oit.fsf@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-01-10_08:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=7 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1801100244 X-IsSubscribed: yes X-SW-Source: 2018-01/txt/msg00205.txt.bz2 Hello Nikola, > This part of code was previously never visited because of lr_reg > shifting. The last line in upper code overwrites value of lr_reg. If in > the furtherer iteration lim_pc would be reached, -2 value would be > shifted and set to fdata->lr_register and that is not what we want. It could be visited when lr_reg == 0 (because in this case lr_reg >> 21 == lr_reg). I suspect this is a common case, when I used gcc it usually generated the mflr/stw sequence using r0. In the old code, even if the limit was reached, the fdata->lr_register variable would not be set to lr_reg if lr_reg was previously invalidated (set to -2), assuming mflr/stw used r0. You are correct that previously, for any other register than r0, the block that detects the "stw" part of the sequence would never be reached, which would cause fdata->lr_register to be set, if lim_pc was reached, even if LR was saved in the stack by the stw. However, it seems to me that this was unintentional, and that the original intention was to set fdata->lr_register only if it was valid and not yet saved in the stack when skip_prologue was called. So I think it's safer to preserve this behavior, that is, fdata->lr_register should be -1 at function exit if either the limit wasn't reached or lr_reg was saved to the stack or never seen (which would be reflected by -2 or -1, respectively). Note that the comment in the definition of struct rs6000_framedata specifices "if trustworthy". int lr_register; /* register of saved lr, if trustworthy */ Here is an example. Take the sequence: foo: stwu 1,-32(1) mflr 0 nop ... If you break at the nop, and run backtrace, skip_prologue will be called to build the frame_cache and figure out where the link register was saved so that the program counter from the previous stack frame can be found. In this case, skip_prologue will be called with lim_pc pointing to the nop (which is where we are). Because r0 is not saved back to the stack, and because in this case pc reaches lim_pc, fdata->lr_register is valid and should hold lr_reg. Take this other sequence: foo: stwu 1,-32(1) mflr 0 stw 0,36(1) li 0,-1 nop If we break at the nop, and call backtrace, LR will already have been saved in the stack, so fdata->lr_offset should be valid, but not fdata->lr_register. Note that it was overwritten by the li instruction, so r0 can no longer be trusted to hold the value of LR. I suspect the callers of skip_prologue will use lr_offset over lr_register if both are set, but I think it's better to be conservative and ensure fdata is valid. >> Still, I think it's safer to keep fdata->lr_register == -1 and set it at >> the end if needed. >> > > > This could be achieved by using one more variable that would be backup > for lr_reg in case it is set to -2. But I think that this approach with > setting it to -1 is more appropriate than that? It could be done without an additional variable by shifting lr_reg and assigning the result to lr_register only at the end, like in Kevin's suggestion. I'm just not sure that the bitwise operations with lr_reg are safe considering it is an int (e.g., is lr_reg | 0x90010000 well-defined?). Thanks! Pedro Nikola Prica writes: > Hiello Pedro, > >>> > - 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. >>> >>> The body of condition that becomes visitable after this patch >>> invalidates lr_reg by setting it to -2 before reaching the limit. >> >> Sorry, I was referring to to fdata->lr_register. What I meant is that it >> seems better to me to only set fdata->lr_register to lr_reg >> 21 at the end >> of the function (as Kevin had suggested), as opposed to setting it to -1 >> at the end of the function, in this part: >> > > I didn't explain it right. The catch is that fdata->lr_register is set > by shifting lr_reg variable which is set only once and it can be > invalidated druing the loop iteration. For example, lets say that > firstly lr_reg is set to some value, then in some further loop iteration > the next code gets visited: > > 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)))) > { /* where Rx == lr */ > fdata->lr_offset = offset; > fdata->nosavedpc = 0; > /* 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 code was previously never visited because of lr_reg > shifting. The last line in upper code overwrites value of lr_reg. If in > the furtherer iteration lim_pc would be reached, -2 value would be > shifted and set to fdata->lr_register and that is not what we want. > > >> >> Previously if there was a mflr/stw sequence that invalidated lr_reg, >> fdata->lr_register would be -1 when the function exited, even if pc == >> lim_pc (because of the second term of the condition). >> > > > Previously if there was s mflr/stw sequence lr_reg would never be > invalidated (set to -2) because part of the code that does this > invalidation was never visitable due to shifting of lr_reg. Invalidation > is performed so that the upper code never gets visited again in same > iteration. lr_reg is set only once and it's value is used only in the > condition of upper part of the code. In other conditions it is only > checked whether it is set (different than -1). The comment where lr_reg > is changed from -1 to some value says the following: > > "remember just the first one, but skip over additional ones." > > Which means that we need to deal only with one value of lr_reg. And the > fdata->lr_register basically saves the register number that we get by > shifting lr_reg. > >> >> In any case, for this part to be correct, shouldn't the condition be the >> full complement of the original condition? >> >> That is, >> >> if (pc != lim_pc || lr_reg < 0) >> >> Which is the complement of: >> >> if (pc == lim_pc && lr_reg >= 0) >> >> Instead of: >> >>> + if (pc != lim_pc) > > > I've changed this condition to following: > > if (pc != lim_pc && lr_reg != -1) > > This is more appropriate condition because this condition requires > change of lr_reg. > > >> Still, I think it's safer to keep fdata->lr_register == -1 and set it at >> the end if needed. >> > > > This could be achieved by using one more variable that would be backup > for lr_reg in case it is set to -2. But I think that this approach with > setting it to -1 is more appropriate than that? > > Thanks, > > Nikola Prica > From d93689d414137926818429fe0910ad9a9e6ef0dd Mon Sep 17 00:00:00 2001 > From: Prica > Date: Tue, 19 Dec 2017 14:29:09 +0100 > Subject: [PATCH] [PATCH] Fix for prologue processing on PowerPc > > One of conditions in skip_prologue() is never visited because it > expects non shifted `lr_reg`. That condtition 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: > *rs600-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. > *testsuite/gdb.arch/ppc-prologue-frame.s: New file. > *testsuite/gdb.arch/ppc-prologue-frame.c: Likewise. > *testsuite/gdb.arch/ppr-prologue-frame.exp: Likewise. > --- > gdb/rs6000-tdep.c | 14 ++++--- > gdb/testsuite/gdb.arch/powerpc-prologue-frame.S | 35 +++++++++++++++++ > gdb/testsuite/gdb.arch/powerpc-prologue-frame.c | 28 ++++++++++++++ > gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp | 47 +++++++++++++++++++++++ > 4 files changed, 119 insertions(+), 5 deletions(-) > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.S > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > create mode 100644 gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > > diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c > index 456dbcc..7bf4f60 100644 > --- a/gdb/rs6000-tdep.c > +++ b/gdb/rs6000-tdep.c > @@ -1655,9 +1655,13 @@ 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 +2184,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 && lr_reg != -1) > + fdata->lr_register = -1; > > fdata->offset = -fdata->offset; > return last_prologue_pc; > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S > new file mode 100644 > index 0000000..e30ca23 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.S > @@ -0,0 +1,35 @@ > +/* This test is part of GDB, the GNU debugger. > + > + Copyright 2018 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include > + > +FUNC_START(foo) > + stwu 1,-32(1) > + mflr 3 > + stw 3,36(1) > + stw 31,28(1) > + mr 31,1 > + bl bar > + mr 9,3 > + mr 3,9 > + addi 11,31,32 > + lwz 0,4(11) > + mtlr 0 > + lwz 31,-4(11) > + mr 1,11 > + blr > +FUNC_END(foo) > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > new file mode 100644 > index 0000000..8cab6f2 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.c > @@ -0,0 +1,28 @@ > +/* This test is part of GDB, the GNU debugger. > + > + Copyright 2018 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +int bar() > +{ > + return 0; > +} > + > +int foo(); > + > +int main(void) > +{ > + return foo(); > +} > diff --git a/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > new file mode 100644 > index 0000000..4f988db > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/powerpc-prologue-frame.exp > @@ -0,0 +1,47 @@ > +# Copyright 2018 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see > + > +if {![istarget "powerpc*"] } { > + verbose "Skipping powerpc back trace test." > + return > +} > + > +standard_testfile .c .S > +set binfile [standard_output_file ${testfile}] > + > +if {![istarget "powerpc-*-*"]} then { > + verbose "Skipping PowerPC instructions disassembly." > + return -1 > +} > + > + > +if {[gdb_compile \ > + [list ${srcdir}/${subdir}/$srcfile ${srcdir}/${subdir}/$srcfile2] \ > + "${binfile}" executable {}] != ""} { > + untested "failed to build $binfile" > + return -1 > +} > + > + > +clean_restart ${binfile} > + > +if ![runto bar] { > + untested "could not run to bar" > + return -1 > +} > + > +gdb_test "bt" \ > + "#0\[ \t\]*$hex in bar.*\r\n#1\[ \t\]*$hex in foo.*\r\n#2\[ \t\]*$hex in main.*" \ > + "Backtrace to the main frame" > -- > 2.7.4