Patchwork [U-Boot,1/4] ARM: fix u-boot.lds for -ffunction-sections/-fdata-sections

login
register
mail settings
Submitter Stephen Warren
Date Oct. 16, 2012, 9:50 p.m.
Message ID <1350424209-11186-1-git-send-email-swarren@wwwdotorg.org>
Download mbox | patch
Permalink /patch/191898/
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Comments

Stephen Warren - Oct. 16, 2012, 9:50 p.m.
From: Stephen Warren <swarren@nvidia.com>

When -ffunction-sections or -fdata-section are used, symbols are placed
into sections such as .data.eserial1_device and .bss.serial_current.
Update the linker script to explicitly include these. Without this
change (at least with my gcc-4.5.3 built using crosstool-ng), I see that
the sections do end up being included, but __bss_end__ gets set to the
same value as __bss_start.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
This series fixes an SPL size overflow problem on Tegra. Tom Warren is
out on vacation until Oct 25th, so he certainly won't be able to review
this. Perhaps it could be applied directly to the ARM tree if enough
Tegra people ack the series?

Note that this series is not enough to make Tegra support work; either
you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call
serial_initialize() right before serial_init() in preloader_console_init()
or wait for Allen Martin to rework Tegra's SPL support using the common
SPL code.

 arch/arm/cpu/u-boot.lds |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
Simon Glass - Oct. 17, 2012, 11:58 p.m.
Hi Stephen,

On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> When -ffunction-sections or -fdata-section are used, symbols are placed
> into sections such as .data.eserial1_device and .bss.serial_current.
> Update the linker script to explicitly include these. Without this
> change (at least with my gcc-4.5.3 built using crosstool-ng), I see that
> the sections do end up being included, but __bss_end__ gets set to the
> same value as __bss_start.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> This series fixes an SPL size overflow problem on Tegra. Tom Warren is
> out on vacation until Oct 25th, so he certainly won't be able to review
> this. Perhaps it could be applied directly to the ARM tree if enough
> Tegra people ack the series?
>
> Note that this series is not enough to make Tegra support work; either
> you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call
> serial_initialize() right before serial_init() in preloader_console_init()
> or wait for Allen Martin to rework Tegra's SPL support using the common
> SPL code.

Are you going to submit a patch to enable function-sections, or is
that a separate discussion?

>
>  arch/arm/cpu/u-boot.lds |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index e49ca0c..ae04a6e 100644
> --- a/arch/arm/cpu/u-boot.lds
> +++ b/arch/arm/cpu/u-boot.lds
> @@ -35,7 +35,7 @@ SECTIONS
>         {
>                 __image_copy_start = .;
>                 CPUDIR/start.o (.text)
> -               *(.text)
> +               *(.text*)
>         }
>
>         . = ALIGN(4);
> @@ -43,14 +43,14 @@ SECTIONS
>
>         . = ALIGN(4);
>         .data : {
> -               *(.data)
> +               *(.data*)
>         }
>
>         . = ALIGN(4);
>
>         . = .;
>         __u_boot_cmd_start = .;
> -       .u_boot_cmd : { *(.u_boot_cmd) }
> +       .u_boot_cmd : { *(.u_boot_cmd*) }

I don't think this line is needed?

>         __u_boot_cmd_end = .;
>
>         . = ALIGN(4);
> @@ -65,7 +65,7 @@ SECTIONS
>
>         .dynsym : {
>                 __dynsym_start = .;
> -               *(.dynsym)
> +               *(.dynsym*)

Nor this one?

>         }
>
>         _end = .;
> @@ -81,7 +81,7 @@ SECTIONS
>
>         .bss __rel_dyn_start (OVERLAY) : {
>                 __bss_start = .;
> -               *(.bss)
> +               *(.bss*)
>                  . = ALIGN(4);
>                 __bss_end__ = .;
>         }
> --
> 1.7.0.4
>

Regards,
Simon
Stephen Warren - Oct. 18, 2012, 3:17 a.m.
On 10/17/2012 05:58 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> When -ffunction-sections or -fdata-section are used, symbols are placed
>> into sections such as .data.eserial1_device and .bss.serial_current.
>> Update the linker script to explicitly include these. Without this
>> change (at least with my gcc-4.5.3 built using crosstool-ng), I see that
>> the sections do end up being included, but __bss_end__ gets set to the
>> same value as __bss_start.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>> This series fixes an SPL size overflow problem on Tegra. Tom Warren is
>> out on vacation until Oct 25th, so he certainly won't be able to review
>> this. Perhaps it could be applied directly to the ARM tree if enough
>> Tegra people ack the series?
>>
>> Note that this series is not enough to make Tegra support work; either
>> you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call
>> serial_initialize() right before serial_init() in preloader_console_init()
>> or wait for Allen Martin to rework Tegra's SPL support using the common
>> SPL code.
> 
> Are you going to submit a patch to enable function-sections, or is
> that a separate discussion?

For the SPL on Tegra, those flags were already on; this patch fixes a
bug rather than prepares for new functionality.

>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds

>> -       .u_boot_cmd : { *(.u_boot_cmd) }
>> +       .u_boot_cmd : { *(.u_boot_cmd*) }
> 
> I don't think this line is needed?
> 
...
>> -               *(.dynsym)
>> +               *(.dynsym*)
> 
> Nor this one?

Possibly. I changed all the section names to be future-proof. Perhaps a
more targeted patch is warranted.
Albert ARIBAUD - Oct. 18, 2012, 8:36 p.m.
Hi Stephen,

On Wed, 17 Oct 2012 21:17:45 -0600, Stephen Warren
<swarren@wwwdotorg.org> wrote:

> On 10/17/2012 05:58 PM, Simon Glass wrote:
> > Hi Stephen,
> > 
> > On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >> From: Stephen Warren <swarren@nvidia.com>
> >>
> >> When -ffunction-sections or -fdata-section are used, symbols are placed
> >> into sections such as .data.eserial1_device and .bss.serial_current.
> >> Update the linker script to explicitly include these. Without this
> >> change (at least with my gcc-4.5.3 built using crosstool-ng), I see that
> >> the sections do end up being included, but __bss_end__ gets set to the
> >> same value as __bss_start.
> >>
> >> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> >> ---
> >> This series fixes an SPL size overflow problem on Tegra. Tom Warren is
> >> out on vacation until Oct 25th, so he certainly won't be able to review
> >> this. Perhaps it could be applied directly to the ARM tree if enough
> >> Tegra people ack the series?
> >>
> >> Note that this series is not enough to make Tegra support work; either
> >> you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call
> >> serial_initialize() right before serial_init() in preloader_console_init()
> >> or wait for Allen Martin to rework Tegra's SPL support using the common
> >> SPL code.
> > 
> > Are you going to submit a patch to enable function-sections, or is
> > that a separate discussion?
> 
> For the SPL on Tegra, those flags were already on; this patch fixes a
> bug rather than prepares for new functionality.
> 
> >> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> 
> >> -       .u_boot_cmd : { *(.u_boot_cmd) }
> >> +       .u_boot_cmd : { *(.u_boot_cmd*) }
> > 
> > I don't think this line is needed?
> > 
> ...
> >> -               *(.dynsym)
> >> +               *(.dynsym*)
> > 
> > Nor this one?
> 
> Possibly. I changed all the section names to be future-proof. Perhaps a
> more targeted patch is warranted.

Has this been (at least build-)tested on all boards which have
-ffunction-sections or -fdata-sections?

