From patchwork Thu Oct 5 02:01:48 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Buettner X-Patchwork-Id: 23336 Received: (qmail 45078 invoked by alias); 5 Oct 2017 02:01:58 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 45019 invoked by uid 89); 5 Oct 2017 02:01:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=suitably, proposition, scanning X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 05 Oct 2017 02:01:50 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8C109FECD for ; Thu, 5 Oct 2017 02:01:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 8C109FECD Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=kevinb@redhat.com Received: from pinnacle.lan (ovpn-116-131.phx2.redhat.com [10.3.116.131]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 661C95D9C0 for ; Thu, 5 Oct 2017 02:01:49 +0000 (UTC) Date: Wed, 4 Oct 2017 19:01:48 -0700 From: Kevin Buettner To: gdb-patches@sourceware.org Subject: Re: Fix for prologue processing on PowerPC Message-ID: <20171004190148.0a8c9879@pinnacle.lan> In-Reply-To: References: MIME-Version: 1.0 X-IsSubscribed: yes On Fri, 22 Sep 2017 14:11:51 +0200 Nikola Prica 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;