On 11 January 2017 at 17:13, Christophe Lyon <christophe.l...@linaro.org> wrote:
> On 11 January 2017 at 16:48, Richard Earnshaw (lists)
> <richard.earns...@arm.com> wrote:
>> On 01/12/16 14:27, Christophe Lyon wrote:
>>> Hi,
>>>
>>>
>>> On 10 November 2016 at 15:10, Christophe Lyon
>>> <christophe.l...@linaro.org> wrote:
>>>> On 10 November 2016 at 11:05, Richard Earnshaw
>>>> <richard.earns...@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.
>>>
>>
>> This looks sensible, however, I think you should use 'non-weak' rather
>> than 'strong' in your comments (I've seen ABIs with weak, normal and
>> strong symbol definitions).
>>
>> Also, you're missing a space before each macro/function call.
>>
>> OK with those changes.
>>
>
> Thanks, I have attached what I have committed (r244320).
>
I forgot to ask: OK to backport to gcc-5 and gcc-6 branches?
> Christophe
>
>
>> R.
>>
>>> Christophe
>>>
>>>
>>>>> R.
>>>>>> Thanks,
>>>>>>
>>>>>> Christophe
>>>>>>
>>>>>
>>>>>
>>>>> pr78253.chlog.txt
>>>>>
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>> 2016-12-01 Christophe Lyon <christophe.l...@linaro.org>
>>>>>
>>>>> PR target/78253
>>>>> * config/arm/arm.c (legitimize_pic_address): Handle reference to
>>>>> weak symbol.
>>>>> (arm_assemble_integer): Likewise.
>>>>>
>>>>>
>>>>>
>>>>> pr78253.patch.txt
>>>>>
>>>>>
>>>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>>>> index 74cb64c..258ceb1 100644
>>>>> --- a/gcc/config/arm/arm.c
>>>>> +++ b/gcc/config/arm/arm.c
>>>>> @@ -6923,10 +6923,13 @@ legitimize_pic_address (rtx orig, machine_mode
>>>>> mode, rtx reg)
>>>>> same segment as the GOT. Unfortunately, the flexibility of linker
>>>>> scripts means that we can't be sure of that in general, so assume
>>>>> that GOTOFF is never valid on VxWorks. */
>>>>> + /* References to weak symbols cannot be resolved locally: they
>>>>> + may be overridden by a strong definition at link time. */
>>>>> rtx_insn *insn;
>>>>> if ((GET_CODE (orig) == LABEL_REF
>>>>> - || (GET_CODE (orig) == SYMBOL_REF &&
>>>>> - SYMBOL_REF_LOCAL_P (orig)))
>>>>> + || (GET_CODE (orig) == SYMBOL_REF
>>>>> + && SYMBOL_REF_LOCAL_P (orig)
>>>>> + && (SYMBOL_REF_DECL(orig) ? !DECL_WEAK(SYMBOL_REF_DECL(orig))
>>>>> : 1)))
>>>>> && NEED_GOT_RELOC
>>>>> && arm_pic_data_is_text_relative)
>>>>> insn = arm_pic_static_addr (orig, reg);
>>>>> @@ -21583,8 +21586,13 @@ arm_assemble_integer (rtx x, unsigned int size,
>>>>> int aligned_p)
>>>>> {
>>>>> /* See legitimize_pic_address for an explanation of the
>>>>> TARGET_VXWORKS_RTP check. */
>>>>> + /* References to weak symbols cannot be resolved locally:
>>>>> + they may be overridden by a strong definition at link
>>>>> + time. */
>>>>> if (!arm_pic_data_is_text_relative
>>>>> - || (GET_CODE (x) == SYMBOL_REF && !SYMBOL_REF_LOCAL_P (x)))
>>>>> + || (GET_CODE (x) == SYMBOL_REF
>>>>> + && (!SYMBOL_REF_LOCAL_P (x)
>>>>> + || (SYMBOL_REF_DECL(x) ? DECL_WEAK(SYMBOL_REF_DECL(x))
>>>>> : 0))))
>>>>> fputs ("(GOT)", asm_out_file);
>>>>> else
>>>>> fputs ("(GOTOFF)", asm_out_file);
>>
