[rfa] mips heuristic_proc_start fix

[rfa] mips heuristic_proc_start fix

[Daniel Jacobowitz]

This one was fun to track down... I've been getting corrupt PC values off
the stack in backtraces, something which needs to be fixed elsewhere.  The
interesting thing is that the PC I was reading was 0x2.  Remember that
CORE_ADDR on MIPS is an unsigned 64-bit quantity.

There's some wrapping bugs here.  If start_pc - heuristic_fence_post wraps,
it may be greater than VM_MIN_ADDRESS, but it will also be greater than
start_pc, so we will fail - not ideal, maybe, but safe.  On the other hand,
if start_pc == 0x2, and heuristic_fence_post == 0, then fence gets set to
VM_MIN_ADDRESS (0x400000 here).  start_pc -= instlen is 0xfffffffffffffffe. 
That's above the fencepost!  Oops.

OK to commit?

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

2001-07-06  Daniel Jacobowitz  <drow@mvista.com>

	* mips-tdep.c (heuristic_proc_start):  Avoid long loop if start_pc
	is corrupt.

Index: mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.56
diff -u -r1.56 mips-tdep.c
--- mips-tdep.c	2001/07/06 05:35:17	1.56
+++ mips-tdep.c	2001/07/06 18:03:57
@@ -1506,6 +1506,13 @@
       || fence < VM_MIN_ADDRESS)
     fence = VM_MIN_ADDRESS;
 
+  if (start_pc < fence)
+    {
+      warning ("Warning: GDB can't find the start of the function at 0x%s (wraparound).",
+	       paddr_nz (pc));
+      return 0;
+    }
+
   instlen = pc_is_mips16 (pc) ? MIPS16_INSTLEN : MIPS_INSTLEN;
 
   /* search back for previous return */

Re: [rfa] mips heuristic_proc_start fix

[Andrew Cagney]

> This one was fun to track down... I've been getting corrupt PC values off
> the stack in backtraces, something which needs to be fixed elsewhere.  The
> interesting thing is that the PC I was reading was 0x2.  Remember that
> CORE_ADDR on MIPS is an unsigned 64-bit quantity.


You mean ***SIGNED** right?

	Andrew

Re: [rfa] mips heuristic_proc_start fix

[Daniel Jacobowitz]

On Fri, Jul 06, 2001 at 02:27:23PM -0400, Andrew Cagney wrote:
> > This one was fun to track down... I've been getting corrupt PC values off
> > the stack in backtraces, something which needs to be fixed elsewhere.  The
> > interesting thing is that the PC I was reading was 0x2.  Remember that
> > CORE_ADDR on MIPS is an unsigned 64-bit quantity.
> 
> 
> You mean ***SIGNED** right?

Glarg.  Yes, it's signed, of course!  Let me peer at this for a moment
and see why the math was coming out wrong; the patch is correct, but
now it's not quite as obvious to me why :)

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

Re: [rfa] mips heuristic_proc_start fix

[Daniel Jacobowitz]

On Fri, Jul 06, 2001 at 11:32:32AM -0700, Daniel Jacobowitz wrote:
> On Fri, Jul 06, 2001 at 02:27:23PM -0400, Andrew Cagney wrote:
> > > This one was fun to track down... I've been getting corrupt PC values off
> > > the stack in backtraces, something which needs to be fixed elsewhere.  The
> > > interesting thing is that the PC I was reading was 0x2.  Remember that
> > > CORE_ADDR on MIPS is an unsigned 64-bit quantity.
> > 
> > 
> > You mean ***SIGNED** right?
> 
> Glarg.  Yes, it's signed, of course!  Let me peer at this for a moment
> and see why the math was coming out wrong; the patch is correct, but
> now it's not quite as obvious to me why :)

No, I stand uncorrected:

(gdb) ptype CORE_ADDR
type = long long unsigned int

It's sign extended as if it were signed, but the type itself is not!


typedef BFD_HOST_U_64_BIT bfd_vma;
typedef BFD_HOST_64_BIT bfd_signed_vma;

and

typedef bfd_vma CORE_ADDR;

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

Re: [rfa] mips heuristic_proc_start fix

[Andrew Cagney]