Amicalement,
Stephen Warren - Oct. 18, 2012, 8:58 p.m.
On 10/18/2012 02:36 PM, Albert ARIBAUD wrote:
> Hi Stephen,
> 
> On Wed, 17 Oct 2012 21:17:45 -0600, Stephen Warren
> <swarren@wwwdotorg.org> wrote:
> 
>> On 10/17/2012 05:58 PM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> When -ffunction-sections or -fdata-section are used, symbols are placed
>>>> into sections such as .data.eserial1_device and .bss.serial_current.
>>>> Update the linker script to explicitly include these. Without this
>>>> change (at least with my gcc-4.5.3 built using crosstool-ng), I see that
>>>> the sections do end up being included, but __bss_end__ gets set to the
>>>> same value as __bss_start.
>>>>
>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>>> ---
>>>> This series fixes an SPL size overflow problem on Tegra. Tom Warren is
>>>> out on vacation until Oct 25th, so he certainly won't be able to review
>>>> this. Perhaps it could be applied directly to the ARM tree if enough
>>>> Tegra people ack the series?
>>>>
>>>> Note that this series is not enough to make Tegra support work; either
>>>> you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call
>>>> serial_initialize() right before serial_init() in preloader_console_init()
>>>> or wait for Allen Martin to rework Tegra's SPL support using the common
>>>> SPL code.
>>>
>>> Are you going to submit a patch to enable function-sections, or is
>>> that a separate discussion?
>>
>> For the SPL on Tegra, those flags were already on; this patch fixes a
>> bug rather than prepares for new functionality.
>>
>>>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
>>
>>>> -       .u_boot_cmd : { *(.u_boot_cmd) }
>>>> +       .u_boot_cmd : { *(.u_boot_cmd*) }
>>>
>>> I don't think this line is needed?
>>>
>> ...
>>>> -               *(.dynsym)
>>>> +               *(.dynsym*)
>>>
>>> Nor this one?
>>
>> Possibly. I changed all the section names to be future-proof. Perhaps a
>> more targeted patch is warranted.
> 
> Has this been (at least build-)tested on all boards which have
> -ffunction-sections or -fdata-sections?

Yes; those flags both appear to be turned on when building the SPL for
Tegra (although strangely, not when building the main U-Boot):

xxx-gcc -M -g  -Os   -fno-common -ffixed-r8 -msoft-float  -D__KERNEL__
-ffunction-sections -fdata-sections -DCONFIG_SYS_TEXT_BASE=0x0010c000
-DCONFIG_SPL_TEXT_BASE=0x00108000 -DCONFIG_SPL_BUILD
-I/home/swarren/shared/git_wa/u-boot/include -fno-builtin -ffreestanding
-nostdinc -isystem xxx -pipe -DCONFIG_ARM -D__ARM__ -marm
-mno-thumb-interwork -mabi=aapcs-linux -march=armv4 -mtune=arm7tdmi -MQ
xxx/spl/arch/arm/cpu/arm720t/interrupts.o interrupts.c >
xxx/spl/arch/arm/cpu/arm720t/.depend.interrupts
Stephen Warren - Oct. 18, 2012, 9:17 p.m.
On 10/18/2012 02:58 PM, Stephen Warren wrote:
> On 10/18/2012 02:36 PM, Albert ARIBAUD wrote:
>> Hi Stephen,
>>
>> On Wed, 17 Oct 2012 21:17:45 -0600, Stephen Warren
>> <swarren@wwwdotorg.org> wrote:
>>
>>> On 10/17/2012 05:58 PM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>
>>>>> When -ffunction-sections or -fdata-section are used, symbols are placed
>>>>> into sections such as .data.eserial1_device and .bss.serial_current.
>>>>> Update the linker script to explicitly include these. Without this
>>>>> change (at least with my gcc-4.5.3 built using crosstool-ng), I see that
>>>>> the sections do end up being included, but __bss_end__ gets set to the
>>>>> same value as __bss_start.
>>>>>
>>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>>>>> ---
>>>>> This series fixes an SPL size overflow problem on Tegra. Tom Warren is
>>>>> out on vacation until Oct 25th, so he certainly won't be able to review
>>>>> this. Perhaps it could be applied directly to the ARM tree if enough
>>>>> Tegra people ack the series?
>>>>>
>>>>> Note that this series is not enough to make Tegra support work; either
>>>>> you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call
>>>>> serial_initialize() right before serial_init() in preloader_console_init()
>>>>> or wait for Allen Martin to rework Tegra's SPL support using the common
>>>>> SPL code.
>>>>
>>>> Are you going to submit a patch to enable function-sections, or is
>>>> that a separate discussion?
>>>
>>> For the SPL on Tegra, those flags were already on; this patch fixes a
>>> bug rather than prepares for new functionality.
>>>
>>>>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
>>>
>>>>> -       .u_boot_cmd : { *(.u_boot_cmd) }
>>>>> +       .u_boot_cmd : { *(.u_boot_cmd*) }
>>>>
>>>> I don't think this line is needed?
>>>>
>>> ...
>>>>> -               *(.dynsym)
>>>>> +               *(.dynsym*)
>>>>
>>>> Nor this one?
>>>
>>> Possibly. I changed all the section names to be future-proof. Perhaps a
>>> more targeted patch is warranted.
>>
>> Has this been (at least build-)tested on all boards which have
>> -ffunction-sections or -fdata-sections?
> 
> Yes; those flags both appear to be turned on when building the SPL for
> Tegra (although strangely, not when building the main U-Boot):
> 
> xxx-gcc -M -g  -Os   -fno-common -ffixed-r8 -msoft-float  -D__KERNEL__
> -ffunction-sections -fdata-sections -DCONFIG_SYS_TEXT_BASE=0x0010c000
> -DCONFIG_SPL_TEXT_BASE=0x00108000 -DCONFIG_SPL_BUILD
> -I/home/swarren/shared/git_wa/u-boot/include -fno-builtin -ffreestanding
> -nostdinc -isystem xxx -pipe -DCONFIG_ARM -D__ARM__ -marm
> -mno-thumb-interwork -mabi=aapcs-linux -march=armv4 -mtune=arm7tdmi -MQ
> xxx/spl/arch/arm/cpu/arm720t/interrupts.o interrupts.c >
> xxx/spl/arch/arm/cpu/arm720t/.depend.interrupts

