This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [ARM] PR 78253 do not resolve weak ref locally
- From: "Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>
- To: Christophe Lyon <christophe dot lyon at linaro dot org>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 11 Jan 2017 15:48:14 +0000
- Subject: Re: [ARM] PR 78253 do not resolve weak ref locally
- Authentication-results: sourceware.org; auth=none
- References: <CAKdteOY57XHHGSKxTuWesTypp7e-8HS6HBLqcvP50FEssQfqcw@mail.gmail.com> <22e91ae6-7a21-3fd6-c85f-7021571b0c5e@foss.arm.com> <CAKdteOa5nH0a1u1uq0c2P10JLROPjheoTPKfGL=3YR0h3KKB8A@mail.gmail.com> <CAKdteOYPj7mCUD3GEYhriqXgLMMP4WFFF3U-TB_kVF_ATRdKWw@mail.gmail.com>
On 01/12/16 14:27, Christophe Lyon wrote:
> 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.
>
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.
R.
> Christophe
>
>
>>> R.
>>>> Thanks,
>>>>
>>>> Christophe
>>>>
>>>
>>>
>>> pr78253.chlog.txt
>>>
>>>
>>> gcc/ChangeLog:
>>>
>>> 2016-12-01 Christophe Lyon <christophe.lyon@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);