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

[Kevin Buettner <kevinb@redhat.com>]

On Fri, 02 Mar 2012 12:17:39 +0100
Thomas Schwinge <thomas@codesourcery.com> wrote:

> > > -      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?

What you say makes sense, but is still not my preference.  However, I
don't see this as that big of a deal and am willing to let it go.

I'd actually prefer not to see a comment about this.  IMO, sometimes
it's just better to let the code speak for itself.  A lot of times
comments become out of date and what's in the comment doesn't match up
with what the code is actually doing.  Such comments are worse than
having no comment at all because sometimes the reader will just accept
what the comment is saying.  And, then, if a mismatch if found, the
question becomes, "Is the code wrong, or is the comment wrong?"

> > 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.

Okay, thanks.

[...]

> > > @@ -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.

In the past, these limits were often calculated by hand coding a
"maximal prologue", i.e. an instruction sequence which made the
necessary frame adjustments (counting a potential alloca in the
function body), saved all possible callee-saved registers and wrote
all possible register arguments to the stack.  One simply counted the
instructions in this sequence and that was your limit.

A lot of prologue analyzers had this kind of limit in them at one
time.  Perhaps many of them still do; I haven't made a recent survey. 
The problem with this is that often times instructions from the
function body get moved into the prologue, especially for optimized
code.  But, these days, it's happening even for unoptimized code too. 
It's not clear to me if that constant is still large enough.

And sometimes, especially for leaf functions, there is no identifiable
prologue in which case the limit is much too high.

As I said before, my preference is to scan until some instruction is
reached that we know cannot be part of the prologue or until some other
limit is hit such as the address of the instruction pointer.  An
instruction which changes the control flow is often an excellent
place to stop.  Sometimes though, special provisions must be made
for accomodating PIC code.  In such a case, it's not uncommon for
there to be a JSR or some such to a nearby address just so that
the current PC value is known.  I don't know if this happens on sh
or not.

With all that said, if you're preserving existing functionality, I'm
not going to hold up your patch.  But enhancements which remove those
limits would be welcome.  (You may want to revisit the patch that I
posted earlier.)  Or, alternately, if you can find a compelling case
for why those limits should be preserved, then a comment describing
such would be appreciated too.

> > 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.

Yes, subject to my comments above.

> > 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).

Hmm.  Have you tried it when only minimal symbols are present?  (It is
useful to have this stuff working so that you can get decent stack
traces when only linker symbols are present.)

> 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.

Okay.  (You can check it in.)

Kevin