Re: [patch,gas,avr] PR21683: Add new pseudo instruction __gcc_isr

[Nick Clifton <nickc at redhat dot com>]

Hi Georg-Johann,

  In general I think that the patch is OK, but there are a few small niggles
  that I would like to see fixed:

> +	chunk 1 (Prologue) or chunk 2 (Epilogue).  Funktions might come
> +	without epilogue or with more than one epilogue, and even code
> +	located statically after the last epologue might belong to a function.

Typos: Funktions and epologue


> +static struct
> +{
> +  /* Previous __gcc_isr chunk (one of the enums above or 0),
> +     and it's location for diagnostics.  */
> +  int prev_chunk;

Umm - 0 is a valid value for the enum you are using.  Either the
enum should not contain 0, or else a different value should be used
to indicate no-previous-chunk, or else the meaning of this field should
be changed.


> +  if (ISR_CHUNK_Done != avr_isr.prev_chunk
> +      && name[0] == '_' && name[1] == '_'
> +      && 0 == strcmp (name, "__gcc_isr.n_pushed"))

Why do you bother checking name[0] and name[1] ?  Unless speed of assembly
is a real issue, you could just simplify the code and drop these two tests.


> +    {
> +      if (!avr_isr.sym_n_pushed)
> +	{
> +	  static int suffix;
> +	  char xname[30];
> +	  sprintf (xname, "%s.%u", name, ++suffix);

Paranoia - 30 bytes may not be enough for a pathological input file which
contains 2^31 nested symbol pushes...  Using snprintf would catch this problem.


Cheers
  Nick