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. 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);