Re: [rfa] mips heuristic_proc_start fix

[Andrew Cagney <ac131313 at cygnus dot com>]

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