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

[Georg-Johann Lay]

On 29.06.2017 14:43, Nick Clifton wrote:
> 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

Fixed.

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

Fixed.

There is no difference between "no-previous chunk" and "chunk Done",
that is what I tried to express.


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

Fixed.

>> +    {
>> +      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.

now using

     sprintf (xname, "%s.%03u", name, (++suffix) % 1000);

The biggest AVRs don't have more than 127 vector entries, so
a module with such many ISR means the compiler went nuts...

The suffix is actually only seen in GAS listings as the symbols are
soon transformed into O_constant expressions.  These symbols might
even have all the same name actually.  Hence if the above is too much
obfuscation, using the original "name" would also do.

Johann

> 
> 
> Cheers
>    Nick
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bin-pr21683-gccisr-v3.diff
Type: text/x-patch
Size: 35262 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20170629/b61045b8/attachment.bin>