diff mbox

[U-Boot,V3,1/3] arm926ejs: fix linker file for newer ld support

Message ID 4CD28EF0.5020506@ahsoftware.de
State Superseded
Headers show

Commit Message

Alexander Holler Nov. 4, 2010, 10:46 a.m. UTC
Am 04.11.2010 11:26, schrieb Alexander Holler:
> I've just tested that patch using my small test-patch:
>
> -----------------------------------------------------------
> Marvell>>  g 0x700000
> ## Starting application at 0x00700000 ...
>
>
> U-Boot 2010.12-rc1-00037-g0ab2006 (Nov 04 2010 - 11:14:24)
> Seagate-DockStar
>
> SoC:   Kirkwood 88F6281_A0
> DRAM:  128 MiB
> (relocated) BSS is from 07fa8f60 to 07fef040
> &monitor_flash_len: 00752f60
> WARNING: relocation failed (&monitor_flash_len is outside reloctated BSS)!
> -----------------------------------------------------------

So the suggested change from Steve Sakoman (reordered fix from Albert 
Aribaud) still seems to be the one to prefer.

Here it is again:

--------------------------
  arch/arm/cpu/arm926ejs/u-boot.lds |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Albert ARIBAUD Nov. 4, 2010, 12:40 p.m. UTC | #1
Le 04/11/2010 11:46, Alexander Holler a écrit :
> So the suggested change from Steve Sakoman (reordered fix from Albert
> Aribaud) still seems to be the one to prefer.

Something is wrong here:

> -       .rel.dyn : { *(.rel.dyn) }
> +       .rel.dyn : { *(.rel.*) }

This change is the same as the one I submitted, replacing any .rel.dyn 
input sections references with .rel* input sections (this includes 
.rel.* sections), so I fail to see the difference.

Can you please perform a build and wrap the u-boot, u-boot.lds, 
u-boot.map files and build standard and error outputs in a tarball and 
send that to me?

Amicalement,
Alexander Holler Nov. 4, 2010, 12:47 p.m. UTC | #2
Am 04.11.2010 13:40, schrieb Albert ARIBAUD:
> Le 04/11/2010 11:46, Alexander Holler a écrit :
>> So the suggested change from Steve Sakoman (reordered fix from Albert
>> Aribaud) still seems to be the one to prefer.
>
> Something is wrong here:
>
>> - .rel.dyn : { *(.rel.dyn) }
>> + .rel.dyn : { *(.rel.*) }
>
> This change is the same as the one I submitted, replacing any .rel.dyn
> input sections references with .rel* input sections (this includes
> .rel.* sections), so I fail to see the difference.

The difference is the place. Your patch v1 puts that after the end which 
results in a larger binary. This one just leaves the place as before and 
only changes what is included. In regard to your v3 I don't know what is 
the difference, I haven't looked at what it (v3) really does because I 
don't know much about the linker sections and (currently) don't want to 
drive deeper into that.

>
> Can you please perform a build and wrap the u-boot, u-boot.lds,
> u-boot.map files and build standard and error outputs in a tarball and
> send that to me?

Will do so.

Regards,

Alexander
Albert ARIBAUD Nov. 4, 2010, 12:55 p.m. UTC | #3
Le 04/11/2010 13:47, Alexander Holler a écrit :
> Am 04.11.2010 13:40, schrieb Albert ARIBAUD:
>> Le 04/11/2010 11:46, Alexander Holler a écrit :
>>> So the suggested change from Steve Sakoman (reordered fix from Albert
>>> Aribaud) still seems to be the one to prefer.
>>
>> Something is wrong here:
>>
>>> - .rel.dyn : { *(.rel.dyn) }
>>> + .rel.dyn : { *(.rel.*) }
>>
>> This change is the same as the one I submitted, replacing any .rel.dyn
>> input sections references with .rel* input sections (this includes
>> .rel.* sections), so I fail to see the difference.
>
> The difference is the place. Your patch v1 puts that after the end which
> results in a larger binary.

V3 doesn't result in a larger binary any more, at least as far as my own 
tests have shown, and that is why I asked people to test V3.

> This one just leaves the place as before and
> only changes what is included. In regard to your v3 I don't know what is
> the difference, I haven't looked at what it (v3) really does because I
> don't know much about the linker sections and (currently) don't want to
> drive deeper into that.

Then please do test this V3 patch and verify if the issues you raise 
still apply or are fixed.

>> Can you please perform a build and wrap the u-boot, u-boot.lds,
>> u-boot.map files and build standard and error outputs in a tarball and
>> send that to me?
>
> Will do so.

