[PATCH] [SH] Prologue skipping if there is none
Thomas Schwinge
thomas@codesourcery.com
Thu Feb 16 16:59:00 GMT 2012
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 489 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20120216/f9d1bdf5/attachment.sig>
More information about the Gdb-patches
mailing list