diff mbox

[WIP] Avoid -fcompare-debug regressions due to ipa-polymorphic-call stuff (PR ipa/65610)

Message ID 20150330180339.GL1746@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek March 30, 2015, 6:03 p.m. UTC 
Hi!

As discussed in the PR, we have -fcompare-debug failures, because
remove_unused_scope_block_p can sometimes remove blocks relevant
to ipa-polymorphic-call.c analysis (where it is checking if
there is some ctor or dtor in BLOCKs from current tree block upward in
BLOCK_SUPERCONTEXT hierarchy) - and for -fcompare-debug removes them just
with -g0 and not with -g.

This patch, bootstrapped/regtested on x86_64-linux and i686-linux, fixes
that, but only for !cfun->after_inlining - dunno if the polymorphic IPA
optimizations can happen even after IPA passes, if yes, then 
+  if (!cfun->after_inlining)
+    {
and
+    }
should be removed and the code reindented.
Also, looking for better name of the function if you have ideas.
And, I admit I haven't added a patch to gather statistics for how many
blocks aren't removed because of this compared to how many blocks that are
still removed.  As it is only about ctors or dtors, I'd hope it shouldn't be
anything significant.

2015-03-30  Jakub Jelinek  <jakub@redhat.com>

	PR ipa/65610
	* ipa-utils.h (ctor_dtor_block_p): Declare.
	* ipa-polymorphic-call.c (ctor_dtor_block_p): New function.
	(decl_maybe_in_construction_p, noncall_stmt_may_be_vtbl_ptr_store):
	Use it.
	* tree-ssa-live.c: Include ipa-utils.h and its dependencies.
	(remove_unused_scope_block_p): Add in_ctor_dtor_block
	argument.  Before inlining, preserve ctor_dtor_block_p blocks
	and the outermost block with FUNCTION_DECL BLOCK_ABSTRACT_ORIGIN
	inside of them.  Adjust recursive calls.
	(remove_unused_locals): Adjust remove_unused_scope_block_p caller.

	* g++.dg/ubsan/pr65610.C: New test.


	Jakub

Comments

Jan Hubicka March 30, 2015, 6:45 p.m. UTC | #1 
> Hi!
> 
> As discussed in the PR, we have -fcompare-debug failures, because
> remove_unused_scope_block_p can sometimes remove blocks relevant
> to ipa-polymorphic-call.c analysis (where it is checking if
> there is some ctor or dtor in BLOCKs from current tree block upward in
> BLOCK_SUPERCONTEXT hierarchy) - and for -fcompare-debug removes them just
> with -g0 and not with -g.
> 
> This patch, bootstrapped/regtested on x86_64-linux and i686-linux, fixes
> that, but only for !cfun->after_inlining - dunno if the polymorphic IPA
> optimizations can happen even after IPA passes, if yes, then 
> +  if (!cfun->after_inlining)
> +    {
> and
> +    }
> should be removed and the code reindented.

We do another round of polymorphic call analysis in PRE pass. (it is not that
important to do so as it is too late to inline the call, but we do that)

> Also, looking for better name of the function if you have ideas.

I think inlined_polymorphic_ctor_dtor_block_p may be descriptive name? :)

> +/* Return a FUNCTION_DECL if BLOCK represents a constructor or destructor.
> +   If CHECK_CLONES is true, also check for clones of ctor/dtors.  */
> +
> +tree
> +ctor_dtor_block_p (tree block, bool check_clones)
> +{
> +  tree fn = BLOCK_ABSTRACT_ORIGIN (block);
> +  if (fn == NULL || TREE_CODE (fn) != FUNCTION_DECL)
> +    return NULL_TREE;
> +
> +  if (TREE_CODE (TREE_TYPE (fn)) != METHOD_TYPE
> +      || (!DECL_CXX_CONSTRUCTOR_P (fn) && !DECL_CXX_DESTRUCTOR_P (fn)))
> +    {
> +      if (!check_clones)
> +	return NULL_TREE;
> +
> +      /* Watch for clones where we constant propagated the first
> +	 argument (pointer to the instance).  */
> +      fn = DECL_ABSTRACT_ORIGIN (fn);
> +      if (!fn
> +	  || TREE_CODE (TREE_TYPE (fn)) != METHOD_TYPE
> +	  || (!DECL_CXX_CONSTRUCTOR_P (fn) && !DECL_CXX_DESTRUCTOR_P (fn)))
> +	return NULL_TREE;
> +    }
> +
> +  if (flags_from_decl_or_type (fn) & (ECF_PURE | ECF_CONST))
> +    return NULL_TREE;
> +
> +  return fn;
> +}

