This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Hi! On Wed, 15 Feb 2012 16:59:07 -0700, Kevin Buettner <kevinb@redhat.com> wrote: > On Wed, 15 Feb 2012 07:54:13 -0700 > Kevin Buettner <kevinb@redhat.com> wrote: > > > On Wed, 15 Feb 2012 14:51:31 +0100 > > Thomas Schwinge <thomas@codesourcery.com> wrote: > > > > > 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. > > > > 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? > > 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: PASS -> FAIL: default/gdb.sum:gdb.base/store.exp: continue to add_charest 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_doublest 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_longest 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<n> for finish; return 2 structs-td-tf PASS -> FAIL: default/gdb.sum:gdb.base/structs.exp: advance to fun<n> for finish; return 2 structs-tf-td PASS -> FAIL: default/gdb.sum:gdb.base/structs.exp: advance to fun<n> for return; return 2 structs-td-tf PASS -> FAIL: default/gdb.sum:gdb.base/structs.exp: advance to fun<n> for return; return 2 structs-tf-td New FAIL: default/gdb.sum:gdb.threads/staticthreads.exp: Continue to main's call of sem_post (the program exited) New FAIL: default/gdb.sum:gdb.threads/staticthreads.exp: handle SIG32 helps (the program exited) Looking at the first one: (gdb) PASS: gdb.base/store.exp: var doublest l; print incremented l, expecting 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. Temporary breakpoint 10, add_charest (u=-1 '\377', v=32 ' ') at /scratch/tschwing/FM_sh-linux-gnu/src/gdb-mainline/gdb/testsuite/gdb.base/store.c:14 14 { (gdb) FAIL: gdb.base/store.exp: continue to add_charest gdb.base/store.c: 10 typedef signed char charest; 11 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. > > Index: sh-tdep.c > =================================================================== > 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 > > 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 = gdbarch_byte_order (gdbarch); > ULONGEST inst; > - CORE_ADDR opc; > + CORE_ADDR after_last_frame_setup_insn = pc; Save the original pc instead of having the caller do that. > + CORE_ADDR next_pc; > int offset; > int sav_offset = 0; > int r3_val = 0; > int reg, sav_reg = -1; > > - if (pc >= current_pc) > - return current_pc; > - > cache->uses_fp = 0; That sets cache->uses_fp to false even in case that the limit_pc (was: current_pc) was already reached. > - for (opc = pc + (2 * 28); pc < opc; pc += 2) > + > + for (;pc < limit_pc; pc = 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 = read_memory_unsigned_integer (pc, 2, byte_order); > + next_pc = pc + 2; > + > /* See where the registers will be saved to. */ > if (IS_PUSH (inst)) > { > cache->saved_regs[GET_SOURCE_REG (inst)] = cache->sp_offset; > cache->sp_offset += 4; > + after_last_frame_setup_insn = next_pc; > } > else if (IS_STS (inst)) > { > cache->saved_regs[PR_REGNUM] = cache->sp_offset; > cache->sp_offset += 4; > + after_last_frame_setup_insn = next_pc; > } > else if (IS_MACL_STS (inst)) > { > cache->saved_regs[MACL_REGNUM] = cache->sp_offset; > cache->sp_offset += 4; > + after_last_frame_setup_insn = 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 += -r3_val; > + after_last_frame_setup_insn = next_pc; > } > else if (IS_ADD_IMM_SP (inst)) > { > offset = ((inst & 0xff) ^ 0x80) - 0x80; > cache->sp_offset -= offset; > + if (offset < 0) > + after_last_frame_setup_insn = next_pc; > } > else if (IS_MOVW_PCREL_TO_REG (inst)) > { > @@ -626,6 +633,7 @@ sh_analyze_prologue (struct gdbarch *gdb > sav_reg = -1; > } > cache->sp_offset += sav_offset; > + after_last_frame_setup_insn = next_pc; > } > else if (IS_FPUSH (inst)) > { > @@ -637,17 +645,20 @@ sh_analyze_prologue (struct gdbarch *gdb > { > cache->sp_offset += 4; > } > + after_last_frame_setup_insn = next_pc; > } > else if (IS_MOV_SP_FP (inst)) > { > + CORE_ADDR opc; > cache->uses_fp = 1; > + after_last_frame_setup_insn = 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 += 2; > - for (opc = pc + 12; pc < opc; pc += 2) > + for (opc = pc + 12; pc < opc && pc < limit_pc; pc += 2) > { > inst = 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 = read_memory_integer (pc + 2, 2, byte_order); > if (IS_MOV_SP_FP (inst)) > - cache->uses_fp = 1; > + { > + cache->uses_fp = 1; > + after_last_frame_setup_insn = 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 > } > > - return pc; > + return after_last_frame_setup_insn; > } > > /* Skip any prologue before the guts of a function. */ > > -/* 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 = 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 the > - source line. Second is the end of the source line extends beyond the > - 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; > > - /* See if we can determine the end of the prologue via the symbol table. > - If so, then return either PC, or the PC after the prologue, whichever > - is greater. */ > - pc = 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? > > cache.sp_offset = -4; > - pc = sh_analyze_prologue (gdbarch, start_pc, (CORE_ADDR) -1, &cache, 0); > - if (!cache.uses_fp) > - return start_pc; > + pc = sh_analyze_prologue (gdbarch, func_addr, func_end, &cache, 0); > > - return pc; > + sal_end = skip_prologue_using_sal (gdbarch, start_pc); I had func_addr here, instead of start_pc. > + if (sal_end != 0 && sal_end != start_pc && sal_end < pc) > + return sal_end; > + else > + return pc; > } > > /* The ABI says: GrÃÃe, Thomas
Attachment:
pgp00000.pgp
Description: PGP signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |