Re: [PATCH] [SH] Prologue skipping if there is none

[Kevin Buettner <kevinb@redhat.com>]

Hi Thomas,

Please see my comments on your patch below...

On Thu, 01 Mar 2012 10:00:00 +0100
Thomas Schwinge <thomas@codesourcery.com> wrote:

> @@ -594,6 +590,7 @@ sh_analyze_prologue (struct gdbarch *gdb
>  		{
>  		  sav_reg = reg;
>  		  offset = (inst & 0xff) << 1;
> +		  /* TODO: check that this is a valid address.	*/
>  		  sav_offset =
>  		    read_memory_integer ((pc + 4) + offset, 2, byte_order);
>  		}

FIXME and TODO comments are generally frowned upon.  All too often,
they end up hanging about for many years.

If you want to check the validity of the address, you should call
something like target_read_memory() and examine the return status.

You may want to just keep that TODO comment in your tree or in
some other TODO list on the side.

> @@ -608,13 +605,15 @@ sh_analyze_prologue (struct gdbarch *gdb
>  		{
>  		  sav_reg = reg;
>  		  offset = (inst & 0xff) << 2;
> +		  /* TODO: check that this is a valid address.	*/

Ditto.

>  		  sav_offset =
>  		    read_memory_integer (((pc & 0xfffffffc) + 4) + offset,
>  					 4, byte_order);
>  		}
>  	    }
>  	}
> -      else if (IS_MOVI20 (inst))
> +      else if (IS_MOVI20 (inst)
> +	       && (pc + 2 < limit_pc))

For this, my preference would be for the limit check to
be moved a bit later on...


>          {
>  	  if (sav_reg < 0)
>  	    {
> @@ -623,14 +622,15 @@ sh_analyze_prologue (struct gdbarch *gdb
>  	        {
>  		  sav_reg = reg;
>  		  sav_offset = GET_SOURCE_REG (inst) << 16;
> -		  /* MOVI20 is a 32 bit instruction!  */
> -		  pc += 2;

I.e. put the test here and break if the limit has been exceeded.

Also, is there a good reason for moving the increment further on?  I
think it reads better to increment first and then fetch from the
pc instead of pc+2.

>  		  sav_offset
> -		    |= read_memory_unsigned_integer (pc, 2, byte_order);
> +		    |= read_memory_unsigned_integer (pc + 2, 2, byte_order);
>  		  /* Now sav_offset contains an unsigned 20 bit value.
>  		     It must still get sign extended.  */
>  		  if (sav_offset & 0x00080000)
>  		    sav_offset |= 0xfff00000;
> +
> +		  /* MOVI20 is a 32-bit instruction.  */
> +		  pc += 2;
>  		}
>  	    }
>  	}
> @@ -656,14 +656,16 @@ sh_analyze_prologue (struct gdbarch *gdb
>  	}
>        else if (IS_MOV_SP_FP (inst))
>  	{
> +	  pc += 2;
> +	  limit_pc = min (limit_pc, pc + (2 * 6));	/* NUMERO MYSTERIOSO */

Either get rid of that comment or put something meaningful in its place.
(I'd sort of like to see an explanation of how 2*6 was decided upon.
If it's historical and you don't know why, then just get rid of the
comment.)

> +
>  	  cache->uses_fp = 1;
>  	  /* At this point, only allow argument register moves to other
>  	     registers or argument register moves to @(X,fp) which are
>  	     moving the register arguments onto the stack area allocated
>  	     by a former add somenumber to SP call.  Don't allow moving
>  	     to an fp indirect address above fp + cache->sp_offset.  */
> -	  pc += 2;
> -	  for (opc = pc + 12; pc < opc; pc += 2)
> +	  for (; pc < limit_pc; pc += 2)
>  	    {
>  	      inst = read_memory_integer (pc, 2, byte_order);
>  	      if (IS_MOV_ARG_TO_IND_R14 (inst))
> @@ -686,7 +688,8 @@ sh_analyze_prologue (struct gdbarch *gdb
>  	    }
>  	  break;
>  	}
> -      else if (IS_JSR (inst))
> +      else if (IS_JSR (inst)
> +	       && (pc + 2 < limit_pc))

Again, I'd like to see that limit check moved further on.

The reason for this is that I'd like to have a case for the JSR
instruction.  I'd rather have the details regarding what might happen
if some limit is exceeded to be contained in the code implementing
that case.

(This is just my preference.  Some other maintainer may disagree with
me.)

>  	{
>  	  /* We have found a jsr that has been scheduled into the prologue.
>  	     If we continue the scan and return a pc someplace after this,
> @@ -716,13 +719,13 @@ sh_analyze_prologue (struct gdbarch *gdb
>  static CORE_ADDR
>  sh_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>  {
> -  CORE_ADDR post_prologue_pc, func_addr;
> +  CORE_ADDR post_prologue_pc, func_addr, func_end_addr, limit_pc;
>    struct sh_frame_cache cache;
>  
>    /* See if we can determine the end of the prologue via the symbol table.
>       If so, then return either PC, or the PC after the prologue, whichever
>       is greater.  */
> -  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
> +  if (find_pc_partial_function (pc, NULL, &func_addr, &func_end_addr))
>      {
>        post_prologue_pc = skip_prologue_using_sal (gdbarch, func_addr);
>        if (post_prologue_pc != 0)
> @@ -732,8 +735,20 @@ sh_skip_prologue (struct gdbarch *gdbarc
>    /* Can't determine prologue from the symbol table, need to examine
>       instructions.  */
>  
> +  /* Find an upper limit on the function prologue using the debug
> +     information.  If the debug information could not be used to provide
> +     that bound, then use an arbitrary large number as the upper bound.  */
> +  limit_pc = skip_prologue_using_sal (gdbarch, pc);
> +  if (limit_pc == 0)
> +    limit_pc = pc + (2 * 28);				/* NUMERO MYSTERIOSO */

I assume this is the limit that had been used before?  If so, just say
so in a comment - if you like, you can also state that you don't know
how this number was derived.

FWIW, I'm not very fond of doing it this way.  I think it'd be preferable
to make the prologue analyzer bail when it hits a branch, call, or
return instruction.  It shouldn't be too hard to encode these cases
in the prologue analyzer.

We do need some limit though.  I'm just concerned about debugging leaf
functions where that limit will put us into the next function.  (This
was one of the problems with my earlier patch - it didn't handle that
case.)

> +
> +  /* Do not allow limit_pc to be past the function end, if we know
> +     where that end is...  */
> +  if (func_end_addr != 0)
> +    limit_pc = min (limit_pc, func_end_addr);
> +
>    cache.sp_offset = -4;
> -  post_prologue_pc = sh_analyze_prologue (gdbarch, pc, (CORE_ADDR) -1, &cache, 0);
> +  post_prologue_pc = sh_analyze_prologue (gdbarch, pc, limit_pc, &cache, 0);
>    if (cache.uses_fp)
>      pc = post_prologue_pc;


Kevin