Re: Fix folding of EQ/NE_EXPR WRT symtab aliases

[Jan Hubicka <hubicka at ucw dot cz>]

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;
+}