Re: Pre-Release binutils 2.17; [PATCH] Request for inserting bug-fix prior to releasing 2.17.

Re: Pre-Release binutils 2.17; [PATCH] Request for inserting bug-fix prior to releasing 2.17.

[Björn Haase]

Hi,

the present CVS version has a problem with the avr target when dealing with 
immediate values for program memory related relocs. Presently, gas 
seg-faults. The attached patch fixes the most immediate issue and would be 
IMO worth being applied prior to shipping 2.17.

The bugfix is also readily included in the (long) patch I have posted some 
days ago for inclusion in mainline. This large patch, however should probably 
wait for 2.18 since it implements quite a number of possibly harmful changes 
that justify more testing.

I'd, thus, ask and suggest to apply this patch only at the 2.17 branch.

Bjoern.


2006-05-10  Bjoern Haase  <bjoern.m.haase@web.de>

        * gas/config/tc-avr.h
        TC_VALIDATE_FIX: handle case of immediate constant values.


--- tc-avr.h    5 May 2006 17:46:47 -0000       1.12
+++ tc-avr.h    15 May 2006 21:28:08 -0000
@@ -127,12 +127,13 @@
    However, there is no serious performance penilty when making the linker
    make the fixup work.  */
 #define TC_VALIDATE_FIX(FIXP,SEG,SKIP)                      \
-  if ( FIXP->fx_r_type == BFD_RELOC_AVR_7_PCREL             \
-    || FIXP->fx_r_type == BFD_RELOC_AVR_13_PCREL            \
-    || FIXP->fx_r_type == BFD_RELOC_AVR_LO8_LDI_PM          \
-    || FIXP->fx_r_type == BFD_RELOC_AVR_HI8_LDI_PM          \
-    || FIXP->fx_r_type == BFD_RELOC_AVR_HH8_LDI_PM          \
-    || FIXP->fx_r_type == BFD_RELOC_AVR_16_PM)              \
+  if ((FIXP->fx_r_type == BFD_RELOC_AVR_7_PCREL             \
+       || FIXP->fx_r_type == BFD_RELOC_AVR_13_PCREL         \
+       || FIXP->fx_r_type == BFD_RELOC_AVR_LO8_LDI_PM       \
+       || FIXP->fx_r_type == BFD_RELOC_AVR_HI8_LDI_PM       \
+       || FIXP->fx_r_type == BFD_RELOC_AVR_HH8_LDI_PM       \
+       || FIXP->fx_r_type == BFD_RELOC_AVR_16_PM)           \
+      && (FIXP->fx_addsy))                                  \
     {                                                       \
       goto SKIP;                                            \
     }

Re: Pre-Release binutils 2.17; [PATCH] Request for inserting bug-fix prior to releasing 2.17.

[Nick Clifton]

Hi Björn,

> 2006-05-10  Bjoern Haase  <bjoern.m.haase@web.de>
> 
>         * gas/config/tc-avr.h
>         TC_VALIDATE_FIX: handle case of immediate constant values.

I have applied this patch to the mainline and 2.17 branch.

A few small points:

   * The patch would not apply to the 2.17 branch without bringing in
     your earlier patch to define the TC_VALIDATE_FIX macro, so I did
     that as well.

   * Your ChangeLog entry is not correctly formatted.  The filename
     should be relative to the location of the ChangeLog.  So, since this
     ChangeLog is in the gas/ directory, the filename should be
     config/tc-avr.h not gas/config/tc-avr.h.  Also the context for the
     change (in this case the TC_VALIDATE_FIX macro) should, if possible,
     be include on the same line and enclosed in parentheses.  Finally
     the description of the change should be formatted as a proper
     English sentence, complete with a capital initial letter.  Ideally
     the description should allow the reader to deduce the contents of
     the patch without actually seeing the changed code.

     Thus I reformatted your ChangeLog entry as:

     * config/tc-avr.h (TC_VALIDATE_FIX): Allow fixups for immediate
     constant values.

   * Finally, you mentioned that without this fix the assembler could
     encounter a seg-fault.  It would be very helpful if you could create
     a small gas testsuite case for this, so that we can be sure that
     this problem does not reappear in the future.

Cheers
   Nick

Re: Pre-Release binutils 2.17; [PATCH] Request for inserting bug-fix prior to releasing 2.17.

[Björn Haase]

Nick Clifton wrote on Dienstag, 16. Mai 2006 10:36 :
> Hi Björn,
>
> > 2006-05-10  Bjoern Haase  <bjoern.m.haase@web.de>
> >
> >         * gas/config/tc-avr.h
> >         TC_VALIDATE_FIX: handle case of immediate constant values.
>
> I have applied this patch to the mainline and 2.17 branch.
>
> A few small points:
>
>    * Your ChangeLog entry is not correctly formatted.  The filename
>      should be relative to the location of the ChangeLog.  So, since this
>      ChangeLog is in the gas/ directory, the filename should be
>      config/tc-avr.h not gas/config/tc-avr.h.  Also the context for the
>      change (in this case the TC_VALIDATE_FIX macro) should, if possible,
>      be include on the same line and enclosed in parentheses.  Finally
>      the description of the change should be formatted as a proper
>      English sentence, complete with a capital initial letter.  Ideally
>      the description should allow the reader to deduce the contents of
>      the patch without actually seeing the changed code.
>
>      Thus I reformatted your ChangeLog entry as:
>
>      * config/tc-avr.h (TC_VALIDATE_FIX): Allow fixups for immediate
>      constant values.
Understood. I will be reworking the change log entries also for the large 
pending (avr6) patch. I'll wait for the remarks of the avr maintainers and 
provide a possibly corrected patch and new change log entries.

>    * Finally, you mentioned that without this fix the assembler could
>      encounter a seg-fault.  It would be very helpful if you could create
>      a small gas testsuite case for this, so that we can be sure that
>      this problem does not reappear in the future.
I'll have a look at how the testsuite operates and plan to add the test to the 
avr6 patch so that there is less work for those having CVS write permissions.

Thank's for the fast reply.

Bjoern.