This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [rfa] mips heuristic_proc_start fix
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