>> 
>> Glarg.  Yes, it's signed, of course!  Let me peer at this for a moment
>> and see why the math was coming out wrong; the patch is correct, but
>> now it's not quite as obvious to me why :)
> 
> 
> No, I stand uncorrected:


Sorry, yes, my bad.

> +  if (start_pc < fence)
> +    {
> +      warning ("Warning: GDB can't find the start of the function at 0x%s (wraparound).",
> +	       paddr_nz (pc));
> +      return 0;
> +    }


Hmm, with this patch UINT_MAX would give a fence of VM_MIN_ADDRESS yet 
UINT_MAX-1 would give the above warning.

Could I suggest instead something like:

   pc = ADDR_BITS_REMOVE (pc);
   start_pc = pc;
   if (start_pc >= heuristic_fence_post)
     fence = start_pc - heuristic_fence_post;
   else
     fence = 0;
   if (start_pc == 0)
     return 0;

   if (heuristic_fence_post == UINT_MAX
       || fence < VM_MIN_ADDRESS)
     fence = VM_MIN_ADDRESS;

The test being moved to before the assignment to make what is happening 
more transparent.

	enjoy,
		Andrew

Re: [rfa] mips heuristic_proc_start fix

[Daniel Jacobowitz]

On Thu, Jul 12, 2001 at 03:20:38AM -0400, Andrew Cagney wrote:
> Hmm, with this patch UINT_MAX would give a fence of VM_MIN_ADDRESS yet 
> UINT_MAX-1 would give the above warning.
> 
> Could I suggest instead something like:
> 
>    pc = ADDR_BITS_REMOVE (pc);
>    start_pc = pc;
>    if (start_pc >= heuristic_fence_post)
>      fence = start_pc - heuristic_fence_post;
>    else
>      fence = 0;
>    if (start_pc == 0)
>      return 0;
> 
>    if (heuristic_fence_post == UINT_MAX
>        || fence < VM_MIN_ADDRESS)
>      fence = VM_MIN_ADDRESS;
> 
> The test being moved to before the assignment to make what is happening 
> more transparent.

I agree it would be clearer to check for overflow, but just that
won't solve the problem.  If start_pc is 2 and instlen is 4, it doesn't
matter what fence gets set to.  First time through the for loop we
decrement start_pc by instlen, and that's where the overflow is.

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.

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;
+  start_pc = pc - instlen;
+  fence = start_pc - heuristic_fence_post;
 
   if (heuristic_fence_post == UINT_MAX
       || fence < VM_MIN_ADDRESS)
     fence = VM_MIN_ADDRESS;
 
-  instlen = pc_is_mips16 (pc) ? MIPS16_INSTLEN : MIPS_INSTLEN;
-
   /* search back for previous return */
-  for (start_pc -= instlen;; start_pc -= instlen)
+  for (;; start_pc -= instlen)
     if (start_pc < fence)
       {
 	/* It's not clear to me why we reach this point when


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

Re: [rfa] mips heuristic_proc_start fix

[Andrew Cagney]

> 
> I agree it would be clearer to check for overflow, but just that
> won't solve the problem.  If start_pc is 2 and instlen is 4, it doesn't
> matter what fence gets set to.  First time through the for loop we
> decrement start_pc by instlen, and that's where the overflow is.


Yes, I looked at this, played with it a little and gave up.  It is nasty :-)


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


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

Should that also be ``pc <= instlen'' as otherwize:

	pc - instlen - instlen

can underflow.  I suspect it depends on the for loop.

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


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


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

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.

	Andrew

>      if (start_pc < fence)
>        {
>  	/* It's not clear to me why we reach this point when
> 
> 
> -- Daniel Jacobowitz Carnegie Mellon University MontaVista Software Debian GNU/Linux Developer
> 

Re: [rfa] mips heuristic_proc_start fix

[Daniel Jacobowitz]

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

Re: [rfa] mips heuristic_proc_start fix

[Andrew Cagney]

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


(Technically, before the call to ADDR_BITS_REMOVE() both 3 and 1 are 
valid, after 2 is valid - MIPS16 code.  :-)

I think just having the one message is easier.  As the comment notes:

                 /* This actually happens frequently in embedded
                    development, when you first connect to a board
                    and your stack pointer and pc are nowhere in
                    particular.  This message needs to give people
                    in that situation enough information to
                    determine that it's no big deal.  */

so any other message would need to be just as long.

	Andrew