Re: [PATCH] New ia64 @slotcount pseudo func

[Alan Modra <amodra at bigpond dot net dot au>]

On Wed, Oct 28, 2009 at 01:24:08PM -0700, Douglas B Rupp wrote:
> +	case PSEUDO_FUNC_EXPR:
> +	  end = input_line_pointer;

This seems an unnecessary assignment to "end".  Not your bug since
you're copying existing code, but no need to make things worse.

> +	  if (*nextcharP != '(')
> +	    {
> +	      as_bad (_("Expected '('"));
> +	      break;
> +	    }
> +	  /* Skip '('.  */
> +	  ++input_line_pointer;

*input_line_pointer++ = '('; otherwise we're left with a NUL in the
input line which may result in unusual error messages.

> +
> +	  /* The only expression pseudo func at this time is @slotcount
> +	     which takes an expression guaranteed to look like (beg-end),
> +	     there beg and end are addresses, and figure out how many
> +	     Itanium instruction slots separate the addresses.  The value
> +	     is needed for a couple of VMS debugger attributes relating to
> +	     prologue and epilogue size.  */
> +
> +	  if (strcmp (pseudo_func[idx].name, "slotcount") == 0)

Why strcmp if you know that it must be "slotcount"?

> +	    {
> +	      char *name;
> +	      char c;
> +	      symbolS *symbolPend, *symbolPbeg;
> +	      int end, beg, val;
> +
> +	      name = input_line_pointer;
> +	      c = get_symbol_end ();

Check for comma here?

> +	      symbolPend = symbol_find_or_make (name);
> +
> +	      ++input_line_pointer;

*input_line_pointer++ = c;

> +	      name = input_line_pointer;
> +	      c = get_symbol_end ();
> +	      symbolPbeg = symbol_find_or_make (name);
> +
> +	      /* Calculate the number of instruction slots between the two
> +		 labels.  They are guaranteed to be in the same (.text)
> +		 section.  */
> +	      end = S_GET_VALUE (symbolPend);
> +	      beg = S_GET_VALUE (symbolPbeg);
> +
> +	      val = (((end & -16) - (beg & -16)) / 16 * 3)
> +		    + (end & 15)
> +		    - (beg & 15);
> +
> +	      e->X_op = O_constant;
> +	      e->X_add_number = val;
> +
> +	      /* Put the ')' back for later error checking.  */
> +	      *input_line_pointer = ')';

You should be writing back "c".  Best done immediately after you're
finished with "name".

> +	    }
> +	  else
> +            {
> +	      /* Do something by default which is not unexpected.  */
> +	      expression (e);
> +	    }
> +
> +	  if (*input_line_pointer != ')')
> +	    {
> +	      as_bad (_("Missing ')'"));
> +	      goto done1;
> +	    }
> +	  /* Skip ')'.  */
> +	  ++input_line_pointer;
> +	done1:

I'm sure you can rewrite this without the goto.

> +	  *nextcharP = *input_line_pointer;
> +	  break;
> +
>  	case PSEUDO_FUNC_CONST:
>  	  e->X_op = O_constant;
>  	  e->X_add_number = pseudo_func[idx].u.ival;


-- 
Alan Modra
Australia Development Lab, IBM