Message ID | 1399448485-1740-1-git-send-email-tmirza@codesourcery.com |
---|---|
State | Committed |
Headers |
Return-Path: <x14314964@homiemail-mx23.g.dreamhost.com> X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx23.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id 00939360079 for <siddhesh@wilcox.dreamhost.com>; Wed, 7 May 2014 00:41:48 -0700 (PDT) Received: by homiemail-mx23.g.dreamhost.com (Postfix, from userid 14314964) id 81D0463827666; Wed, 7 May 2014 00:41:48 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx23.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx23.g.dreamhost.com (Postfix) with ESMTPS id 5667C6382764A for <gdb@patchwork.siddhesh.in>; Wed, 7 May 2014 00:41:48 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:date:message-id :mime-version:content-type; q=dns; s=default; b=hVkkQ83HjG7h43/D INV+DCcuiuYgtIl+wtWbpWFKBarFUH7vhcYWcOiMwmNDTO4uUkW4Hw7+Szc8wZXu qiIrvs8Gsuab856BWaWftuWPNuguQJV+eqgjxq/csFGXFCNynZzsukCjbtd6sf4R qTIQbCjKSnzcTIUh9R6Q9WMIoPM= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:date:message-id :mime-version:content-type; s=default; bh=pqMVPZYUFHtImUWFbFAU9O XXyLc=; b=lqqOimLoajHgbVyrVf/ZPmhuY5yGEfQQyM+A5GK4Co9MMGfMJ3mod8 wmZCJ0j+Pku4y+A+iCdnn3iSr/7DTB4jXM30N5zPJtfie8cZmiC/U8mE9zW+3LIT IT9mAq5O0XbmKV7Z35Aadd64b1S102iprwBvL8RJjJTmZvedsX6w0= Received: (qmail 27517 invoked by alias); 7 May 2014 07:41:46 -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-gdb=patchwork.siddhesh.in@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 27506 invoked by uid 89); 7 May 2014 07:41:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00 autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 07 May 2014 07:41:44 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1WhwU0-0006Mn-NQ from Taimoor_Mirza@mentor.com for gdb-patches@sourceware.org; Wed, 07 May 2014 00:41:41 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Wed, 7 May 2014 00:41:38 -0700 Received: from PKL-TAIMOORMI-LT.pkl.mentorg.com (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server (TLS) id 14.2.247.3; Wed, 7 May 2014 08:41:37 +0100 From: Taimoor Mirza <tmirza@codesourcery.com> To: <gdb-patches@sourceware.org> CC: Taimoor Mirza <tmirza@codesourcery.com> Subject: [PATCH] Fix prologue analysis for ldr.w and ldrd instruction Date: Wed, 7 May 2014 12:41:25 +0500 Message-ID: <1399448485-1740-1-git-send-email-tmirza@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-DH-Original-To: gdb@patchwork.siddhesh.in |
Commit Message
Taimoor Mirza
May 7, 2014, 7:41 a.m. UTC
Hi Guys, We experienced a problem while debugging startup code on Cortex-M targets (LPC4350 and ATSAM3UEK). On stepping through the code, GDB was throwing "invalid memory access" error. We found out that problem is coming because of wrong offset calculation for ldr.w and ldrd instruction during prologue analysis in thumb-mode. Below is problematic code in *thumb_analyze_prologue* function: else if ((insn & 0xff7f) == 0xf85f) /* ldr.w Rt,<label> */ { /* Constant pool loads. */ unsigned int constant; CORE_ADDR loc; offset = bits (insn, 0, 11); if (insn & 0x0080) loc = start + 4 + offset; else loc = start + 4 - offset; constant = read_memory_unsigned_integer (loc, 4, byte_order); regs[bits (inst2, 12, 15)] = pv_constant (constant); } The problem is at line *offset = bits (insn, 0, 11);* where it is obtaining offset from first two bytes of instruction that contain opcode of ldr.w instruction. As per Cortex-M reference manual and BFD code, it should be: offset = bits (inst2, 0, 11); inst2 contains next two bytes of ldr.w instruction and it is correctly used to get register information. Similarly inst2 should be used to obtain offset. Similar problem exists in ldrd instruction's offset calculation. Below patch provides fix of this problem. Is it OK? Thanks, Taimoor 2014-05-06 Taimoor Mirza <tmirza@codesourcery.com> gdb/ * arm-tdep.c (thumb_analyze_prologue): Fix offset calculation for ldr.w and ldrd instructions. --- gdb/arm-tdep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Comments
On 7 May 2014 08:41, Taimoor Mirza <tmirza@codesourcery.com> wrote: > Hi Guys, > > We experienced a problem while debugging startup code on Cortex-M targets (LPC4350 and ATSAM3UEK). On stepping through the code, GDB was throwing "invalid memory access" error. We found out that problem > is coming because of wrong offset calculation for ldr.w and ldrd instruction during prologue analysis in thumb-mode. Below is problematic code in *thumb_analyze_prologue* function: > > else if ((insn & 0xff7f) == 0xf85f) /* ldr.w Rt,<label> */ > { > /* Constant pool loads. */ > unsigned int constant; > CORE_ADDR loc; > > offset = bits (insn, 0, 11); > if (insn & 0x0080) > loc = start + 4 + offset; > else > loc = start + 4 - offset; > > constant = read_memory_unsigned_integer (loc, 4, byte_order); > regs[bits (inst2, 12, 15)] = pv_constant (constant); > } > > The problem is at line *offset = bits (insn, 0, 11);* where it is obtaining offset from first two bytes of instruction that contain opcode of ldr.w instruction. As per Cortex-M reference manual and BFD code, it should be: > > offset = bits (inst2, 0, 11); > > inst2 contains next two bytes of ldr.w instruction and it is correctly used to get register information. Similarly inst2 should be used to obtain offset. Similar problem exists in ldrd instruction's offset calculation. Below patch provides fix of this problem. Is it OK? > > Thanks, > Taimoor > > 2014-05-06 Taimoor Mirza <tmirza@codesourcery.com> > > gdb/ > * arm-tdep.c (thumb_analyze_prologue): Fix offset calculation for > ldr.w and ldrd instructions. > --- > gdb/arm-tdep.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > index e3b1c3d..7271777 100644 > --- a/gdb/arm-tdep.c > +++ b/gdb/arm-tdep.c > @@ -1071,7 +1071,7 @@ thumb_analyze_prologue (struct gdbarch *gdbarch, > unsigned int constant; > CORE_ADDR loc; > > - offset = bits (insn, 0, 11); > + offset = bits (inst2, 0, 11); > if (insn & 0x0080) > loc = start + 4 + offset; > else > @@ -1087,7 +1087,7 @@ thumb_analyze_prologue (struct gdbarch *gdbarch, > unsigned int constant; > CORE_ADDR loc; > > - offset = bits (insn, 0, 7) << 2; > + offset = bits (inst2, 0, 7) << 2; > if (insn & 0x0080) > loc = start + 4 + offset; > else This looks good to me, assuming the testsuite was run with no regressions.
> > 2014-05-06 Taimoor Mirza <tmirza@codesourcery.com> > > > > gdb/ > > * arm-tdep.c (thumb_analyze_prologue): Fix offset calculation for > > ldr.w and ldrd instructions. [...] > This looks good to me, assuming the testsuite was run with no regressions. Thanks, Will, for reviewing the patch. I concur on both counts: patch is approved, but can you please also confirm how the patch was tested? Thank you,
On 05/07/2014 06:25 PM, Joel Brobecker wrote: >>> 2014-05-06 Taimoor Mirza <tmirza@codesourcery.com> >>> >>> gdb/ >>> * arm-tdep.c (thumb_analyze_prologue): Fix offset calculation for >>> ldr.w and ldrd instructions. > [...] >> This looks good to me, assuming the testsuite was run with no regressions. Thanks for the review Will. Yes I ran testsuites. > Thanks, Will, for reviewing the patch. I concur on both counts: > patch is approved, but can you please also confirm how the patch > was tested? I ran testsuites before and after applying my patch and did not find any regression. > > Thank you,
Joel, On 05/07/2014 12:33 PM, tmirza wrote: > On 05/07/2014 06:25 PM, Joel Brobecker wrote: >>>> 2014-05-06 Taimoor Mirza <tmirza@codesourcery.com> >>>> >>>> gdb/ >>>> * arm-tdep.c (thumb_analyze_prologue): Fix offset >>>> calculation for >>>> ldr.w and ldrd instructions. >> [...] >>> This looks good to me, assuming the testsuite was run with no >>> regressions. > Thanks for the review Will. Yes I ran testsuites. >> Thanks, Will, for reviewing the patch. I concur on both counts: >> patch is approved, but can you please also confirm how the patch >> was tested? > I ran testsuites before and after applying my patch and did not find any > regression. >> >> Thank you, Just for the sake of completeness, the patch was tested against real boards and a simulator. Luis
> >>>2014-05-06 Taimoor Mirza <tmirza@codesourcery.com> > >>> > >>> gdb/ > >>> * arm-tdep.c (thumb_analyze_prologue): Fix offset calculation for > >>> ldr.w and ldrd instructions. > >[...] > >>This looks good to me, assuming the testsuite was run with no regressions. > Thanks for the review Will. Yes I ran testsuites. > >Thanks, Will, for reviewing the patch. I concur on both counts: > >patch is approved, but can you please also confirm how the patch > >was tested? > I ran testsuites before and after applying my patch and did not find > any regression. I noticed that there is a new branch on the binutils-gdb repository called CB-2131, and that branch only contains your patch. Does this ring a bell to you? Perhaps that's the name of the branch you used on your end and you pushed it by accident? Thanks,
On 05/19/2014 08:11 PM, Joel Brobecker wrote: > I noticed that there is a new branch on the binutils-gdb repository > called CB-2131, and that branch only contains your patch. Does this > ring a bell to you? Perhaps that's the name of the branch you used > on your end and you pushed it by accident? yes, it was pushed accidentally. It was my local branch that I created to work on this issue and accidentally pushed it before realizing my mistake and pushing changes to origin. I actually got some error message and I thought its not pushed. Anyways, I'll delete this CB-2131 remote branch. Is it Okay? -Taimoor
> yes, it was pushed accidentally. It was my local branch that I > created to work on this issue and accidentally pushed it before > realizing my mistake and pushing changes to origin. Thanks for confirming. > I actually got some error message and I thought its not pushed. I think the error message might actually been from the hook. Something about 00000[...] not being a valid commit? > Anyways, I'll delete this CB-2131 remote branch. Is it Okay? Yes, please! Thank you,
On 05/20/2014 05:09 PM, Joel Brobecker wrote: >> yes, it was pushed accidentally. It was my local branch that I >> created to work on this issue and accidentally pushed it before >> realizing my mistake and pushing changes to origin. > > Thanks for confirming. > >> I actually got some error message and I thought its not pushed. > > I think the error message might actually been from the hook. > Something about 00000[...] not being a valid commit? yes, IIRC, it was something like this. > >> Anyways, I'll delete this CB-2131 remote branch. Is it Okay? > > Yes, please! I have removed CB-2131 branch. kindly confirm its removal. I tried "git remote -ra | grep CB" and don't see it anymore. Thanks, Taimoor
> I have removed CB-2131 branch. kindly confirm its removal. I tried > "git remote -ra | grep CB" and don't see it anymore. Confirmed: % git fetch -p origin From ssh://sourceware.org/git/binutils-gdb x [deleted] (none) -> origin/CB-2131 Thank you!
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index e3b1c3d..7271777 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -1071,7 +1071,7 @@ thumb_analyze_prologue (struct gdbarch *gdbarch, unsigned int constant; CORE_ADDR loc; - offset = bits (insn, 0, 11); + offset = bits (inst2, 0, 11); if (insn & 0x0080) loc = start + 4 + offset; else @@ -1087,7 +1087,7 @@ thumb_analyze_prologue (struct gdbarch *gdbarch, unsigned int constant; CORE_ADDR loc; - offset = bits (insn, 0, 7) << 2; + offset = bits (inst2, 0, 7) << 2; if (insn & 0x0080) loc = start + 4 + offset; else