From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x444.google.com (mail-pf1-x444.google.com [IPv6:2607:f8b0:4864:20::444]) by sourceware.org (Postfix) with ESMTPS id 3EBFC386F81F for ; Fri, 4 Sep 2020 07:25:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3EBFC386F81F Received: by mail-pf1-x444.google.com with SMTP id o20so4114378pfp.11 for ; Fri, 04 Sep 2020 00:25:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Ptfh4rMm6wk58NmVHabwVJHNuowYkSnzn7vKbjCxLyk=; b=cZFw8IR1ZGK3mCoh1w+V/v7VFEE4Hm9lAzhqehv9amG3KrPEFJUvsJoztMrzmS5pNC fYAc8xzsqF2qFFb52/NMGg01BlmFjHQdk7sSpLd038YMN7JTVqOmwj1EUiiOd3DVgWia TXBrhu6baUA7YOl4W4wS0ZBl5pur94OY4q3GGdo3kunLTnksOyQkapbKLu0mA0ExIrGg OeEmHfkdRdSCuxrgbb85YTnNwKun2Ie+/q64j5C8+24TOYjwANhmFSD0w5WHfD1l3ZtE sFRsVqraJqmvtenefybvw0kPOivJLm03/m1bwhHVMS4ZWyWhE5RxrfTla5TlKXZvHyun LKHQ== X-Gm-Message-State: AOAM533KjJOQp9XAbm4Cctp9Z/c6F4VWF7fZUqH4jEgJlyvPOs+8QyNF 5QMkJVuQ3q/QShdTDRi4LeY= X-Google-Smtp-Source: ABdhPJzbM1arwm7kNAXHZ6mx32peR6GCilyUxddpW14w0cYiqJDHOUgZ82kRF3m+Iagv8eh1dC10iA== X-Received: by 2002:a63:413:: with SMTP id 19mr6224642pge.310.1599204334298; Fri, 04 Sep 2020 00:25:34 -0700 (PDT) Received: from bubble.grove.modra.org (158.106.96.58.static.exetel.com.au. [58.96.106.158]) by smtp.gmail.com with ESMTPSA id y7sm5540135pfm.68.2020.09.04.00.25.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 04 Sep 2020 00:25:33 -0700 (PDT) Received: by bubble.grove.modra.org (Postfix, from userid 1000) id 0A9E780BD3; Fri, 4 Sep 2020 16:55:29 +0930 (ACST) Date: Fri, 4 Sep 2020 16:55:28 +0930 From: Alan Modra To: "H.J. Lu" Cc: Martin =?utf-8?B?TGnFoWth?= , Binutils Subject: Re: [PATCH] elf: Don't load archive element after dynamic definition Message-ID: <20200904072528.GS15695@bubble.grove.modra.org> References: <20200902081225.GH15695@bubble.grove.modra.org> <20200902130522.GI15695@bubble.grove.modra.org> <20200902142930.GJ15695@bubble.grove.modra.org> <20200903013137.GM15695@bubble.grove.modra.org> <20200903060752.GO15695@bubble.grove.modra.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: binutils@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Binutils mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Sep 2020 07:25:37 -0000 On Thu, Sep 03, 2020 at 04:34:23AM -0700, H.J. Lu wrote: > On Wed, Sep 2, 2020 at 11:07 PM Alan Modra wrote: > > > > On Wed, Sep 02, 2020 at 07:16:14PM -0700, H.J. Lu wrote: > > > On Wed, Sep 2, 2020 at 6:31 PM Alan Modra wrote: > > > > > > > > On Wed, Sep 02, 2020 at 07:35:58AM -0700, H.J. Lu wrote: > > > > > On Wed, Sep 2, 2020 at 7:29 AM Alan Modra wrote: > > > > > > > > > > > > On Wed, Sep 02, 2020 at 06:22:08AM -0700, H.J. Lu wrote: > > > > > > > > It's reasonably obvious that we need to load archive elements when > > > > > > > > they define IR referenced symbols, because the archive element might > > > > > > > > be an LTO object. What's not so obvious is whether loading of shared > > > > > > > > libraries should follow the same rule. I think they should, in order > > > > > > > > for LTO to get symbol resolution correct in corner cases. Basically > > > > > > > > LTO needs to know what shared libraries are loaded before > > > > > > > > recompilation. See commit a896df97b952 log comments. > > > > > > > > > > > > > > There is dynamic_def for this purpose. > > > > > > > > > > > > Your patch doesn't make changes to ld/plugin.c to inform LTO of the > > > > > > availability of these symbols. And if you did, then how does the > > > > > > linker work out whether or not the LTO recompilation depended on those > > > > > > symbols? If it did change LTO recompilation then you had better > > > > > > ensure the library really is loaded. By the time you work all of that > > > > > > out, if it is even possible, your patch will likely be very > > > > > > complicated indeed. > > > > > > > > > > A testcase? > > > > > > > > What don't you understand from my emails in this thread? I suggest > > > > you look at what happened with the testcase in PR26314, in regard to > > > > my comment > > > > The lto recompilation didn't see symbol references from libbfd.so and > > > > variables like _xexit_cleanup are made local in the recompiled > > > > objects. Oops, two copies of them. > > > > > > A testcase? > > > > You kindly provided it yourself. > > https://sourceware.org/bugzilla/show_bug.cgi?id=26314#c4 > > > > It takes only a small amount of digging to see the _xexit_cleanup > > problem. > > This particular problem came from: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96385 > > where GCC generated incorrect output and I do have a mitigation > patch. Yes, you gave that as a testcase for unwanted dynamic symbols. I suspected there was more than just that going on, so didn't approve the entirety of your patch. While my initial guess at a ranlib issue was wrong, on spending time properly analysing the testcase I found that we had a problem with symbol info we hand off to LTO. Specifically, we need to tell LTO about symbols in all shared libraries loaded. That means we can't load extra shared libraries after LTO recompilation, at least, not those that affect the set of symbols that LTO cares about, the IR symbols. Committed. bfd/ PR 15146 PR 26314 PR 26530 * elflink.c (elf_link_add_object_symbols): Do set def_regular and ref_regular for IR symbols. Don't clear dynsym, allowing IR symbols to load --as-needed shared libraries, but prevent IR symbols from becoming dynamic. ld/ * testsuite/ld-plugin/lto.exp: Don't run pr15146 tests. * testsuite/ld-plugin/pr15146.d: Delete. * testsuite/ld-plugin/pr15146a.c: Delete. * testsuite/ld-plugin/pr15146b.c: Delete. * testsuite/ld-plugin/pr15146c.c: Delete. * testsuite/ld-plugin/pr15146d.c: Delete. diff --git a/bfd/elflink.c b/bfd/elflink.c index 5c085b14b7..1384c1a46b 100644 --- a/bfd/elflink.c +++ b/bfd/elflink.c @@ -4977,11 +4977,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info) object and a shared object. */ bfd_boolean dynsym = FALSE; - /* Plugin symbols aren't normal. Don't set def_regular or - ref_regular for them, or make them dynamic. */ - if ((abfd->flags & BFD_PLUGIN) != 0) - ; - else if (! dynamic) + if (! dynamic) { if (! definition) { @@ -5162,10 +5158,6 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info) && !bfd_link_relocatable (info)) dynsym = FALSE; - /* Nor should we make plugin symbols dynamic. */ - if ((abfd->flags & BFD_PLUGIN) != 0) - dynsym = FALSE; - if (definition) { h->target_internal = isym->st_target_internal; @@ -5192,7 +5184,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info) } } - if (dynsym && h->dynindx == -1) + if (dynsym && (abfd->flags & BFD_PLUGIN) == 0 && h->dynindx == -1) { if (! bfd_elf_link_record_dynamic_symbol (info, h)) goto error_free_vers; diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp index adad1e4895..684d1db314 100644 --- a/ld/testsuite/ld-plugin/lto.exp +++ b/ld/testsuite/ld-plugin/lto.exp @@ -320,21 +320,6 @@ set lto_link_elf_tests [list \ [list "PR ld/13244" \ "-shared -O2 -fPIC -flto -fuse-linker-plugin -nostdlib" "-O2 -fno-early-inlining -flto" \ {pr13244.c} {{"readelf" {-s --wide} "pr13244.d"}} "pr13244.so" "c"] \ - [list "Build libpr15146a.a" \ - "$plug_opt" "-flto -O2" \ - {pr15146a.c} {} "lib15146a.a"] \ - [list "Build pr15146b.so" \ - "-shared" "-O2 -fpic" \ - {pr15146b.c} {} "pr15146b.so" "c"] \ - [list "Build pr15146c.so" \ - "-shared -Wl,--no-as-needed tmpdir/pr15146b.so" "-O2 -fpic $no_lto" \ - {pr15146c.c} {} "pr15146c.so" "c"] \ - [list "PR ld/15146 (1)" \ - "-O2 -flto -fuse-linker-plugin -Wl,-rpath-link,. -Wl,--no-copy-dt-needed-entries -Wl,--no-as-needed tmpdir/pr15146a.o tmpdir/pr15146c.so" "" \ - {dummy.c} {{"readelf" {-d} "pr15146.d"}} "pr15146a.exe"] \ - [list "Build libpr15146d.a" \ - "$plug_opt" "-flto -O2" \ - {pr15146d.c} {} "lib15146d.a"] \ [list "Build libpr16746a.a" \ "" "" \ {pr16746a.c pr16746b.c} {} "lib15146a.a"] \ @@ -605,13 +590,6 @@ run_cc_link_tests $lto_compile_elf_tests # Restrict these to ELF targets that support shared libs and PIC. if { [is_elf_format] && [check_lto_shared_available] } { run_cc_link_tests $lto_link_elf_tests - set testname "PR ld/15146 (2)" - set exec_output [run_host_cmd "$CC" "-O2 -flto -fuse-linker-plugin -Wl,-rpath-link,. -Wl,--no-copy-dt-needed-entries -Wl,--no-as-needed tmpdir/pr15146d.o tmpdir/pr15146c.so"] - if { [ regexp "undefined reference to symbol '\\.?xxx'" $exec_output ] } { - pass $testname - } { - fail $testname - } set testname "PR ld/16746 (3)" set exec_output [run_host_cmd "$CC" "-O2 -flto -fuse-linker-plugin tmpdir/pr16746b.o tmpdir/pr16746d.o"] if { [ regexp "warning: \\.?foobar" $exec_output ] && ![ regexp "symbol from plugin" $exec_output ] } { diff --git a/ld/testsuite/ld-plugin/pr15146.d b/ld/testsuite/ld-plugin/pr15146.d deleted file mode 100644 index 48d4b85446..0000000000 --- a/ld/testsuite/ld-plugin/pr15146.d +++ /dev/null @@ -1,4 +0,0 @@ -#failif -#... - +0x[0-9a-f]+ +\(NEEDED\) +Shared library: +\[.*pr15146b.so\] -#... diff --git a/ld/testsuite/ld-plugin/pr15146a.c b/ld/testsuite/ld-plugin/pr15146a.c deleted file mode 100644 index a22860af5b..0000000000 --- a/ld/testsuite/ld-plugin/pr15146a.c +++ /dev/null @@ -1,13 +0,0 @@ -extern int xxx; - -int -bar (void) -{ - return xxx; -} - -int -main () -{ - return 0; -} diff --git a/ld/testsuite/ld-plugin/pr15146b.c b/ld/testsuite/ld-plugin/pr15146b.c deleted file mode 100644 index 90eb21ea55..0000000000 --- a/ld/testsuite/ld-plugin/pr15146b.c +++ /dev/null @@ -1 +0,0 @@ -int xxx = 3; diff --git a/ld/testsuite/ld-plugin/pr15146c.c b/ld/testsuite/ld-plugin/pr15146c.c deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/ld/testsuite/ld-plugin/pr15146d.c b/ld/testsuite/ld-plugin/pr15146d.c deleted file mode 100644 index ba1e0abfa6..0000000000 --- a/ld/testsuite/ld-plugin/pr15146d.c +++ /dev/null @@ -1,7 +0,0 @@ -extern int xxx; - -int -main () -{ - return xxx; -} -- Alan Modra Australia Development Lab, IBM