I tried to test this more widely. I found that only
arch/arm/cpu/ixp/config.mk enables this on ARM, with the comment:

> # -fdata-sections triggers "section .bss overlaps section .rel.dyn" linker error
> PLATFORM_RELFLAGS += -ffunction-sections
> LDFLAGS_u-boot += --gc-sections

Indeed, if I edit arch/arm/cpu/armv7/config.mk to enable both
-ffunction-sections -fdata-sections, I do see that same section overlap
error building for Tegra.

Then, if I apply the .lds patch we're discussing, Tegra builds (and
runs) OK with those linker flags enabled. So, this patch seems to fix
two issues not just one - bonus:-)
Albert ARIBAUD - Oct. 20, 2012, 10:31 a.m.
Hi Stephen,

On Thu, 18 Oct 2012 15:17:52 -0600, Stephen Warren
<swarren@wwwdotorg.org> wrote:

> On 10/18/2012 02:58 PM, Stephen Warren wrote:
> > On 10/18/2012 02:36 PM, Albert ARIBAUD wrote:
> >> Hi Stephen,
> >>
> >> On Wed, 17 Oct 2012 21:17:45 -0600, Stephen Warren
> >> <swarren@wwwdotorg.org> wrote:
> >>
> >>> On 10/17/2012 05:58 PM, Simon Glass wrote:
> >>>> Hi Stephen,
> >>>>
> >>>> On Tue, Oct 16, 2012 at 2:50 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >>>>> From: Stephen Warren <swarren@nvidia.com>
> >>>>>
> >>>>> When -ffunction-sections or -fdata-section are used, symbols are placed
> >>>>> into sections such as .data.eserial1_device and .bss.serial_current.
> >>>>> Update the linker script to explicitly include these. Without this
> >>>>> change (at least with my gcc-4.5.3 built using crosstool-ng), I see that
> >>>>> the sections do end up being included, but __bss_end__ gets set to the
> >>>>> same value as __bss_start.
> >>>>>
> >>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> >>>>> ---
> >>>>> This series fixes an SPL size overflow problem on Tegra. Tom Warren is
> >>>>> out on vacation until Oct 25th, so he certainly won't be able to review
> >>>>> this. Perhaps it could be applied directly to the ARM tree if enough
> >>>>> Tegra people ack the series?
> >>>>>
> >>>>> Note that this series is not enough to make Tegra support work; either
> >>>>> you must hack ./arch/arm/cpu/arm720t/tegra-common/spl.c to call
> >>>>> serial_initialize() right before serial_init() in preloader_console_init()
> >>>>> or wait for Allen Martin to rework Tegra's SPL support using the common
> >>>>> SPL code.
> >>>>
> >>>> Are you going to submit a patch to enable function-sections, or is
> >>>> that a separate discussion?
> >>>
> >>> For the SPL on Tegra, those flags were already on; this patch fixes a
> >>> bug rather than prepares for new functionality.
> >>>
> >>>>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> >>>
> >>>>> -       .u_boot_cmd : { *(.u_boot_cmd) }
> >>>>> +       .u_boot_cmd : { *(.u_boot_cmd*) }
> >>>>
> >>>> I don't think this line is needed?
> >>>>
> >>> ...
> >>>>> -               *(.dynsym)
> >>>>> +               *(.dynsym*)
> >>>>
> >>>> Nor this one?
> >>>
> >>> Possibly. I changed all the section names to be future-proof. Perhaps a
> >>> more targeted patch is warranted.
> >>
> >> Has this been (at least build-)tested on all boards which have
> >> -ffunction-sections or -fdata-sections?
> > 
> > Yes; those flags both appear to be turned on when building the SPL for
> > Tegra (although strangely, not when building the main U-Boot):
> > 
> > xxx-gcc -M -g  -Os   -fno-common -ffixed-r8 -msoft-float  -D__KERNEL__
> > -ffunction-sections -fdata-sections -DCONFIG_SYS_TEXT_BASE=0x0010c000
> > -DCONFIG_SPL_TEXT_BASE=0x00108000 -DCONFIG_SPL_BUILD
> > -I/home/swarren/shared/git_wa/u-boot/include -fno-builtin -ffreestanding
> > -nostdinc -isystem xxx -pipe -DCONFIG_ARM -D__ARM__ -marm
> > -mno-thumb-interwork -mabi=aapcs-linux -march=armv4 -mtune=arm7tdmi -MQ
> > xxx/spl/arch/arm/cpu/arm720t/interrupts.o interrupts.c >
> > xxx/spl/arch/arm/cpu/arm720t/.depend.interrupts
> 
> I tried to test this more widely. I found that only
> arch/arm/cpu/ixp/config.mk enables this on ARM, with the comment:
> 
> > # -fdata-sections triggers "section .bss overlaps section .rel.dyn" linker error
> > PLATFORM_RELFLAGS += -ffunction-sections
> > LDFLAGS_u-boot += --gc-sections
> 
> Indeed, if I edit arch/arm/cpu/armv7/config.mk to enable both
> -ffunction-sections -fdata-sections, I do see that same section overlap
> error building for Tegra.
> 
> Then, if I apply the .lds patch we're discussing, Tegra builds (and
> runs) OK with those linker flags enabled. So, this patch seems to fix
> two issues not just one - bonus:-)

