From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cagney <ac131313@cygnus.com>
To: Daniel Jacobowitz <dmj+@andrew.cmu.edu>
Cc: gdb-patches@sources.redhat.com
Subject: Re: [rfa] mips heuristic_proc_start fix
Date: Thu, 12 Jul 2001 14:11:00 -0000
Message-id: <3B4E1277.8090003@cygnus.com>
References: <20010706112010.A5578@nevyn.them.org> <3B46030B.2010007@cygnus.com> <20010706113232.A6209@nevyn.them.org> <20010706114028.A6366@nevyn.them.org> <3B4D4FC6.6090003@cygnus.com> <20010712121358.A25739@nevyn.them.org>
X-SW-Source: 2001-07/msg00311.html

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