Please make sure to indicate which source commit you're testing on and 
which toolchain you're using, and to compare with V3 results.

> Regards,
>
> Alexander

Amicalement,
Alexander Holler Nov. 4, 2010, 1:18 p.m. UTC | #4
Am 04.11.2010 13:55, schrieb Albert ARIBAUD:

> Then please do test this V3 patch and verify if the issues you raise
> still apply or are fixed.

V3 failed using my relocation-test-patch (and resulted in the same 
problems as without any relocation patch). Therefor I've posted the 
output. I though it was clear that I've used v3 (because of the subject).

> Please make sure to indicate which source commit you're testing on and
> which toolchain you're using, and to compare with V3 results.

I've created a tarball with the patches for the DockStar I'm currently 
using for those tests.

It is available here:

http://ahsoftware.de/dockstar_uboot_patches.tar.bz2

Regards,

Alexander
Albert ARIBAUD Nov. 4, 2010, 4:56 p.m. UTC | #5
Le 04/11/2010 14:18, Alexander Holler a écrit :
> Am 04.11.2010 13:55, schrieb Albert ARIBAUD:
>
>> Then please do test this V3 patch and verify if the issues you raise
>> still apply or are fixed.
>
> V3 failed using my relocation-test-patch (and resulted in the same
> problems as without any relocation patch). Therefor I've posted the
> output. I though it was clear that I've used v3 (because of the subject).

The body kept talking about V1 and did mention that you didn't look at 
V3, so I wanted to make sure there was no mistake here.

>> Please make sure to indicate which source commit you're testing on and
>> which toolchain you're using, and to compare with V3 results.
>
> I've created a tarball with the patches for the DockStar I'm currently
> using for those tests.
>
> It is available here:
>
> http://ahsoftware.de/dockstar_uboot_patches.tar.bz2
>
> Regards,
>
> Alexander

You're still using a locally-built toolchain different from CS and ELDK 
toolhcains that were tested so far, right? There is a warning at the 
linker stage in the build log you sent me:

ld: warning: creating a DT_TEXTREL in object.

which I don't have using the CS 2010q1 toolchain.

Can you please checkout the pristine master branch of u-boot and build 
sheevaplug with just my V3 patch applied and you toolchain, and tell me 
if you see this warning?

Amicalement,
Alexander Holler Nov. 4, 2010, 6:37 p.m. UTC | #6
> You're still using a locally-built toolchain different from CS and ELDK
> toolhcains that were tested so far, right? There is a warning at the

Yes, I'm still using Gentoo to compile u-boot native on that board.

> linker stage in the build log you sent me:
>
> ld: warning: creating a DT_TEXTREL in object.
>
> which I don't have using the CS 2010q1 toolchain.

This warning comes through one of the patches Gentoo applies to 
binutils. That patch can be found inside the archive here:

ftp://de-mirror.org/distro/gentoo/distfiles/binutils-2.20.1-patches-1.1.tar.bz2

> Can you please checkout the pristine master branch of u-boot and build
> sheevaplug with just my V3 patch applied and you toolchain, and tell me
> if you see this warning?

The warning is still there. But I'll see this warning with working 
versions of u-boot too.

Regards,

Alexander
Alexander Holler Nov. 4, 2010, 6:42 p.m. UTC | #7
Am 04.11.2010 19:37, schrieb Alexander Holler:

>> You're still using a locally-built toolchain different from CS and ELDK
>> toolhcains that were tested so far, right? There is a warning at the
>
> Yes, I'm still using Gentoo to compile u-boot native on that board.
>
>> linker stage in the build log you sent me:
>>
>> ld: warning: creating a DT_TEXTREL in object.
>>
>> which I don't have using the CS 2010q1 toolchain.
>
> This warning comes through one of the patches Gentoo applies to
> binutils. That patch can be found inside the archive here:
>
> ftp://de-mirror.org/distro/gentoo/distfiles/binutils-2.20.1-patches-1.1.tar.bz2

If I read that patch (66_*) correctly, it's the same as when using 
--warn-shared-textrel in LDFLAGS.

Regards,

Alexander
Albert ARIBAUD Nov. 4, 2010, 7:20 p.m. UTC | #8
Le 04/11/2010 19:42, Alexander Holler a écrit :
> Am 04.11.2010 19:37, schrieb Alexander Holler:
>
>>> You're still using a locally-built toolchain different from CS and ELDK
>>> toolhcains that were tested so far, right? There is a warning at the
>>
>> Yes, I'm still using Gentoo to compile u-boot native on that board.
>>
>>> linker stage in the build log you sent me:
>>>
>>> ld: warning: creating a DT_TEXTREL in object.
>>>
>>> which I don't have using the CS 2010q1 toolchain.
>>
>> This warning comes through one of the patches Gentoo applies to
>> binutils. That patch can be found inside the archive here:
>>
>> ftp://de-mirror.org/distro/gentoo/distfiles/binutils-2.20.1-patches-1.1.tar.bz2
>
> If I read that patch (66_*) correctly, it's the same as when using
> --warn-shared-textrel in LDFLAGS.