Unless someone opposes, I will pick this one in u-boot-arm/master later
today, as I will be doing some .lds merging on ARM (and then some
start.S merging). Once this is done, the Tegra tree (Tom already Cc:)
should merge u-boot-arm/master to get this first commit in, then apply
commits 2 to 5.

Amicalement,

Patch

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index e49ca0c..ae04a6e 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -35,7 +35,7 @@  SECTIONS
 	{
 		__image_copy_start = .;
 		CPUDIR/start.o (.text)
-		*(.text)
+		*(.text*)
 	}
 
 	. = ALIGN(4);
@@ -43,14 +43,14 @@  SECTIONS
 
 	. = ALIGN(4);
 	.data : {
-		*(.data)
+		*(.data*)
 	}
 
 	. = ALIGN(4);
 
 	. = .;
 	__u_boot_cmd_start = .;
-	.u_boot_cmd : { *(.u_boot_cmd) }
+	.u_boot_cmd : { *(.u_boot_cmd*) }
 	__u_boot_cmd_end = .;
 
 	. = ALIGN(4);
@@ -65,7 +65,7 @@  SECTIONS
 
 	.dynsym : {
 		__dynsym_start = .;
-		*(.dynsym)
+		*(.dynsym*)
 	}
 
 	_end = .;
@@ -81,7 +81,7 @@  SECTIONS
 
 	.bss __rel_dyn_start (OVERLAY) : {
 		__bss_start = .;
-		*(.bss)
+		*(.bss*)
 		 . = ALIGN(4);
 		__bss_end__ = .;
 	}