From patchwork Wed Jan 11 16:13:44 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christophe Lyon X-Patchwork-Id: 90932 Delivered-To: patch@linaro.org Received: by 10.140.20.99 with SMTP id 90csp1196916qgi; Wed, 11 Jan 2017 08:14:20 -0800 (PST) X-Received: by 10.84.195.228 with SMTP id j91mr14127900pld.88.1484151260289; Wed, 11 Jan 2017 08:14:20 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id v69si6216114pgd.119.2017.01.11.08.14.20 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 11 Jan 2017 08:14:20 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-return-445878-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-445878-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-445878-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; q=dns; s=default; b=lOx9+eciij2wvMR 5QAr2odafj7Yrl0ALqRCTY9lry24J7cPIF63Qi9Ktk8277ilkzy/2X8K+FOq4Ax5 lawB2++8ZLq57V1GDxOeRwoRB66YnjwcGpSvFU7cMXwQvBJ+wDx/8Bn8XWiWrrGb Eoc441AltRPFd68gQGIh72A40CcA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; s=default; bh=CoxFZRjDpALbSiaWubk+U 4AmZWc=; b=ZFMQkqCxQ9De7ItOt+GByLDn/dxgwwmcHbAgfBn1c+rfrY7rcUCYI fhHth8cMXAmkbhXWtfad18VYNsovmXh+OtBl1P6le0YhMwzTD1/AHOIAqtjG3JhB Z2eAHgKAmdBt368iY/4C+DjIZ2c9/tdqAHFFZT0L1LuZor65Lvhzg8= Received: (qmail 106002 invoked by alias); 11 Jan 2017 16:13:58 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 105931 invoked by uid 89); 11 Jan 2017 16:13:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, KAM_STOCKGEN, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=richardearnshawarmcom, richard.earnshaw@arm.com, RichardEarnshawarmcom, Richard.Earnshaw@arm.com X-HELO: mail-qt0-f174.google.com Received: from mail-qt0-f174.google.com (HELO mail-qt0-f174.google.com) (209.85.216.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 11 Jan 2017 16:13:47 +0000 Received: by mail-qt0-f174.google.com with SMTP id x49so133511685qtc.2 for ; Wed, 11 Jan 2017 08:13:47 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=sRm/Om13qXrfmdscmj+W/I2LR4mfr7lZqssQrZxwXIc=; b=unwbpTZDDykAHU3WGNrLI8/MOJwkpvX6Is5xMECb3C3jYvk4tf/bUWaLYLHJ7gJzAj 0s/mYRYJ2qFWApKp5Z9xoFxUgBM4ZEbz4EDif7HWWD5JjioD4LnwVhlMN0hqRDlV9HX+ 1tsgr5B3e3DKh14MKrXvvr7NkZOUk/mx86Fl4BOMadWe+1EZvg+TpYkp0+yjE8QwswZA HutOp5+vEKhk031Z7w5YvpUizmOttnm9JZKBNDqLhkQQ+5fklnk9lOyj1F7g3cOInsm+ hoPKBejw9JS8Kw7kDcCHHJHQpgGBbw9ocb3uxHrnRGKAzGOvI0+EpSfKnl3QbX13djJx y3aw== X-Gm-Message-State: AIkVDXIOAxOdfFr8W2KALNABDIT/4qOaUhUb5D40mZzs0TrTcgrOevbiJv2qDI8ACtVs69nk51dXMf1qx6c53B49 X-Received: by 10.200.54.182 with SMTP id a51mr9231477qtc.221.1484151225855; Wed, 11 Jan 2017 08:13:45 -0800 (PST) MIME-Version: 1.0 Received: by 10.140.101.144 with HTTP; Wed, 11 Jan 2017 08:13:44 -0800 (PST) In-Reply-To: <8f132ef6-800b-0eae-a46e-43369dc68809@arm.com> References: <22e91ae6-7a21-3fd6-c85f-7021571b0c5e@foss.arm.com> <8f132ef6-800b-0eae-a46e-43369dc68809@arm.com> From: Christophe Lyon Date: Wed, 11 Jan 2017 17:13:44 +0100 Message-ID: Subject: Re: [ARM] PR 78253 do not resolve weak ref locally To: "Richard Earnshaw (lists)" Cc: "gcc-patches@gcc.gnu.org" X-IsSubscribed: yes On 11 January 2017 at 16:48, Richard Earnshaw (lists) wrote: > On 01/12/16 14:27, Christophe Lyon wrote: >> Hi, >> >> >> On 10 November 2016 at 15:10, Christophe Lyon >> wrote: >>> On 10 November 2016 at 11:05, Richard Earnshaw >>> 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). Christophe > R. > >> Christophe >> >> >>>> R. >>>>> Thanks, >>>>> >>>>> Christophe >>>>> >>>> >>>> >>>> pr78253.chlog.txt >>>> >>>> >>>> gcc/ChangeLog: >>>> >>>> 2016-12-01 Christophe Lyon >>>> >>>> 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); > gcc/ChangeLog: 2017-01-11 Christophe Lyon PR target/78253 * config/arm/arm.c (legitimize_pic_address): Handle reference to weak symbol. (arm_assemble_integer): Likewise. commit 65657b3d9b51c2f94036e5096e6fe3d3c4488cf8 Author: Christophe Lyon Date: Wed Nov 30 12:00:14 2016 +0100 WIP bug 2562 Change-Id: I461e97a44da8aa7061a1740ed3faa2c085d1cb5f diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 74cb64c..4c0a626 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -6923,10 +6923,14 @@ 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 non-weak 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 +21587,14 @@ 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 non-weak 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);