Looking at it, the function is unnecesarily conservative - it can rule out all
non-polymorphic cdtors.  I will drop that to my TODO for next stage1.

There is one extra occurence of the same code in param_type_may_change_p.
Please unify it, too.  I should not have introduced the duplicates at first place
but initial version of the code had those all different.

> --- gcc/tree-ssa-live.c.jj	2015-03-09 08:05:13.000000000 +0100
> +++ gcc/tree-ssa-live.c	2015-03-30 11:57:25.492574441 +0200
> @@ -76,6 +76,10 @@ along with GCC; see the file COPYING3.
>  #include "diagnostic-core.h"
>  #include "debug.h"
>  #include "tree-ssa.h"
> +#include "lto-streamer.h"
> +#include "ipa-ref.h"
> +#include "cgraph.h"
> +#include "ipa-utils.h"

Header flattening still makes me sad ;(
>  
>  #ifdef ENABLE_CHECKING
>  static void  verify_live_on_entry (tree_live_info_p);
> @@ -509,11 +513,30 @@ mark_scope_block_unused (tree scope)
>     done by the inliner.  */
>  
>  static bool
> -remove_unused_scope_block_p (tree scope)
> +remove_unused_scope_block_p (tree scope, bool in_ctor_dtor_block)
>  {
>    tree *t, *next;
>    bool unused = !TREE_USED (scope);
>    int nsubblocks = 0;
> +  if (!cfun->after_inlining)
> +    {
> +      /* For ipa-polymorphic-call.c purposes, preserve blocks:
> +	 1) with BLOCK_ABSTRACT_ORIGIN of a ctor/dtor or their clones  */
> +      if (ctor_dtor_block_p (scope, true))

Please drop the cfun->after_inlining check.
I do not think it is major problem - the blocks of inlined cdtors should not be that
common (we can alternatively add the check for the polymorphic type in the predicate
above - polymorphic cdtors are even less common, but I doubt that is going to make
memory use difference).

Patch is OK with these changes.
Thanks a lot!
Honza
diff mbox

Patch

--- gcc/ipa-utils.h.jj	2015-03-09 08:05:07.000000000 +0100
+++ gcc/ipa-utils.h	2015-03-30 11:35:44.289599703 +0200
@@ -70,6 +70,7 @@  bool possible_polymorphic_call_target_p
 				         const ipa_polymorphic_call_context &,
 					 struct cgraph_node *);
 tree method_class_type (const_tree);
+tree ctor_dtor_block_p (tree, bool);
 bool decl_maybe_in_construction_p (tree, tree, gimple, tree);
 tree vtable_pointer_value_to_binfo (const_tree);
 bool vtable_pointer_value_to_vtable (const_tree, tree *, unsigned HOST_WIDE_INT *);
--- gcc/ipa-polymorphic-call.c.jj	2015-03-09 08:05:06.000000000 +0100
+++ gcc/ipa-polymorphic-call.c	2015-03-30 11:24:48.280199943 +0200
@@ -513,6 +513,38 @@  contains_type_p (tree outer_type, HOST_W
 }
 
 
+/* Return a FUNCTION_DECL if BLOCK represents a constructor or destructor.
+   If CHECK_CLONES is true, also check for clones of ctor/dtors.  */
+
+tree
+ctor_dtor_block_p (tree block, bool check_clones)
+{
+  tree fn = BLOCK_ABSTRACT_ORIGIN (block);
+  if (fn == NULL || TREE_CODE (fn) != FUNCTION_DECL)
+    return NULL_TREE;
+
+  if (TREE_CODE (TREE_TYPE (fn)) != METHOD_TYPE
+      || (!DECL_CXX_CONSTRUCTOR_P (fn) && !DECL_CXX_DESTRUCTOR_P (fn)))
+    {
+      if (!check_clones)
+	return NULL_TREE;
+
+      /* Watch for clones where we constant propagated the first
+	 argument (pointer to the instance).  */
+      fn = DECL_ABSTRACT_ORIGIN (fn);
+      if (!fn
+	  || TREE_CODE (TREE_TYPE (fn)) != METHOD_TYPE
+	  || (!DECL_CXX_CONSTRUCTOR_P (fn) && !DECL_CXX_DESTRUCTOR_P (fn)))
+	return NULL_TREE;
+    }
+
+  if (flags_from_decl_or_type (fn) & (ECF_PURE | ECF_CONST))
+    return NULL_TREE;
+
+  return fn;
+}
+
+
 /* We know that the instance is stored in variable or parameter
    (not dynamically allocated) and we want to disprove the fact
    that it may be in construction at invocation of CALL.
@@ -552,28 +584,8 @@  decl_maybe_in_construction_p (tree base,
 
   for (tree block = gimple_block (call); block && TREE_CODE (block) == BLOCK;
        block = BLOCK_SUPERCONTEXT (block))
-    if (BLOCK_ABSTRACT_ORIGIN (block)
-	&& TREE_CODE (BLOCK_ABSTRACT_ORIGIN (block)) == FUNCTION_DECL)
+    if (tree fn = ctor_dtor_block_p (block, !base || is_global_var (base)))
       {
-	tree fn = BLOCK_ABSTRACT_ORIGIN (block);
-
-	if (TREE_CODE (TREE_TYPE (fn)) != METHOD_TYPE
-	    || (!DECL_CXX_CONSTRUCTOR_P (fn)
-		&& !DECL_CXX_DESTRUCTOR_P (fn)))
-	  {
-	    /* Watch for clones where we constant propagated the first
-	       argument (pointer to the instance).  */
-	    fn = DECL_ABSTRACT_ORIGIN (fn);
-	    if (!fn
-		|| (base && !is_global_var (base))
-	        || TREE_CODE (TREE_TYPE (fn)) != METHOD_TYPE
-		|| (!DECL_CXX_CONSTRUCTOR_P (fn)
-		    && !DECL_CXX_DESTRUCTOR_P (fn)))
-	      continue;
-	  }
-	if (flags_from_decl_or_type (fn) & (ECF_PURE | ECF_CONST))
-	  continue;
-
 	tree type = TYPE_MAIN_VARIANT (method_class_type (TREE_TYPE (fn)));
 
 	if (!outer_type || !types_odr_comparable (type, outer_type))
@@ -1163,15 +1175,7 @@  noncall_stmt_may_be_vtbl_ptr_store (gimp
        block = BLOCK_SUPERCONTEXT (block))
     if (BLOCK_ABSTRACT_ORIGIN (block)
 	&& TREE_CODE (BLOCK_ABSTRACT_ORIGIN (block)) == FUNCTION_DECL)
-      {
-	tree fn = BLOCK_ABSTRACT_ORIGIN (block);
-
-	if (flags_from_decl_or_type (fn) & (ECF_PURE | ECF_CONST))
-	  return false;
-	return (TREE_CODE (TREE_TYPE (fn)) == METHOD_TYPE
-		&& (DECL_CXX_CONSTRUCTOR_P (fn)
-		    || DECL_CXX_DESTRUCTOR_P (fn)));
-      }
+      return ctor_dtor_block_p (block, false);
   return (TREE_CODE (TREE_TYPE (current_function_decl)) == METHOD_TYPE
 	  && (DECL_CXX_CONSTRUCTOR_P (current_function_decl)
 	      || DECL_CXX_DESTRUCTOR_P (current_function_decl)));
--- gcc/tree-ssa-live.c.jj	2015-03-09 08:05:13.000000000 +0100
+++ gcc/tree-ssa-live.c	2015-03-30 11:57:25.492574441 +0200
@@ -76,6 +76,10 @@  along with GCC; see the file COPYING3.
 #include "diagnostic-core.h"
 #include "debug.h"
 #include "tree-ssa.h"
+#include "lto-streamer.h"
+#include "ipa-ref.h"
+#include "cgraph.h"
+#include "ipa-utils.h"
 
 #ifdef ENABLE_CHECKING
 static void  verify_live_on_entry (tree_live_info_p);
@@ -509,11 +513,30 @@  mark_scope_block_unused (tree scope)
    done by the inliner.  */
 
 static bool
-remove_unused_scope_block_p (tree scope)
+remove_unused_scope_block_p (tree scope, bool in_ctor_dtor_block)
 {
   tree *t, *next;
   bool unused = !TREE_USED (scope);
   int nsubblocks = 0;
+  if (!cfun->after_inlining)
+    {
+      /* For ipa-polymorphic-call.c purposes, preserve blocks:
+	 1) with BLOCK_ABSTRACT_ORIGIN of a ctor/dtor or their clones  */
+      if (ctor_dtor_block_p (scope, true))
+	{
+	  in_ctor_dtor_block = true;
+	  unused = false;
+	}
+      /* 2) inside such blocks, the outermost block with BLOCK_ABSTRACT_ORIGIN
+            being a FUNCTION_DECL.  */
+      else if (in_ctor_dtor_block
+	       && BLOCK_ABSTRACT_ORIGIN (scope)
+	       && TREE_CODE (BLOCK_ABSTRACT_ORIGIN (scope)) == FUNCTION_DECL)
+	{
+	  in_ctor_dtor_block = false;
+	  unused = false;
+	}
+    }
 
   for (t = &BLOCK_VARS (scope); *t; t = next)
     {
@@ -594,7 +617,7 @@  remove_unused_scope_block_p (tree scope)
     }
 
   for (t = &BLOCK_SUBBLOCKS (scope); *t ;)
-    if (remove_unused_scope_block_p (*t))
+    if (remove_unused_scope_block_p (*t, in_ctor_dtor_block))
       {
 	if (BLOCK_SUBBLOCKS (*t))
 	  {
@@ -959,7 +982,7 @@  remove_unused_locals (void)
       cfun->local_decls->truncate (dstidx);
     }
 
-  remove_unused_scope_block_p (DECL_INITIAL (current_function_decl));
+  remove_unused_scope_block_p (DECL_INITIAL (current_function_decl), false);
   clear_unused_block_pointer ();
 
   BITMAP_FREE (usedvars);
--- gcc/testsuite/g++.dg/ubsan/pr65610.C.jj	2015-03-30 12:01:09.595952739 +0200
+++ gcc/testsuite/g++.dg/ubsan/pr65610.C	2015-03-30 12:00:35.000000000 +0200
@@ -0,0 +1,57 @@ 
+// PR ipa/65610
+// { dg-do compile }
+// { dg-options "-std=c++11 -fsanitize=undefined -fno-sanitize=vptr -fcompare-debug" }
+
+class A;
+class B {};
+enum C { D };
+class E;
+class F;
+class G;
+class H
+{
+  F m1 (const A &t) const;
+  G m2 () const;
+};
+class G {};
+template <class S, class T>
+class I;
+template <class S, class T>
+class J
+{
+  friend class I <S,T>;
+  J <S,T> *j;
+};
+template <class S, class T>
+struct I
+{
+  virtual ~I ();
+  virtual void m3 (void *p) {}
+  J <S,T> *i;
+  void m4 (J <S,T>*& t);
+};
+template <class S, class T>
+void I <S,T>::m4 (J <S,T> * &t)
+{
+  m4 (t->j);
+  m3 (t);
+}
+template <class S, class T>
+I <S,T>::~I ()
+{
+  m4 (i);
+}
+struct F
+{
+  explicit inline F (C v);
+  inline ~F ();
+  I <B, E> f;
+};
+inline F::F (C v) {}
+inline F::~F () {}
+F H::m1 (const A &t) const
+{
+  F q (D);
+  G r = m2 ();
+  return q;
+}