On 11/01/17 16:14, Christophe Lyon wrote:
> 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?
>
Yes. But give it a couple of days, just in case it throws up any issues.
R.
>
>> 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);
>>>
