[U-Boot] [PATCH v2 6/7] arm: add a common .lds link script

[Albert ARIBAUD]

Hi Simon,

Le 21/02/2012 03:32, Simon Glass a écrit :
> Hi Albert,
>
> On Fri, Feb 17, 2012 at 3:08 AM, Albert ARIBAUD
> <albert.u.boot at aribaud.net>  wrote:
>> Hi Simon,
>>
>> Le 21/11/2011 21:49, Simon Glass a écrit :
>>
>>
>>> +       .text :
>>> +       {
>>> +               __text_start = .;
>>
>>
>> This assignment to __text_start does not exist in any of the existing
>> u-boot.lds files. What is the point of it?
>
> I would like to have a single symbol for the start of the text region
> across all architectures. ARM has _start but this is not present
> elsewhere. Also I think it should be defined by the link script.

I suspect what you would like to have is a single symbol for the start 
of the region to copy, rather than the start of the test region per se.

>>
>>
>>> +       .u_boot_cmd : { *(.u_boot_cmd) }
>>> +       __u_boot_cmd_end = .;
>>> +
>>> +       . = ALIGN(4);
>>> +
>>> +       __image_copy_end = .;
>>
>>
>> Ditto for __image_copy_end.
>
> This is the end of the region that needs to be copied when U-Boot is
> relocated. The symbol exists in ARMv7 so I have reproduced it here. If
> I read the SPL code correctly then it seems to need this.
>
>>
>> These two changes are unexplained in the commit message. Mind you, the one
>> about CPUDIR and start.S isn't either... and it should, because once the
>> commit is in, there is no indication left that it is part of a set, so
>> readers will have a difficulty spotting the changes introduced.
>
> OK I will take a look at these problems.
>
>>
>> But what bothers me most is that the patch set produces u-boot.bin files
>> which are not binary identical to those produced without the patch set; if I
>> remove the two assignments, then binary identity is preserved.
>>
>> Note: I check for binary identity by diff'ing hex dumps of u-boot.bin files
>> produced with and without the patch set. If the only difference is the build
>> version and date, I deem the files binary identical. The dump is done with
>> 'od -t x1z -A x u-boot.bin'.
>>
>> So unless there is a compelling and strictly unavoidable reason for these
>> assignements, please drop them in the V3 patch set.
>
> The binary will need to include values for these symbols in the dynsym
> area, so adding a link symbol cannot produce an identical binary. But
> the change should be harmless - it is just an extra symbol.

If that is only an extra symbol (hence unreferred to so far) then it 
should not have any impact on relocation data, which apply only to 
existing references -- just like adding an "extern" definition for  int 
abc;" won't have any effect unless/until some reference to abc exists -- 
and it should certainly not affect text and data.

However, if I compare builds of edminiv2 with and without patch 7/7 
(that's the patch which makes edminiv2 use the common u-boot.lds), then 
I see changes as early as the 69th byte of the text section.

I suspect the issue is cause by the additional ALIGN() directive that 
you are adding along with the symbols. Considering that this change 
should only replace existing symbols (_start and _bss_start) with other, 
functionally identical symbols, there is no reason to alter alignments 
in the linker file.

> I do not want to drop these - in fact I want to standardize on these
> (or something similar that we agree) across all architectures since it
> makes the generic relocation code easier (at present it uses #ifdef to
> work out what to do in the two different cases (SPL and non-SPL).
> Ultimately we can look towards more uniformity across architectures in
> the .lds files.

I do understand what you want to achieve here -- replace the existing 
arch-specific symbols that mark the start and end of the area to copy 
and relocate with arch-agnostic ones. But then, that change in itself is 
akin to a rename or alias and should have zero effect on the resulting 
binary.

> I will take another look at this series.

I see there were V3 issued for 6/7 and 7/7. I'll comment these.

> Regards,
> Simon

Amicalement,
-- 
Albert.