gcc: Thumb interworking and weakly linked functions

[Aneesh V]

On Tuesday 21 February 2012 03:27 PM, Dave Martin wrote:
> On Mon, Feb 20, 2012 at 06:59:53PM +0100, Ulrich Weigand wrote:
>> "V, Aneesh"<aneesh at ti.com>  wrote:
>>
>>> I agree that not marking the assembly functions ' %function' is a problem
>>> in the code, so it's not a critical bug. But I would've been happier if
>>> the linker refused to link it rather than branching with the wrong
>>> instruction. Isn't that a problem?
>>
>> Well, if the target symbol of a branch is not marked as %function,
>> the linker has no way of knowing whether that target is ARM or Thumb,
>> so it cannot specifically error out if (and only if) the instruction
>> is wrong.
>>
>> The linker *could* in theory give an error *unconditionally* whenever
>> it detects a branch to a non-%function symbol.  The reason this is not
>> done is probably for backwards compatibility with old hand-written code,
>> say from an ARM-only era: branches to non-function symbols used to be
>> allowed, and in fact work fine if all code is ARM-only.  Adding an error
>> would break such old code.
>>
>>
>>> Problem No:2
>>> *************
>>> Linaro GCC 2012.01 is giving a new problem w.r.to Thumb build
>>> that is not existing in Sourcery G++ Lite 2010q1-202. However, I
>>> couldn't reproduce this problem with a small program like above. So,
>>> let me give you reference to the original u-boot code that shows the
>>> problem and steps to reproduce it.
>> [snip]
>>> Please note that the .rodata symbols have odd addresses. These arrays
>>> actually need to be aligned at least to half-word boundary. In fact, in
>>> the image I verified that they are put at even addresses. So, the
>>> symbols have been kept as real address +1.
>>
>> This seems strange.  How did you verify "that they are put at even
>> addresses"?
>> I can reproduce the odd addresses of .rodata symbols.  However, this
>> occurs simply because the linker put *no* alignment requirement whatsoever
>> on those sections:
>>
>> Section Headers:
>>    [Nr] Name              Type            Addr     Off    Size   ES Flg Lk
>> Inf Al
>> [snip]
>>    [11] .rodata.wkup_padc PROGBITS        00000000 000100 000004 00   A  0
>> 0  1
>>    [12] .rodata.wkup_padc PROGBITS        00000000 000104 000048 00   A  0
>> 0  1
>>    [13] .rodata.wkup_padc PROGBITS        00000000 00014c 00000c 00   A  0
>> 0  1
>>    [14] .rodata.wkup_padc PROGBITS        00000000 000158 000004 00   A  0
>> 0  1
>>
>> Note the "Al" column values of 1.  In the final executable, those sections
>> happen to end up immediately following a .rodata.str string section with
>> odd
>> lenght, and since they don't have any alignment requirement, they start out
>> at an odd address.
>>
>> The reason for the lack of alignment requirement is actually in the source:
>>
>> const struct pad_conf_entry core_padconf_array_essential[] = {
>>
>> where
>>
>> struct pad_conf_entry {
>>
>>          u16 offset;
>>
>>          u16 val;
>>
>> } __attribute__ ((packed));
>>
>>
>> The "packed" attribute specifies that all struct elements ought to be
>> considered to have alignment requirement 1 instead of their default
>> alignment.  Thus the whole struct ends up having alignment requirement 1;
>> and since the section contains only a single variable of such struct
>> type, this is then also the alignment requirement of the section.
>
> Is "packed" even wanted here?
>
> Based on a very brief skim of the code, it looks like the packed attribute
> is an unnecessary attempt to save some space -- unnecessary because all
> ARM ABI variants I know of (actually, all arches I know of) will pack that
> structure into a word anyway as a result of natural alignment of the
> members.  It doesn't look to me like the packed structure is used to map a
> memory-mapped peripheral directly or otherwise communicate with the outside
> world -- which would be the only situations in which a packed structure
> would normally make sense.
>
> Of course, I may have missed something...

No. I think packed is not needed here. Removing it didn't change the
size of the arrays. I will remove it.

Thanks,
Aneesh