Re: [Patch, avr] Fix PR 13697 without KEEPing .data

[Senthil Kumar Selvaraj <senthil_kumar dot selvaraj at atmel dot com>]

On Wed, Jul 02, 2014 at 07:59:04PM +0200, Georg-Johann Lay wrote:
> Senthil Kumar Selvaraj schrieb:
> >The fix for PR 13697
> >(https://sourceware.org/bugzilla/show_bug.cgi?id=13697) added a
> >KEEP(*(.data)) to prevent the .bss section from getting an incorrect
> >VMA.
> >
> >The patch  fixes the problem, but it also prevents the linker from gc'ing
> >.data sections. It also only keeps sections named
> >.data, so -fdata-sections could potentially cause the fix to break (it
> >works right now because the assembler generates an empty .data section).
> >
> >The following patch fixes the problem by explicitly setting the
> >.bss section's VMA to the end of .data's VMA. It kinda looks redundant, as
> >both .data and .bss are assigned to the same region, but is needed because
> >the region's ORIGIN is hardcoded whereas the start of .data/.bss varies
> >by device (and is passed along as -Tdata=<address> by the compiler
> >driver).
> >
> >Does this look ok? I verified that the testcase mentioned in the
> >bug report works fine.
> 
> Can you explain how it works? Just by mentioning ADDR(.data)?
> 
Johann,
 
 ADDR(.data) gives the VMA of the .data section, and adding that to the 
 size of the .data section should evaluate to a VMA value that
 represents the end of .data. The patch sets the VMA of .bss to that
 value.

 The -Tdata option only affects the output of the .data section, not the
 data region. Without your KEEP(data) fix, this caused problems if the
 .data section was empty - .bss got placed at the (hardcoded) start of 
 the data region, and that would be incorrect for any device that has
 data memory mapped to a different address.

 With the KEEP fix, the linker did not remove .data, -Tdata placed data
 at the right address and .bss followed it (because it was assigned to
 the same memory region as .data).

 With this fix, .bss is explicitly set to start at end of .data (instead
 of just being assigned to the same memory region as .data). -Tdata sets
 the start of (potentially empty) .data section, and .bss now follows
 it, instead of defaulting to the hardcoded memory region origin.


 BTW, if you are polishing the linker script, you could also take care of
> some missing .gnu.linkonce sections? At least .t, .r and .b are missing.
> 

I'll submit a separate patch for this.

Regards
Senthil

> Johann
> 
> >ld/ChangeLog
> >
> >2014-07-02  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
> >
> >	* scripttempl/avr.sc: Remove KEEP for .data and
> >	force .bss VMA to end of .data VMA.
> >
> >diff --git ld/scripttempl/avr.sc ld/scripttempl/avr.sc
> >index 10714e1..d356b71 100644
> >--- ld/scripttempl/avr.sc
> >+++ ld/scripttempl/avr.sc
> >@@ -166,10 +166,7 @@ SECTIONS
> >   .data        ${RELOCATING-0} :
> >   {
> >     ${RELOCATING+ PROVIDE (__data_start = .) ; }
> >-    /* --gc-sections will delete empty .data. This leads to wrong start
> >-       addresses for subsequent sections because -Tdata= from the command
> >-       line will have no effect, see PR13697.  Thus, keep .data  */
> >-    KEEP (*(.data))    +    *(.data)
> >     ${RELOCATING+ *(.data*)}
> >     *(.rodata)  /* We need to include .rodata here if gcc is used */
> >     ${RELOCATING+ *(.rodata*)} /* with -fdata-sections.  */
> >@@ -179,7 +176,7 @@ SECTIONS
> >     ${RELOCATING+ PROVIDE (__data_end = .) ; }
> >   } ${RELOCATING+ > data ${RELOCATING+AT> text}}
> >-  .bss ${RELOCATING-0} :${RELOCATING+ AT (ADDR (.bss))}
> >+  .bss ${RELOCATING+ ADDR(.data) + SIZEOF (.data)} ${RELOCATING-0} :${RELOCATING+ AT (ADDR (.bss))}
> >   {
> >     ${RELOCATING+ PROVIDE (__bss_start = .) ; }
> >     *(.bss)
> >