On Wed, Dec 17, 2014 at 11:30 AM, Tejas Belagod <tejas.bela...@arm.com> wrote:
> On 07/12/14 07:38, Jan Hubicka wrote:
>>
>> 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;
>> +}
>>
>
> addr_equal-1.c is a nopic test as agreed in this thread.
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64328
>
> Here is a patch. Tested on aarch64-none-elf/-fPIC
> FAIL -> UNSUPPORTED
>
> OK to apply?
Ok.
Thanks,
Richard.
> Thanks,
> Tejas.
>
> Changelog:
>
> 2014-12-17 Tejas Belagod <tejas.bela...@arm.com>
>
> PR testsuite/64328
> * gcc.dg/addr_equal-1.c: Not supported for -fPIC.
