Re: [rfa] mips heuristic_proc_start fix

[Daniel Jacobowitz <dmj+@andrew.cmu.edu>]

On Thu, Jul 12, 2001 at 05:11:19PM -0400, Andrew Cagney wrote:
> > How's this instead?  Instead of checking for pc == 0, check for pc <
> > instlen.  If fence overflows, that's fine, because start_pc will be
> > less than fence; or I could explicitly check for that too.
> 
> 
> I'd do both.  I think it is better to spell out the intent of each thing 
> so that you can introduce a few invariants.

Right.

> > diff -u -r1.57 mips-tdep.c
> > --- mips-tdep.c 2001/07/12 17:34:33     1.57
> > +++ mips-tdep.c 2001/07/12 19:12:20
> > @@ -1497,19 +1497,19 @@
> >    int seen_adjsp = 0;
> >  
> >    pc = ADDR_BITS_REMOVE (pc);
> > -  start_pc = pc;
> > -  fence = start_pc - heuristic_fence_post;
> > -  if (start_pc == 0)
> > +  instlen = pc_is_mips16 (pc) ? MIPS16_INSTLEN : MIPS_INSTLEN;
> > +
> > +  if (pc < instlen)
> >      return 0;
> 
> 
> Suggest adding:
> 
> 	gdb_assert ((pc % instlen) == 0);

Nope!  We just read pc off the stack.  If the PC is legitimate in the
first place, none of these nasty loops are a problem.  My pc was 0x2;
that's bad, but not cause for gdb to die.  What'd you say to a warning
and early return?

> Should that also be ``pc <= instlen'' as otherwize:
> 
> 	pc - instlen - instlen
> 
> can underflow.  I suspect it depends on the for loop.

The for loop will check before instlen is subtracted the second time,
so that's OK.

> > +  start_pc = pc - instlen;
> > +  fence = start_pc - heuristic_fence_post;
> 
> 
> I think this should still have the underflow check as otherwize you're 
> not quite sure what fence is upto.

Sure.

> >    if (heuristic_fence_post == UINT_MAX
> >        || fence < VM_MIN_ADDRESS)
> >      fence = VM_MIN_ADDRESS;
> >  
> > -  instlen = pc_is_mips16 (pc) ? MIPS16_INSTLEN : MIPS_INSTLEN;
> > -
> 
> 
> gdb_assert (fence >= VM_MIN_ADDRESS);
> gdb_assert (start_pc >= instlen);
> 
> Hmm, what happens if VM_MIN_ADDRESS < instlen.

Well, VM_MIN_ADDRESS is just a constant 0x400000 right now (for
whatever reason...) so I'm not too concerned about that.  I suppose it
could change, though.  I don't really see the need for these asserts,
especially given that if fence < VM_MIN_ADDRESS we set it back to
VM_MIN_ADDRESS - or were you suggesting changing that?

> >    /* search back for previous return */
> > -  for (start_pc -= instlen;; start_pc -= instlen)
> > +  for (;; start_pc -= instlen)
> 
> 
> Er, if VM_MIN_ADDRESS == 0 (hence fence == 0) then this ain't going to work.

Oh, yes, that's true.  That's the problem with unsigned math :)  Just
doesn't behave intuitively.

> what a rats nest.  Would be better to change the for loop to:
> 
> 	for (; start_pc > fence; start_pc -= instlen;
> 
> reversing the exit condition?
> 
> Alternativly, should fence be guarenteed to be >= instlen.

Let me think about this and try to come up with something cleaner for
this function.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer