From patchwork Tue Feb 10 21:19:14 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Henderson X-Patchwork-Id: 438576 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id EA4571401AB for ; Wed, 11 Feb 2015 08:20:02 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=LHc0YBvuDMlhQ7zm5 ZnV6/q5rtd3kGuQOeCesDBsvJOWk6kL5e32Y+Wl+rcaPFdO4rvNjg2DtOKaBjqko ra5IYNHpZh/9+3c3vwHGXkuV8BwpHhTdOEOzf3uof7dFB3OKZ8aJHuT0KrUgmZzT uiRuiWl4Hx2MKCUp3fDoWAJl38= 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 :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=S6wIapRfKfZUBbBBYMlVh4z rEqI=; b=cFeF+LUrN2sl8e9X2b4NUgSMy0QlQBgHU0t2lyoPqeSYRDIPi3Rl1Ip 1/2Dh+3yhyzr2yq1QMilCcUry0P6hvlRpaP2pOB5k2qvuZRDlTQ10Y7yi+0fP/Rt iePbexLNZhYjfNrGyZds0gZUAQChyYbSFJox8Un0jv7ErNiJOYwI= Received: (qmail 13052 invoked by alias); 10 Feb 2015 21:19:25 -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 13039 invoked by uid 89); 10 Feb 2015 21:19:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.3 required=5.0 tests=AWL, BAYES_20, KAM_STOCKGEN, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 10 Feb 2015 21:19:23 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t1ALJHpv013696 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 10 Feb 2015 16:19:17 -0500 Received: from anchor.twiddle.net (vpn-232-27.phx2.redhat.com [10.3.232.27]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t1ALJFBH031780; Tue, 10 Feb 2015 16:19:16 -0500 Message-ID: <54DA75D2.40402@redhat.com> Date: Tue, 10 Feb 2015 13:19:14 -0800 From: Richard Henderson User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: "H.J. Lu" CC: Jack Howarth , GCC Patches , Mike Stump , Iain Sandoe , Jan Hubicka Subject: Re: [PATCH] PR rtl-optimization/32219: optimizer causes wrong code in pic/hidden/weak symbol checking References: <20150206162314.GA12597@intel.com> <20150207122739.GA25185@gmail.com> <20150207155606.GA14159@gmail.com> <20150207164507.GA19402@gmail.com> In-Reply-To: <20150207164507.GA19402@gmail.com> X-IsSubscribed: yes On 02/07/2015 08:45 AM, H.J. Lu wrote: > Here is an updated patch. This is an annoying comment to differentiate 3 different versions, FYI. > @@ -6826,11 +6817,18 @@ default_binds_local_p_1 (const_tree exp, int shlib) > && (TREE_STATIC (exp) || DECL_EXTERNAL (exp))) > { > varpool_node *vnode = varpool_node::get (exp); > - if (vnode && (resolution_local_p (vnode->resolution) || vnode->in_other_partition)) > - resolved_locally = true; > - if (vnode > - && resolution_to_local_definition_p (vnode->resolution)) > - resolved_to_local_def = true; > + /* If not building shared library, common or initialized symbols > + are also resolved locally, regardless they are weak or not if > + weak_dominate is true. */ > + if (vnode) > + { > + if ((weak_dominate && !shlib && vnode->definition) > + || vnode->in_other_partition > + || resolution_local_p (vnode->resolution)) > + resolved_locally = true; > + if (resolution_to_local_definition_p (vnode->resolution)) > + resolved_to_local_def = true; > + } > } > else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp)) > { Not the same test for a function_decl? That's certainly a mistake. Indeed, I believe we can now unify these two cases by using the base symtab_node instead of varpool_node and cgraph_node. I'm a bit confused about why we have both resolved_to_local_def vs resolved_locally. At least within this function, which only cares about module locality rather than object file locality. Honza? > @@ -6859,9 +6857,15 @@ default_binds_local_p_1 (const_tree exp, int shlib) > else if (! TREE_PUBLIC (exp)) > local_p = true; > /* A variable is local if the user has said explicitly that it will > - be. */ > + be and it is resolved or defined locally, not compiling for PIC, > + not weak or weak_dominate is false. */ > else if ((DECL_VISIBILITY_SPECIFIED (exp) > || resolved_to_local_def) > + && (!weak_dominate > + || resolved_locally > + || !flag_pic > + || !DECL_EXTERNAL (exp) > + || !DECL_WEAK (exp)) > && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT) > local_p = true; I think this test is conflating several issues, and as such it's very confusing. As an existing issue, I'm not sure why "specified" visibility is any different from unspecified visibility. As far as I'm aware, the "specified" bit simply means that the decl doesn't inherit inherit visibility from the class, or from the command-line. But once we're this far, the visibility actually applied to the symbol should be all that matters. Unfortunately, this test is 11 years old, from r85167. It came in as part of the implementation of the visibility attribute for the class. I believe the next test should be else if (DECL_WEAK (exp) && !resolved_locally) local_p = false; Given the change above, resolved_locally will now be true for (weak && defined && weak_dominate), and therefore we need not reiterate that test. I believe the next test should be dictated by normal non-lto usage like extern void bar(void) __attribute__((visibility("hidden"))); void foo(void) { ... bar(); ...) where the user is expecting that the function call make use of a simpler instruction sequence, probably avoiding an PLT. Therefore else if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT) local_p = true; Since we have already excluded undef-weak, we know that any symbol here is either defined or strong, and non-default visibility must resolve it locally. > /* Variables defined outside this object might not be local. */ > @@ -6881,8 +6885,9 @@ default_binds_local_p_1 (const_tree exp, int shlib) > else if (shlib) > local_p = false; > /* Uninitialized COMMON variable may be unified with symbols > - resolved from other modules. */ > + resolved from other modules unless weak_dominate is true. */ > else if (DECL_COMMON (exp) > + && !weak_dominate > && !resolved_locally Like the weak test, surely weak_dominate has already been accounted for. > @@ -7445,9 +7465,10 @@ default_elf_asm_output_external (FILE *file ATTRIBUTE_UNUSED, > { > /* We output the name if and only if TREE_SYMBOL_REFERENCED is > set in order to avoid putting out names that are never really > - used. */ > + used. Always output visibility specified in the source. */ > if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)) > - && targetm.binds_local_p (decl)) > + && (DECL_VISIBILITY_SPECIFIED (decl) > + || targetm.binds_local_p (decl))) > maybe_assemble_visibility (decl); Explain? I'm testing the following in the meantime, in order to validate my hypotheses above. r~ diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c index 48a4b35..a1ed927 100644 --- a/gcc/cgraphunit.c +++ b/gcc/cgraphunit.c @@ -803,8 +803,10 @@ varpool_node::finalize_decl (tree decl) if (node->definition) return; - notice_global_symbol (decl); + /* Set definition first before calling notice_global_symbol so that + it is available to notice_global_symbol. */ node->definition = true; + notice_global_symbol (decl); if (TREE_THIS_VOLATILE (decl) || DECL_PRESERVE_P (decl) /* Traditionally we do not eliminate static variables when not optimizing and when not doing toplevel reoder. */ diff --git a/gcc/testsuite/gcc.target/i386/pr64317.c b/gcc/testsuite/gcc.target/i386/pr64317.c index 33f5b5d..32969fc 100644 --- a/gcc/testsuite/gcc.target/i386/pr64317.c +++ b/gcc/testsuite/gcc.target/i386/pr64317.c @@ -1,7 +1,7 @@ /* { dg-do compile { target { *-*-linux* && ia32 } } } */ /* { dg-options "-O2 -fpie" } */ /* { dg-final { scan-assembler "addl\[ \\t\]+\[$\]_GLOBAL_OFFSET_TABLE_, %ebx" } } */ -/* { dg-final { scan-assembler "movl\[ \\t\]+c@GOT\[(\]%ebx\[)\]" } } */ +/* { dg-final { scan-assembler "movl\[ \\t\]+c@GOTOFF\[(\]%ebx\[)\]" } } */ /* { dg-final { scan-assembler-not "movl\[ \\t\]+\[0-9]+\[(\]%esp\[)\], %ebx" } } */ long c; diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c index 159fa2b..3c45f37 100644 --- a/gcc/tree-eh.c +++ b/gcc/tree-eh.c @@ -3859,6 +3859,17 @@ mark_reachable_handlers (sbitmap *r_reachablep, sbitmap *lp_reachablep) gimple_eh_dispatch_region ( as_a (stmt))); break; + case GIMPLE_CALL: + if (gimple_call_builtin_p (stmt, BUILT_IN_EH_COPY_VALUES)) + for (int i = 0; i < 2; ++i) + { + tree rt = gimple_call_arg (stmt, i); + HOST_WIDE_INT ri = tree_to_shwi (rt); + + gcc_assert (ri = (int)ri); + bitmap_set_bit (r_reachable, ri); + } + break; default: break; } diff --git a/gcc/varasm.c b/gcc/varasm.c index eb65b1f..9d041c7 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -6776,123 +6776,98 @@ default_use_anchors_for_symbol_p (const_rtx symbol) return true; } -/* Return true when RESOLUTION indicate that symbol will be bound to the - definition provided by current .o file. */ - static bool -resolution_to_local_definition_p (enum ld_plugin_symbol_resolution resolution) +default_binds_local_p_2 (const_tree exp, bool shlib, bool weak_dominate) { - return (resolution == LDPR_PREVAILING_DEF - || resolution == LDPR_PREVAILING_DEF_IRONLY_EXP - || resolution == LDPR_PREVAILING_DEF_IRONLY); -} - -/* Return true when RESOLUTION indicate that symbol will be bound locally - within current executable or DSO. */ - -static bool -resolution_local_p (enum ld_plugin_symbol_resolution resolution) -{ - return (resolution == LDPR_PREVAILING_DEF - || resolution == LDPR_PREVAILING_DEF_IRONLY - || resolution == LDPR_PREVAILING_DEF_IRONLY_EXP - || resolution == LDPR_PREEMPTED_REG - || resolution == LDPR_PREEMPTED_IR - || resolution == LDPR_RESOLVED_IR - || resolution == LDPR_RESOLVED_EXEC); -} - -/* Assume ELF-ish defaults, since that's pretty much the most liberal - wrt cross-module name binding. */ + /* A non-decl is an entry in the constant pool. */ + if (!DECL_P (exp)) + return true; -bool -default_binds_local_p (const_tree exp) -{ - return default_binds_local_p_1 (exp, flag_shlib); -} + /* Weakrefs may not bind locally, even though the weakref itself is always + static and therefore local. Similarly, the resolver for ifunc functions + might resolve to a non-local function. + FIXME: We can resolve the weakref case more curefuly by looking at the + weakref alias. */ + if (lookup_attribute ("weakref", DECL_ATTRIBUTES (exp)) + || (TREE_CODE (exp) == FUNCTION_DECL + && lookup_attribute ("ifunc", DECL_ATTRIBUTES (exp)))) + return false; -bool -default_binds_local_p_1 (const_tree exp, int shlib) -{ - bool local_p; - bool resolved_locally = false; - bool resolved_to_local_def = false; + /* Static variables are always local. */ + if (! TREE_PUBLIC (exp)) + return true; /* With resolution file in hands, take look into resolutions. We can't just return true for resolved_locally symbols, because dynamic linking might overwrite symbols in shared libraries. */ + symtab_node *node = NULL; if (TREE_CODE (exp) == VAR_DECL && TREE_PUBLIC (exp) && (TREE_STATIC (exp) || DECL_EXTERNAL (exp))) - { - varpool_node *vnode = varpool_node::get (exp); - if (vnode && (resolution_local_p (vnode->resolution) || vnode->in_other_partition)) - resolved_locally = true; - if (vnode - && resolution_to_local_definition_p (vnode->resolution)) - resolved_to_local_def = true; - } + node = varpool_node::get (exp); else if (TREE_CODE (exp) == FUNCTION_DECL && TREE_PUBLIC (exp)) + node = cgraph_node::get (exp); + + bool resolved_locally = false; + if (node) { - struct cgraph_node *node = cgraph_node::get (exp); - if (node - && (resolution_local_p (node->resolution) || node->in_other_partition)) + /* When not building shared library and weak_dominate is true: + weak, common or initialized symbols are resolved locally. */ + if ((weak_dominate && !shlib && node->definition) + || node->in_other_partition + || node->resolution == LDPR_PREVAILING_DEF + || node->resolution == LDPR_PREVAILING_DEF_IRONLY + || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP + || node->resolution == LDPR_PREEMPTED_REG + || node->resolution == LDPR_PREEMPTED_IR + || node->resolution == LDPR_RESOLVED_IR + || node->resolution == LDPR_RESOLVED_EXEC) resolved_locally = true; - if (node - && resolution_to_local_definition_p (node->resolution)) - resolved_to_local_def = true; } - /* A non-decl is an entry in the constant pool. */ - if (!DECL_P (exp)) - local_p = true; - /* Weakrefs may not bind locally, even though the weakref itself is always - static and therefore local. Similarly, the resolver for ifunc functions - might resolve to a non-local function. - FIXME: We can resolve the weakref case more curefuly by looking at the - weakref alias. */ - else if (lookup_attribute ("weakref", DECL_ATTRIBUTES (exp)) - || (TREE_CODE (exp) == FUNCTION_DECL - && lookup_attribute ("ifunc", DECL_ATTRIBUTES (exp)))) - local_p = false; - /* Static variables are always local. */ - else if (! TREE_PUBLIC (exp)) - local_p = true; - /* A variable is local if the user has said explicitly that it will - be. */ - else if ((DECL_VISIBILITY_SPECIFIED (exp) - || resolved_to_local_def) - && DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT) - local_p = true; + /* Undefined (or non-dominant) weak symbols are not defined locally. */ + if (DECL_WEAK (exp) && !resolved_locally) + return false; + + /* If a strong or defined weak symbol has non-default visibility, + it must be defined locally. */ + if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT) + return true; + /* Variables defined outside this object might not be local. */ - else if (DECL_EXTERNAL (exp) && !resolved_locally) - local_p = false; - /* If defined in this object and visibility is not default, must be - local. */ - else if (DECL_VISIBILITY (exp) != VISIBILITY_DEFAULT) - local_p = true; - /* Default visibility weak data can be overridden by a strong symbol - in another module and so are not local. */ - else if (DECL_WEAK (exp) - && !resolved_locally) - local_p = false; - /* If PIC, then assume that any global name can be overridden by - symbols resolved from other modules. */ - else if (shlib) - local_p = false; - /* Uninitialized COMMON variable may be unified with symbols - resolved from other modules. */ - else if (DECL_COMMON (exp) - && !resolved_locally - && (DECL_INITIAL (exp) == NULL - || (!in_lto_p && DECL_INITIAL (exp) == error_mark_node))) - local_p = false; + if (DECL_EXTERNAL (exp) && !resolved_locally) + return false; + + /* When compiling for a shared library, then assume that any global name + can be overridden by symbols resolved from other modules. */ + if (shlib) + return false; + + /* Uninitialized COMMON variable may be unified with symbols. */ + if (DECL_COMMON (exp) + && !resolved_locally + && (DECL_INITIAL (exp) == NULL + || (!in_lto_p && DECL_INITIAL (exp) == error_mark_node))) + return false; + /* Otherwise we're left with initialized (or non-common) global data which is of necessity defined locally. */ - else - local_p = true; + return true; +} + +/* Assume ELF-ish defaults, since that's pretty much the most liberal + wrt cross-module name binding. */ - return local_p; +bool +default_binds_local_p (const_tree exp) +{ + return default_binds_local_p_2 (exp, flag_shlib != 0, true); +} + +bool +default_binds_local_p_1 (const_tree exp, int shlib) +{ + return default_binds_local_p_2 (exp, shlib != 0, false); } /* Return true when references to DECL must bind to current definition in @@ -7445,9 +7420,10 @@ default_elf_asm_output_external (FILE *file ATTRIBUTE_UNUSED, { /* We output the name if and only if TREE_SYMBOL_REFERENCED is set in order to avoid putting out names that are never really - used. */ + used. Always output visibility specified in the source. */ if (TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)) - && targetm.binds_local_p (decl)) + && (DECL_VISIBILITY_SPECIFIED (decl) + || targetm.binds_local_p (decl))) maybe_assemble_visibility (decl); }