Re: [PATCH] [SH] Prologue skipping if there is none

Thomas Schwinge
2012-03-02T11:18:16Z

[-- Attachment #1: Type: text/plain, Size: 11710 bytes --]

Hi Kevin!

Thanks for reviewing; here are my comments.

On Thu, 1 Mar 2012 17:18:47 -0700, Kevin Buettner <kevinb@redhat.com> wrote:
> On Thu, 01 Mar 2012 10:00:00 +0100
> Thomas Schwinge <thomas@codesourcery.com> wrote:
>
> > @@ -594,6 +590,7 @@ sh_analyze_prologue (struct gdbarch *gdb
> > {
> > sav_reg = reg;
> > offset = (inst & 0xff) << 1;
> > + /* TODO: check that this is a valid address. */
> > sav_offset =
> > read_memory_integer ((pc + 4) + offset, 2, byte_order);
> > }
>
> FIXME and TODO comments are generally frowned upon. All too often,
> they end up hanging about for many years.

> You may want to just keep that TODO comment in your tree or in
> some other TODO list on the side.

Hmm, I don't agree. I think it's better to have such comments in a
central place, instead of each developer having their own set of them in
their own files. I do agree that source code comments are not useful for
more *general* ``work to be done'', but this is a very local issue, where
the comment applies directly to the next line. Anyway, I'm not the one
to set the rules here; I've taken these out.

> > - else if (IS_MOVI20 (inst))
> > + else if (IS_MOVI20 (inst)
> > + && (pc + 2 < limit_pc))
>
> For this, my preference would be for the limit check to
> be moved a bit later on...
>
>
> > {
> > if (sav_reg < 0)
> > {
> > @@ -623,14 +622,15 @@ sh_analyze_prologue (struct gdbarch *gdb
> > {
> > sav_reg = reg;
> > sav_offset = GET_SOURCE_REG (inst) << 16;
> > - /* MOVI20 is a 32 bit instruction! */
> > - pc += 2;
>
> I.e. put the test here and break if the limit has been exceeded.

Here is my reasoning for moving the check early: for having a valid
movi20 instruction at pc (and thus this if-case to match), it is required
that four bytes can be read, that is, the words at pc as well as pc + 2
are within the limit. If that is not the case (because the limit is
hit), we can't analyze this as a movi20 instruction. Also, no other
if-case can then match anyway (if the movi20 one had matched here), and
we will drop out of the loop. Does that make sense now? Would a comment
clarify this?

> Also, is there a good reason for moving the increment further on? I
> think it reads better to increment first and then fetch from the
> pc instead of pc+2.

> > sav_offset
> > - |= read_memory_unsigned_integer (pc, 2, byte_order);
> > + |= read_memory_unsigned_integer (pc + 2, 2, byte_order);
> > /* Now sav_offset contains an unsigned 20 bit value.
> > It must still get sign extended. */
> > if (sav_offset & 0x00080000)
> > sav_offset |= 0xfff00000;
> > +
> > + /* MOVI20 is a 32-bit instruction. */
> > + pc += 2;
> > }

This (non-functional) change was my attempt to make the code easier to
grasp: first handle the full 32-bit instruction (the part at pc, and the
one at pc + 2), then advance over it. I don't feel strongly about this;
I backed it out.

> > @@ -656,14 +656,16 @@ sh_analyze_prologue (struct gdbarch *gdb
> > }
> > else if (IS_MOV_SP_FP (inst))
> > {
> > + pc += 2;
> > + limit_pc = min (limit_pc, pc + (2 * 6)); /* NUMERO MYSTERIOSO */
>
> Either get rid of that comment or put something meaningful in its place.
> (I'd sort of like to see an explanation of how 2*6 was decided upon.
> If it's historical and you don't know why, then just get rid of the
> comment.)

> > - pc += 2;
> > - for (opc = pc + 12; pc < opc; pc += 2)
> > + for (; pc < limit_pc; pc += 2)

I don't like silently hiding constant values (that nobody knows and/or
researched where they're coming from). What I calculate as 2 * 6 was
hidden in the value 12 quoted above. Anyway, I added a bit of a comment.

> > - else if (IS_JSR (inst))
> > + else if (IS_JSR (inst)
> > + && (pc + 2 < limit_pc))
>
> Again, I'd like to see that limit check moved further on.
>
> The reason for this is that I'd like to have a case for the JSR
> instruction. I'd rather have the details regarding what might happen
> if some limit is exceeded to be contained in the code implementing
> that case.
>
> (This is just my preference. Some other maintainer may disagree with
> me.)

In this case, I can be convinced; jsr is not a 32-bit instruction, so
this if-case may well trigger also if only two bytes are within the
limit. But, if won't ever matter in practice, as no complying function
is going to end with an jsr and nothing after it. It may only be that we
have a jsr as the last instruction before we hit limit_pc, and in that
case things are a bit dubious anyway. I changed this.

> > @@ -732,8 +735,20 @@ sh_skip_prologue (struct gdbarch *gdbarc
> > /* Can't determine prologue from the symbol table, need to examine
> > instructions. */
> >
> > + /* Find an upper limit on the function prologue using the debug
> > + information. If the debug information could not be used to provide
> > + that bound, then use an arbitrary large number as the upper bound. */
> > + limit_pc = skip_prologue_using_sal (gdbarch, pc);
> > + if (limit_pc == 0)
> > + limit_pc = pc + (2 * 28); /* NUMERO MYSTERIOSO */
>
> I assume this is the limit that had been used before? If so, just say
> so in a comment - if you like, you can also state that you don't know
> how this number was derived.

Same as with the 2 * 6 case above. Also, a lot of other architectures'
tdep files that I looked at have similar comments, with ``Magic'' being a
very prominent one. We're now clearly improving upon that!

> FWIW, I'm not very fond of doing it this way.

(I'm assuming ``this'' is the 28 instructions limit.) This is what many
other architectures' tdep files are doing, too, and it triggers only as a
safeguard if we fail to find any other limit. Also, a similar thing is
used in sh_in_function_epilogue_p.

> I think it'd be preferable
> to make the prologue analyzer bail when it hits a branch, call, or
> return instruction. It shouldn't be too hard to encode these cases
> in the prologue analyzer.

I consider your suggestion to be an additional improvement, but still
think that my patch (more specifically here, the 28 instructions limit)
remains valid, too.

> We do need some limit though. I'm just concerned about debugging leaf
> functions where that limit will put us into the next function. (This
> was one of the problems with my earlier patch - it didn't handle that
> case.)

As I said, this limit is only a safeguard if everything else fails.
Before that, the end of the function will have tried to be determined
with the symbol table (find_pc_partial_function), or debug information
(skip_prologue_using_sal), which will typically trigger (but not in PLT
slots).

How's this version?

* sh-tdep.c (sh_skip_prologue): Provide an upper limit on the function
prologue to sh_analyze_prologue.
(sh_analyze_prologue): Make better use of such an upper limit, and
generally be more cautious about accessing memory.

Index: gdb/sh-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sh-tdep.c,v
retrieving revision 1.239
diff -u -p -r1.239 sh-tdep.c
--- gdb/sh-tdep.c 27 Feb 2012 16:40:48 -0000 1.239
+++ gdb/sh-tdep.c 2 Mar 2012 11:12:15 -0000
@@ -534,22 +534,18 @@ 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;
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;
- for (opc = pc + (2 * 28); pc < opc; pc += 2)
+ for (; pc < limit_pc; pc += 2)
{
inst = read_memory_unsigned_integer (pc, 2, byte_order);
/* See where the registers will be saved to. */
@@ -614,7 +610,8 @@ sh_analyze_prologue (struct gdbarch *gdb
}
}
}
- else if (IS_MOVI20 (inst))
+ else if (IS_MOVI20 (inst)
+ && (pc + 2 < limit_pc))
{
if (sav_reg < 0)
{
@@ -656,14 +653,17 @@ sh_analyze_prologue (struct gdbarch *gdb
}
else if (IS_MOV_SP_FP (inst))
{
+ pc += 2;
+ /* Don't go any further than six more instructions. */
+ limit_pc = min (limit_pc, pc + (2 * 6));
+
cache->uses_fp = 1;
/* 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 (; pc < limit_pc; pc += 2)
{
inst = read_memory_integer (pc, 2, byte_order);
if (IS_MOV_ARG_TO_IND_R14 (inst))
@@ -695,10 +695,13 @@ sh_analyze_prologue (struct gdbarch *gdb
jsr, which will be very confusing. Most likely the next
instruction is going to be IS_MOV_SP_FP in the delay slot. If
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;
- break;
+ if (pc + 2 < limit_pc)
+ {
+ inst = read_memory_integer (pc + 2, 2, byte_order);
+ if (IS_MOV_SP_FP (inst))
+ cache->uses_fp = 1;
+ }
+ break;
}
#if 0 /* This used to just stop when it found an instruction
that was not considered part of the prologue. Now,
@@ -716,13 +719,13 @@ sh_analyze_prologue (struct gdbarch *gdb
static CORE_ADDR
sh_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
{
- CORE_ADDR post_prologue_pc, func_addr;
+ CORE_ADDR post_prologue_pc, func_addr, func_end_addr, limit_pc;
struct sh_frame_cache cache;

/* 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. */
- if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
+ if (find_pc_partial_function (pc, NULL, &func_addr, &func_end_addr))
{
post_prologue_pc = skip_prologue_using_sal (gdbarch, func_addr);
if (post_prologue_pc != 0)
@@ -732,8 +735,21 @@ sh_skip_prologue (struct gdbarch *gdbarc
/* Can't determine prologue from the symbol table, need to examine
instructions. */

+ /* Find an upper limit on the function prologue using the debug
+ information. If the debug information could not be used to provide
+ that bound, then use an arbitrary large number as the upper bound. */
+ limit_pc = skip_prologue_using_sal (gdbarch, pc);
+ if (limit_pc == 0)
+ /* Don't go any further than 28 instructions. */
+ limit_pc = pc + (2 * 28);
+
+ /* Do not allow limit_pc to be past the function end, if we know
+ where that end is... */
+ if (func_end_addr != 0)
+ limit_pc = min (limit_pc, func_end_addr);
+
cache.sp_offset = -4;
- post_prologue_pc = sh_analyze_prologue (gdbarch, pc, (CORE_ADDR) -1, &cache, 0);
+ post_prologue_pc = sh_analyze_prologue (gdbarch, pc, limit_pc, &cache, 0);
if (cache.uses_fp)
pc = post_prologue_pc;