From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14648 invoked by alias); 16 Feb 2012 16:33:14 -0000 Received: (qmail 14630 invoked by uid 22791); 16 Feb 2012 16:33:05 -0000 X-SWARE-Spam-Status: No, hits=-1.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 16 Feb 2012 16:32:31 +0000 Received: from nat-dem.mentorg.com ([195.212.93.2] helo=eu2-mail.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1Ry4Fy-0000BV-4J from Thomas_Schwinge@mentor.com ; Thu, 16 Feb 2012 08:32:30 -0800 Received: from feldtkeller.schwinge.homeip.net ([172.30.64.128]) by eu2-mail.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Thu, 16 Feb 2012 17:32:27 +0100 From: Thomas Schwinge To: Kevin Buettner Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] [SH] Prologue skipping if there is none In-Reply-To: <20120215165907.33f2e9a6@mesquite.lan> References: <87pqdgciho.fsf@schwinge.name> <20120215075413.1313f7fa@mesquite.lan> <20120215165907.33f2e9a6@mesquite.lan> User-Agent: Notmuch/0.9-101-g81dad07 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) Date: Thu, 16 Feb 2012 16:59:00 -0000 Message-ID: <8739aad9il.fsf@schwinge.name> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" 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 X-SW-Source: 2012-02/txt/msg00331.txt.bz2 --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Content-length: 10836 Hi! On Wed, 15 Feb 2012 16:59:07 -0700, Kevin Buettner wrot= e: > On Wed, 15 Feb 2012 07:54:13 -0700 > Kevin Buettner wrote: >=20 > > On Wed, 15 Feb 2012 14:51:31 +0100 > > Thomas Schwinge wrote: > >=20 > > > The prologue skipping issue is that GDB fails to place breakpoints > > > correctly at the beginning of a function -- such as for ``break main'= ' -- > > > for the case that there is no prologue in that function. > >=20 > > I've been sitting on a patch which is similar to what you just posted, > > though I think it might provide more accurate results in some instances. > > Would you mind checking to see if it solves your problem? >=20 > Below is an updated patch which builds cleanly against current > sources. I've verified that it produces better test results than not > having the patch. I have not compared the test results to Thomas' > patch. I now have (on a SH7785-based board). My patch fixes a few more failures than yours. ;-P Here's the difference from mine to yours: Regressions and new failures: =20=20=20=20 PASS -> FAIL: default/gdb.sum:gdb.base/store.exp: continue to add_chare= st PASS -> FAIL: default/gdb.sum:gdb.base/store.exp: continue to add_double PASS -> FAIL: default/gdb.sum:gdb.base/store.exp: continue to add_doubl= est PASS -> FAIL: default/gdb.sum:gdb.base/store.exp: continue to add_float PASS -> FAIL: default/gdb.sum:gdb.base/store.exp: continue to add_int PASS -> FAIL: default/gdb.sum:gdb.base/store.exp: continue to add_long PASS -> FAIL: default/gdb.sum:gdb.base/store.exp: continue to add_longe= st PASS -> FAIL: default/gdb.sum:gdb.base/store.exp: continue to add_short PASS -> FAIL: default/gdb.sum:gdb.base/structs.exp: advance to fun f= or finish; return 2 structs-td-tf PASS -> FAIL: default/gdb.sum:gdb.base/structs.exp: advance to fun f= or finish; return 2 structs-tf-td PASS -> FAIL: default/gdb.sum:gdb.base/structs.exp: advance to fun f= or return; return 2 structs-td-tf PASS -> FAIL: default/gdb.sum:gdb.base/structs.exp: advance to fun f= or return; return 2 structs-tf-td New FAIL: default/gdb.sum:gdb.threads/staticthreads.exp: Continue to ma= in's call of sem_post (the program exited) New FAIL: default/gdb.sum:gdb.threads/staticthreads.exp: handle SIG32 h= elps (the program exited) Looking at the first one: (gdb) PASS: gdb.base/store.exp: var doublest l; print incremented l, ex= pecting 2 tbreak add_charest Temporary breakpoint 10 at 0x400720: file /scratch/tschwing/FM_sh-linux= -gnu/src/gdb-mainline/gdb/testsuite/gdb.base/store.c, line 14. (gdb) PASS: gdb.base/store.exp: tbreak add_charest continue Continuing. =20=20=20=20 Temporary breakpoint 10, add_charest (u=3D-1 '\377', v=3D32 ' ') at /sc= ratch/tschwing/FM_sh-linux-gnu/src/gdb-mainline/gdb/testsuite/gdb.base/stor= e.c:14 14 { (gdb) FAIL: gdb.base/store.exp: continue to add_charest gdb.base/store.c: 10 typedef signed char charest; 11=20=20 12 charest 13 add_charest (register charest u, register charest v) 14 { 15 return u + v; 16 } So the ``tbreak add_charest'' chose line 14 instead of 15. Just some quick review comments without too much detail; basically me thinking aloud a bit... > * sh-tdep.c (sh_analyze_prologue): Change loop to run to > the limit PC. Keep track of the PC value after frame > related instructions; return this value. > (after_prologue): Delete. > (sh_skip_prologue): Find the function limit and pass that > as the limit address to sh_analyze_prologue(). Also use > skip_prologue_using_sal() and return the lower result. >=20 > Index: sh-tdep.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > RCS file: /cvs/src/src/gdb/sh-tdep.c,v > retrieving revision 1.236 > diff -u -p -r1.236 sh-tdep.c > --- sh-tdep.c 28 Jan 2012 18:08:20 -0000 1.236 > +++ sh-tdep.c 15 Feb 2012 23:55:14 -0000 > @@ -518,39 +518,43 @@ sh_breakpoint_from_pc (struct gdbarch *g >=20=20 > static CORE_ADDR > sh_analyze_prologue (struct gdbarch *gdbarch, > - CORE_ADDR pc, CORE_ADDR current_pc, > + CORE_ADDR pc, CORE_ADDR limit_pc, > struct sh_frame_cache *cache, ULONGEST fpscr) > { > enum bfd_endian byte_order =3D gdbarch_byte_order (gdbarch); > ULONGEST inst; > - CORE_ADDR opc; > + CORE_ADDR after_last_frame_setup_insn =3D pc; Save the original pc instead of having the caller do that. > + CORE_ADDR next_pc; > int offset; > int sav_offset =3D 0; > int r3_val =3D 0; > int reg, sav_reg =3D -1; >=20=20 > - if (pc >=3D current_pc) > - return current_pc; > - > cache->uses_fp =3D 0; That sets cache->uses_fp to false even in case that the limit_pc (was: current_pc) was already reached. > - for (opc =3D pc + (2 * 28); pc < opc; pc +=3D 2) > + > + for (;pc < limit_pc; pc =3D next_pc) The limit of 28 instructions is no longer checked here, but is now the caller's responsibility. (Are all callers doing the right thing?) > { > inst =3D read_memory_unsigned_integer (pc, 2, byte_order); > + next_pc =3D pc + 2; > + > /* See where the registers will be saved to. */ > if (IS_PUSH (inst)) > { > cache->saved_regs[GET_SOURCE_REG (inst)] =3D cache->sp_offset; > cache->sp_offset +=3D 4; > + after_last_frame_setup_insn =3D next_pc; > } > else if (IS_STS (inst)) > { > cache->saved_regs[PR_REGNUM] =3D cache->sp_offset; > cache->sp_offset +=3D 4; > + after_last_frame_setup_insn =3D next_pc; > } > else if (IS_MACL_STS (inst)) > { > cache->saved_regs[MACL_REGNUM] =3D cache->sp_offset; > cache->sp_offset +=3D 4; > + after_last_frame_setup_insn =3D next_pc; > } > else if (IS_MOV_R3 (inst)) > { > @@ -563,11 +567,14 @@ sh_analyze_prologue (struct gdbarch *gdb > else if (IS_ADD_R3SP (inst)) > { > cache->sp_offset +=3D -r3_val; > + after_last_frame_setup_insn =3D next_pc; > } > else if (IS_ADD_IMM_SP (inst)) > { > offset =3D ((inst & 0xff) ^ 0x80) - 0x80; > cache->sp_offset -=3D offset; > + if (offset < 0) > + after_last_frame_setup_insn =3D next_pc; > } > else if (IS_MOVW_PCREL_TO_REG (inst)) > { > @@ -626,6 +633,7 @@ sh_analyze_prologue (struct gdbarch *gdb > sav_reg =3D -1; > } > cache->sp_offset +=3D sav_offset; > + after_last_frame_setup_insn =3D next_pc; > } > else if (IS_FPUSH (inst)) > { > @@ -637,17 +645,20 @@ sh_analyze_prologue (struct gdbarch *gdb > { > cache->sp_offset +=3D 4; > } > + after_last_frame_setup_insn =3D next_pc; > } > else if (IS_MOV_SP_FP (inst)) > { > + CORE_ADDR opc; > cache->uses_fp =3D 1; > + after_last_frame_setup_insn =3D next_pc; > /* At this point, only allow argument register moves to other > registers or argument register moves to @(X,fp) which are > moving the register arguments onto the stack area allocated > by a former add somenumber to SP call. Don't allow moving > to an fp indirect address above fp + cache->sp_offset. */ > pc +=3D 2; > - for (opc =3D pc + 12; pc < opc; pc +=3D 2) > + for (opc =3D pc + 12; pc < opc && pc < limit_pc; pc +=3D 2) > { > inst =3D read_memory_integer (pc, 2, byte_order); > if (IS_MOV_ARG_TO_IND_R14 (inst)) > @@ -681,7 +692,10 @@ sh_analyze_prologue (struct gdbarch *gdb > so, note that before returning the current pc. */ > inst =3D read_memory_integer (pc + 2, 2, byte_order); > if (IS_MOV_SP_FP (inst)) > - cache->uses_fp =3D 1; > + { > + cache->uses_fp =3D 1; > + after_last_frame_setup_insn =3D pc; > + } Shouldn't this perhaps be pc + 2? > break; > } > #if 0 /* This used to just stop when it found an instruction > @@ -693,61 +707,30 @@ sh_analyze_prologue (struct gdbarch *gdb > #endif > } >=20=20 > - return pc; > + return after_last_frame_setup_insn; > } >=20=20 > /* Skip any prologue before the guts of a function. */ >=20=20 > -/* Skip the prologue using the debug information. If this fails we'll > - fall back on the 'guess' method below. */ > -static CORE_ADDR > -after_prologue (CORE_ADDR pc) > -{ > - struct symtab_and_line sal; > - CORE_ADDR func_addr, func_end; > - > - /* If we can not find the symbol in the partial symbol table, then > - there is no hope we can determine the function's start address > - with this code. */ > - if (!find_pc_partial_function (pc, NULL, &func_addr, &func_end)) > - return 0; > - > - /* Get the line associated with FUNC_ADDR. */ > - sal =3D find_pc_line (func_addr, 0); > - > - /* There are only two cases to consider. First, the end of the source= line > - is within the function bounds. In that case we return the end of t= he > - source line. Second is the end of the source line extends beyond t= he > - bounds of the current function. We need to use the slow code to > - examine instructions in that case. */ > - if (sal.end < func_end) > - return sal.end; > - else > - return 0; > -} > - > static CORE_ADDR > sh_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc) > { > - CORE_ADDR pc; > + CORE_ADDR pc, sal_end, func_addr, func_end; > struct sh_frame_cache cache; > + const char *name; >=20=20 > - /* See if we can determine the end of the prologue via the symbol tabl= e. > - If so, then return either PC, or the PC after the prologue, whichev= er > - is greater. */ > - pc =3D after_prologue (start_pc); > - > - /* If after_prologue returned a useful address, then use it. Else > - fall back on the instruction skipping code. */ > - if (pc) > - return max (pc, start_pc); > + /* Try to find the extent of the function that contains PC. */ > + if (!find_pc_partial_function (start_pc, &name, &func_addr, &func_end)) > + return start_pc; Now start_pc is directly returned if find_pc_partial_function fails, without invoking sh_analyze_prologue. Is that right? >=20=20 > cache.sp_offset =3D -4; > - pc =3D sh_analyze_prologue (gdbarch, start_pc, (CORE_ADDR) -1, &cache,= 0); > - if (!cache.uses_fp) > - return start_pc; > + pc =3D sh_analyze_prologue (gdbarch, func_addr, func_end, &cache, 0); >=20=20 > - return pc; > + sal_end =3D skip_prologue_using_sal (gdbarch, start_pc); I had func_addr here, instead of start_pc. > + if (sal_end !=3D 0 && sal_end !=3D start_pc && sal_end < pc) > + return sal_end; > + else > + return pc; > } >=20=20 > /* The ABI says: Gr=C3=BC=C3=9Fe, Thomas --=-=-= Content-Type: application/pgp-signature Content-length: 489 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQEcBAEBAgAGBQJPPS+SAAoJENuKOtuXzphJ8YgH/0hdeGArxnBRIIED94Harmi3 GdOZG7IWNgdcmwkY5bRnxxwwYtqIW4amlA6TxcVvkw73S01+2377Q/1iHJ/jLqGE eJiIXx+3sB1vuN3cJ8ISqj0WNWPfXWRgYILn4qaZ4UGK/A7Nq3dXAOLPa5ePMllO JIirBMjgMtj+zqtwBJ02ZsvXMlMGMg1gNTX4fCT8MlJGT/3ZYfqkJ+bJY9bB90Dx jq7GRMN4qsKnxj7vuYmK4znfGDJbs9HlydMJAOw8EiPEDFujsc1XfPyyEMb5ll1I 1ke9d5OcYkFnrWaNUgdncovtE+wxCmvlBLwxJ1bwsujJnMwrD4wMiNA1UdQaSfw= =bfnT -----END PGP SIGNATURE----- --=-=-=--