From patchwork Sun Dec 7 07:38:14 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Hubicka X-Patchwork-Id: 418449 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 45DCA1400DD for ; Sun, 7 Dec 2014 18:38:33 +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:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=JWKtpGkyTVRizpm1b TRYYxtmEl6yAn6CO8wQSouhtVEa/xiHP/1/x55KLbP4HuJt5NDdr4+g/ryuVLzS5 sjB3G+5kSVcKAy87j1n4jr47PSg79K+wu1tjbaElmIy2YS++qeSPbhFN6er5Bd3o lHK7pnHmhL4CglqTMnFdzfCxzI= 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:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=ZMqRVp6s62OtQCwHEFQP50V 5SJ4=; b=Yn338bfKkjpddjAxZixKM2dtUgZBCPTgZLuw097mbOjVbC6WLR3rKuz WZgvg21X3fEW6/JfwbucUy19JfMqdkyaI2p8nAlfa8goTGlE/v2o1h492E6HhTji iMR38lfvyuFHXAxlOIlJCf2KyXceWL8OgB0fGKTdKVvpBg3V5FxU= Received: (qmail 26849 invoked by alias); 7 Dec 2014 07:38:24 -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 26814 invoked by uid 89); 7 Dec 2014 07:38:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL, BAYES_00, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: nikam.ms.mff.cuni.cz Received: from nikam.ms.mff.cuni.cz (HELO nikam.ms.mff.cuni.cz) (195.113.20.16) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Sun, 07 Dec 2014 07:38:19 +0000 Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id E945B545A61; Sun, 7 Dec 2014 08:38:14 +0100 (CET) Date: Sun, 7 Dec 2014 08:38:14 +0100 From: Jan Hubicka To: Richard Biener Cc: Jan Hubicka , GCC Patches Subject: Re: Fix folding of EQ/NE_EXPR WRT symtab aliases Message-ID: <20141207073814.GA3707@kam.mff.cuni.cz> References: <20141203232831.GB16755@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Hi, this is simplified patch that only adds the equal_address_to predicate (and thus fixes issues with inccorect folding of speculative calls). Hopefully it will mek it easier to handle the rest of fold-const incrementally. Bootstrapped/regtested x86_64-linux, comitted * symtab.c (symtab_node::equal_address_to): New function. * cgraph.h (symtab_node::equal_address_to): Declare. * fold-const.c (fold_comparison, fold_binary_loc): Use it. * c-family/c-common.c: Refuse weaks for symbols that can not change visibility. * gcc.dg/addr_equal-1.c: New testcase. Index: symtab.c =================================================================== --- symtab.c (revision 218457) +++ symtab.c (working copy) @@ -1860,3 +1860,90 @@ symtab_node::nonzero_address () return true; return false; } + +/* Return 0 if symbol is known to have different address than S2, + Return 1 if symbol is known to have same address as S2, + return 2 otherwise. */ +int +symtab_node::equal_address_to (symtab_node *s2) +{ + enum availability avail1, avail2; + + /* A Shortcut: equivalent symbols are always equivalent. */ + if (this == s2) + return 1; + + /* For non-interposable aliases, lookup and compare their actual definitions. + Also check if the symbol needs to bind to given definition. */ + symtab_node *rs1 = ultimate_alias_target (&avail1); + symtab_node *rs2 = s2->ultimate_alias_target (&avail2); + bool binds_local1 = rs1->analyzed && decl_binds_to_current_def_p (this->decl); + bool binds_local2 = rs2->analyzed && decl_binds_to_current_def_p (s2->decl); + bool really_binds_local1 = binds_local1; + bool really_binds_local2 = binds_local2; + + /* Addresses of vtables and virtual functions can not be used by user + code and are used only within speculation. In this case we may make + symbol equivalent to its alias even if interposition may break this + rule. Doing so will allow us to turn speculative inlining into + non-speculative more agressively. */ + if (DECL_VIRTUAL_P (this->decl) && avail1 >= AVAIL_AVAILABLE) + binds_local1 = true; + if (DECL_VIRTUAL_P (s2->decl) && avail2 >= AVAIL_AVAILABLE) + binds_local2 = true; + + /* If both definitions are available we know that even if they are bound + to other unit they must be defined same way and therefore we can use + equivalence test. */ + if (rs1 != rs2 && avail1 >= AVAIL_AVAILABLE && avail2 >= AVAIL_AVAILABLE) + binds_local1 = binds_local2 = true; + + if ((binds_local1 ? rs1 : this) + == (binds_local2 ? rs2 : s2)) + { + /* We made use of the fact that alias is not weak. */ + if (binds_local1 && rs1 != this) + refuse_visibility_changes = true; + if (binds_local2 && rs2 != s2) + s2->refuse_visibility_changes = true; + return 1; + } + + /* If both symbols may resolve to NULL, we can not really prove them different. */ + if (!nonzero_address () && !s2->nonzero_address ()) + return 2; + + /* Except for NULL, functions and variables never overlap. */ + if (TREE_CODE (decl) != TREE_CODE (s2->decl)) + return 0; + + /* If one of the symbols is unresolved alias, punt. */ + if (rs1->alias || rs2->alias) + return 2; + + /* If we have a non-interposale definition of at least one of the symbols + and the other symbol is different, we know other unit can not interpose + it to the first symbol; all aliases of the definition needs to be + present in the current unit. */ + if (((really_binds_local1 || really_binds_local2) + /* If we have both definitions and they are different, we know they + will be different even in units they binds to. */ + || (binds_local1 && binds_local2)) + && rs1 != rs2) + { + /* We make use of the fact that one symbol is not alias of the other + and that the definition is non-interposable. */ + refuse_visibility_changes = true; + s2->refuse_visibility_changes = true; + rs1->refuse_visibility_changes = true; + rs2->refuse_visibility_changes = true; + return 0; + } + + /* TODO: Alias oracle basically assume that addresses of global variables + are different unless they are declared as alias of one to another. + We probably should be consistent and use this fact here, too, and update + alias oracle to use this predicate. */ + + return 2; +} Index: c-family/c-common.c =================================================================== --- c-family/c-common.c (revision 218457) +++ c-family/c-common.c (working copy) @@ -7781,7 +7781,12 @@ handle_weak_attribute (tree *node, tree } else if (TREE_CODE (*node) == FUNCTION_DECL || TREE_CODE (*node) == VAR_DECL) - declare_weak (*node); + { + struct symtab_node *n = symtab_node::get (*node); + if (n && n->refuse_visibility_changes) + error ("%+D declared weak after being used", *node); + declare_weak (*node); + } else warning (OPT_Wattributes, "%qE attribute ignored", name); Index: cgraph.h =================================================================== --- cgraph.h (revision 218457) +++ cgraph.h (working copy) @@ -332,6 +332,11 @@ public: /* Return true if symbol is known to be nonzero. */ bool nonzero_address (); + /* Return 0 if symbol is known to have different address than S2, + Return 1 if symbol is known to have same address as S2, + return 2 otherwise. */ + int equal_address_to (symtab_node *s2); + /* Return symbol table node associated with DECL, if any, and NULL otherwise. */ static inline symtab_node *get (const_tree decl) Index: fold-const.c =================================================================== --- fold-const.c (revision 218457) +++ fold-const.c (working copy) @@ -8985,7 +8985,7 @@ fold_comparison (location_t loc, enum tr } } /* For non-equal bases we can simplify if they are addresses - of local binding decls or constants. */ + declarations with different addresses. */ else if (indirect_base0 && indirect_base1 /* We know that !operand_equal_p (base0, base1, 0) because the if condition was false. But make @@ -8993,16 +8993,13 @@ fold_comparison (location_t loc, enum tr && base0 != base1 && TREE_CODE (arg0) == ADDR_EXPR && TREE_CODE (arg1) == ADDR_EXPR - && (((TREE_CODE (base0) == VAR_DECL - || TREE_CODE (base0) == PARM_DECL) - && (targetm.binds_local_p (base0) - || CONSTANT_CLASS_P (base1))) - || CONSTANT_CLASS_P (base0)) - && (((TREE_CODE (base1) == VAR_DECL - || TREE_CODE (base1) == PARM_DECL) - && (targetm.binds_local_p (base1) - || CONSTANT_CLASS_P (base0))) - || CONSTANT_CLASS_P (base1))) + && DECL_P (base0) + && DECL_P (base1) + /* Watch for aliases. */ + && (!decl_in_symtab_p (base0) + || !decl_in_symtab_p (base1) + || !symtab_node::get_create (base0)->equal_address_to + (symtab_node::get_create (base1)))) { if (code == EQ_EXPR) return omit_two_operands_loc (loc, type, boolean_false_node, @@ -12257,33 +12254,23 @@ fold_binary_loc (location_t loc, unaliased symbols neither of which are extern (since we do not have access to attributes for externs), then we know the result. */ if (TREE_CODE (arg0) == ADDR_EXPR - && VAR_OR_FUNCTION_DECL_P (TREE_OPERAND (arg0, 0)) - && ! DECL_WEAK (TREE_OPERAND (arg0, 0)) - && ! lookup_attribute ("alias", - DECL_ATTRIBUTES (TREE_OPERAND (arg0, 0))) - && ! DECL_EXTERNAL (TREE_OPERAND (arg0, 0)) + && DECL_P (TREE_OPERAND (arg0, 0)) && TREE_CODE (arg1) == ADDR_EXPR - && VAR_OR_FUNCTION_DECL_P (TREE_OPERAND (arg1, 0)) - && ! DECL_WEAK (TREE_OPERAND (arg1, 0)) - && ! lookup_attribute ("alias", - DECL_ATTRIBUTES (TREE_OPERAND (arg1, 0))) - && ! DECL_EXTERNAL (TREE_OPERAND (arg1, 0))) - { - /* We know that we're looking at the address of two - non-weak, unaliased, static _DECL nodes. - - It is both wasteful and incorrect to call operand_equal_p - to compare the two ADDR_EXPR nodes. It is wasteful in that - all we need to do is test pointer equality for the arguments - to the two ADDR_EXPR nodes. It is incorrect to use - operand_equal_p as that function is NOT equivalent to a - C equality test. It can in fact return false for two - objects which would test as equal using the C equality - operator. */ - bool equal = TREE_OPERAND (arg0, 0) == TREE_OPERAND (arg1, 0); - return constant_boolean_node (equal - ? code == EQ_EXPR : code != EQ_EXPR, - type); + && DECL_P (TREE_OPERAND (arg1, 0))) + { + int equal; + + if (decl_in_symtab_p (TREE_OPERAND (arg0, 0)) + && decl_in_symtab_p (TREE_OPERAND (arg1, 0))) + equal = symtab_node::get_create (TREE_OPERAND (arg0, 0)) + ->equal_address_to (symtab_node::get_create + (TREE_OPERAND (arg1, 0))); + else + equal = TREE_OPERAND (arg0, 0) == TREE_OPERAND (arg1, 0); + if (equal != 2) + return constant_boolean_node (equal + ? code == EQ_EXPR : code != EQ_EXPR, + type); } /* Similarly for a NEGATE_EXPR. */ Index: testsuite/gcc.dg/addr_equal-1.c =================================================================== --- testsuite/gcc.dg/addr_equal-1.c (revision 0) +++ testsuite/gcc.dg/addr_equal-1.c (revision 0) @@ -0,0 +1,101 @@ +/* { dg-do run } */ +/* { dg-require-weak "" } */ +/* { dg-require-alias "" } */ +/* { dg-options "-O2" } */ +void abort (void); +extern int undef_var0, undef_var1; +extern __attribute__ ((weak)) int weak_undef_var0; +extern __attribute__ ((weak)) int weak_undef_var1; +__attribute__ ((weak)) int weak_def_var0; +int def_var0=0, def_var1=0; +static int alias_var0 __attribute__ ((alias("def_var0"))); +extern int weak_alias_var0 __attribute__ ((alias("def_var0"))) __attribute__ ((weak)); +void undef_fn0(void); +void undef_fn1(void); +void def_fn0(void) +{ +} +void def_fn1(void) +{ +} +__attribute__ ((weak)) +void weak_def_fn0(void) +{ +} +__attribute__ ((weak)) +void weak_def_fn1(void) +{ +} +__attribute__ ((weak)) void weak_undef_fn0(void); + +inline +void inline_fn0(void) +{ +} +inline +void inline_fn1(void) +{ +} + +int +main(int argc, char **argv) +{ + /* Two definitions are always different unless they can be interposed. */ + if (!__builtin_constant_p (def_fn0 == def_fn1)) + abort(); + if (def_fn0 == def_fn1) + abort(); + + if (!__builtin_constant_p (&def_var0 == &def_var1)) + abort(); + if (&def_var0 == &def_var1) + abort(); + + /* Same symbol is the same no matter on interposition. */ + if (!__builtin_constant_p (undef_fn0 != undef_fn0)) + abort (); + if (undef_fn0 != undef_fn0) + abort (); + + /* Do not get confused by same offset. */ + if (!__builtin_constant_p (&undef_var0 + argc != &undef_var0 + argc)) + abort (); + if (&undef_var0 + argc != &undef_var0 + argc) + abort (); + + /* Alias and its target is equivalent unless one of them can be interposed. */ + if (!__builtin_constant_p (&def_var0 != &alias_var0)) + abort (); + if (&def_var0 != &alias_var0 ) + abort (); + + if (__builtin_constant_p (&def_var0 != &weak_alias_var0)) + abort (); + if (&def_var0 != &weak_alias_var0) + abort (); + + /* Weak definitions may be both NULL. */ + if (__builtin_constant_p ((void *)weak_undef_fn0 == (void *)&weak_undef_var0)) + abort (); + if ((void *)weak_undef_fn0 != (void *)&weak_undef_var0) + abort (); + + /* Variables and functions do not share same memory locations otherwise. */ + if (!__builtin_constant_p ((void *)undef_fn0 == (void *)&undef_var0)) + abort (); + if ((void *)undef_fn0 == (void *)&undef_var0) + abort (); + + /* This works for cases where one object is just weakly defined, too. */ + if (!__builtin_constant_p ((void *)weak_undef_fn0 == (void *)&weak_def_var0)) + abort (); + if ((void *)weak_undef_fn0 == (void *)&weak_def_var0) + abort (); + + /* Inline functions are known to be different. */ + if (!__builtin_constant_p (inline_fn0 != inline_fn1)) + abort (); + if (inline_fn0 == inline_fn1) + abort (); + return 0; +}