[PATCH] D34061: Fix weak symbols on arm and aarch64

[Peter Smith via llvm-commits]

I've created https://reviews.llvm.org/D34138 to add the relocations.

Peter

On 13 June 2017 at 08:55, Peter Smith <peter.smith at linaro.org> wrote:
> Ok, I'll fill out the rest.
>
> Peter
>
> On 12 June 2017 at 19:00, Rafael Avila de Espindola
> <rafael.espindola at gmail.com> wrote:
>> Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:
>>
>>> peter.smith accepted this revision.
>>> peter.smith added a comment.
>>> This revision is now accepted and ready to land.
>>>
>>> Looks good to me, thanks for spotting the mistake and apologies for making it in the first place. I've made a minor suggestion about making the default case unreachable but I don't think that this is that important.
>>>
>>>
>>>
>>> ================
>>> Comment at: ELF/InputSection.cpp:409
>>>    default:
>>> -    return A;
>>> +    return P + A;
>>>    }
>>> ----------------
>>> Might be worth making the default case unreachable.
>>>
>>> The ABI (ARM ELF 4.5.1.1) says:
>>> During linking, the value of an undefined weak reference is:
>>> - Zero if the relocation type is absolute
>>> - The address of the place if the relocation type is pc-relative
>>> - The nominal base address if the relocation type is base-relative.
>>>
>>> The intention for just returning A was that default: would cover the absolute case. However as long as getARMUndefinedRelativeWeakVA is only called from pc-relative locations this wouldn't happen so I don't expect the default case to ever get hit.
>>
>> Making the default of both getARMUndefinedRelativeWeakVA and
>> getAArch64UndefinedRelativeWeakVA unreachable is probably a good
>> thing. Unfortunately some relocations (R_ARM_MOVT_PREL for example) get
>> there.
>>
>> I will commit with the existing default values. Could you please then
>> replace the default with the missing cases?
>>
>> Thanks,
>> Rafael