All right. I don't like getting a warning like that, but as I cannot 
reproduce your toolchain, I cannot avoid it.

Regarding the build you sent me, the fixup for monitor_flash_len is 
present in the fixup table at the 11th entry at 00752fb8 and the fixup 
table is correctly accessible using _rel_dyn_start_ofs and 
_rel_dyn_end_ofs. That means your code should have done the fixup if the 
relocation loop was run correctly.

Can you do a step-by-step run of the fixup loop under a debugger?

> Regards,
>
> Alexander

Amicalement,
Alexander Holler Nov. 4, 2010, 7:39 p.m. UTC | #9
Hello,

Am 04.11.2010 20:20, schrieb Albert ARIBAUD:

>> If I read that patch (66_*) correctly, it's the same as when using
>> --warn-shared-textrel in LDFLAGS.
>
> All right. I don't like getting a warning like that, but as I cannot
> reproduce your toolchain, I cannot avoid it.

I think they enabbled this by default to do some kind of QA, because 
those textrels are forcing the dynamic linker to resolve some stuff 
which otherwise (without those textrels) isn't needed. And this results 
in longer load times.

Sidenote: My toolchain is reproducable, but that means you have to 
compile a Gentoo system, because Gentoo is a source only distribution. 
So it isn't as comfortable as using binary distributions (here), but it 
has other advantages.

> Regarding the build you sent me, the fixup for monitor_flash_len is
> present in the fixup table at the 11th entry at 00752fb8 and the fixup
> table is correctly accessible using _rel_dyn_start_ofs and
> _rel_dyn_end_ofs. That means your code should have done the fixup if the
> relocation loop was run correctly.
>
> Can you do a step-by-step run of the fixup loop under a debugger?

Sorry, no. That thing has 2mm-header for the jtag and I don't have any 
prefabricated cable for that. Maybe someone else with binutils 2.20.1 
could check your V3. I don't know if I will find the time test your v3 
in the next days with some other hw where I can attach a jtag.

Regards,

Alexander
Albert ARIBAUD Nov. 4, 2010, 10:06 p.m. UTC | #10
Le 04/11/2010 20:39, Alexander Holler a écrit :

> Sidenote: My toolchain is reproducable, but that means you have to
> compile a Gentoo system, because Gentoo is a source only distribution.
> So it isn't as comfortable as using binary distributions (here), but it
> has other advantages.

It probably has advantages, but having to adopt and run a given 
distribution in order to get access to a given toolchain goes way beyond 
what I am willing to do.

However, I think I have found the cause of the problem in my V3 build, 
at least on an openrd_base.

V3 tried to overlay bss and rel.dyn so as to minimize the FLASH/NAND 
footprint as well as RAM footprint. However, overlaying was done by 
forcing .rel.dyn address to be equal to __bss_start and marking it 
OVERLAY, which made it disappear from the .bin file, thus causing 
relocation to fail.

I reversed the definition and order by forcing bss to start at 
__rel_dyn_start, which worked but caused a linker warning that two 
sections started at the same VMA -- the linker apparently does not take 
into account that one of them, .bss, is NOLOAD.

I changed NOLOAD into OVERLAY, and then all worked: initial mapping (in 
FLASH or in RAM when loaded from NAND) has text, data, rel.dyn and 
dynsym bytes but not bss, which is fine since no code running there 
should use it; final mapping (once relocated) has text, data and bss 
without rel.dyn and dynsym using up RAM.

Tested on openrd_base, works.

V4 of patch set coming in the next few minutes.

> Regards,
>
> Alexander

Amicalement,
diff mbox

Patch

diff --git a/arch/arm/cpu/arm926ejs/u-boot.lds 
b/arch/arm/cpu/arm926ejs/u-boot.lds
index 72f45f8..eefdca9 100644
--- a/arch/arm/cpu/arm926ejs/u-boot.lds
+++ b/arch/arm/cpu/arm926ejs/u-boot.lds
@@ -46,7 +46,7 @@  SECTIONS
         . = ALIGN(4);

         __rel_dyn_start = .;
-       .rel.dyn : { *(.rel.dyn) }
+       .rel.dyn : { *(.rel.*) }
         __rel_dyn_end = .;

         __dynsym_start = .;