Re: [ARM] PR 78253 do not resolve weak ref locally

[Christophe Lyon <christophe dot lyon at linaro dot org>]

Hi,


On 10 November 2016 at 15:10, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
> On 10 November 2016 at 11:05, Richard Earnshaw
> <Richard.Earnshaw@foss.arm.com> wrote:
>> On 09/11/16 21:29, Christophe Lyon wrote:
>>> Hi,
>>>
>>> PR 78253 shows that the handling of weak references has changed for
>>> ARM with gcc-5.
>>>
>>> When r220674 was committed, default_binds_local_p_2 gained a new
>>> parameter (weak_dominate), which, when true, implies that a reference
>>> to a weak symbol defined locally will be resolved locally, even though
>>> it could be overridden by a strong definition in another object file.
>>>
>>> With r220674, default_binds_local_p forces weak_dominate=true,
>>> effectively changing the previous behavior.
>>>
>>> The attached patch introduces default_binds_local_p_4 which is a copy
>>> of default_binds_local_p_2, but using weak_dominate=false, and updates
>>> the ARM target to call default_binds_local_p_4 instead of
>>> default_binds_local_p_2.
>>>
>>> I ran cross-tests on various arm* configurations with no regression,
>>> and checked that the test attached to the original bugzilla now works
>>> as expected.
>>>
>>> I am not sure why weak_dominate defaults to true, and I couldn't
>>> really understand why by reading the threads related to r220674 and
>>> following updates to default_binds_local_p_* which all deal with other
>>> corner cases and do not discuss the weak_dominate parameter.
>>>
>>> Or should this patch be made more generic?
>>>
>>
>> I certainly don't think it should be ARM specific.
> That was my feeling too.
>
>>
>> The questions I have are:
>>
>> 1) What do other targets do today.  Are they the same, or different?
>
> arm, aarch64, s390 use default_binds_local_p_2 since PR 65780, and
> default_binds_local_p before that. Both have weak_dominate=true
> i386 has its own version, calling default_binds_local_p_3 with true
> for weak_dominate
>
> But the behaviour of default_binds_local_p changed with r220674 as I said above.
> See https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=220674 and
> notice how weak_dominate was introduced
>
> The original bug report is about a different case:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=32219
>
> The original patch submission is
> https://gcc.gnu.org/ml/gcc-patches/2015-02/msg00410.html
> and the 1st version with weak_dominate is in:
> https://gcc.gnu.org/ml/gcc-patches/2015-02/msg00469.html
> but it's not clear to me why this was introduced
>
>> 2) If different why?
> on aarch64, although binds_local_p returns true, the relocations used when
> building the function pointer is still the same (still via the GOT).
>
> aarch64 has different logic than arm when accessing a symbol
> (eg aarch64_classify_symbol)
>
>> 3) Is the current behaviour really what was intended by the patch?  ie.
>> Was the old behaviour actually wrong?
>>
> That's what I was wondering.
> Before r220674, calling a weak function directly or via a function
> pointer had the same effect (in other words, the function pointer
> points to the actual implementation: the strong one if any, the weak
> one otherwise).
>
> After r220674, on arm the function pointer points to the weak
> definition, which seems wrong to me, it should leave the actual
> resolution to the linker.
>
>

After looking at the aarch64 port, I think that references to weak symbols
have to be handled carefully, to make sure they cannot be resolved
by the assembler, since the weak symbol can be overridden by a strong
definition at link-time.

Here is a new patch which does that.
Validated on arm* targets with no regression, and I checked that the
original testcase now executes as expected.

Christophe


>> R.
>>> Thanks,
>>>
>>> Christophe
>>>
>>

Attachment: pr78253.chlog.txt
Description: Text document

Attachment: pr78253.patch.txt
Description: Text document