C++ PATCH for c++/61687 (extra errors with -O2)

On 07/24/2014 07:36 PM, Jan Hubicka wrote:
> ipa-deivrt has code to do the tracking for you. All is needed is to cal get_odr_type
> for all polymorphic type that we care about.  build_type_inheritance_graph just
> walks all virtual methods to register all polymorphic types that matters (i.e.
> have methods associated with them) into the datastructure.  I think within C++
> FE we can just reigster all polymoprhic types we produce.

Makes sense.

> I see that during the reachability walk new types may be instantiated. This may
> extend target lists earlier visited, I guess we need to add some way to make
> to track this?

True.

And more problematic, doing this in a single TU isn't good enough; there 
are cases where LTO can devirtualize a call to a target that isn't 
declared in the TU where the call occurs.  That is,

foo.h:
struct A { virtual ~A() {} };
void g(A*);

bar.h:
#include "foo.h"
struct B: A { virtual void f(); };

one.C:
#include "foo.h"
void g(A* p) { delete p; }

two.C:
#include "bar.h"
int main() { g(new B); }

three.C:
#include "bar.h"
void B::f() {}

$ gcc -fpic -shared -fvisibility-inlines-hidden three.C -o libfoo.so
$ gcc -O2 -flto -fvisibility-inlines-hidden one.C two.C libfoo.so
/home/jason/gtt/foo/one.C:2: undefined reference to `B::~B()'

To deal with this I guess we can either keep -fuse-all-virtuals or not 
devirtualize in LTO to something that is hidden and not defined.  Thoughts?

> On 07/24/2014 07:36 PM, Jan Hubicka wrote:
> >ipa-deivrt has code to do the tracking for you. All is needed is to cal get_odr_type
> >for all polymorphic type that we care about.  build_type_inheritance_graph just
> >walks all virtual methods to register all polymorphic types that matters (i.e.
> >have methods associated with them) into the datastructure.  I think within C++
> >FE we can just reigster all polymoprhic types we produce.
> 
> Makes sense.
> 
> >I see that during the reachability walk new types may be instantiated. This may
> >extend target lists earlier visited, I guess we need to add some way to make
> >to track this?
> 
> True.
> 
> And more problematic, doing this in a single TU isn't good enough;
> there are cases where LTO can devirtualize a call to a target that
> isn't declared in the TU where the call occurs.  That is,

Yeah, in LTO I generally apply scheme that I drop this kind of devirtualizations
to the floor if the COMDAT function happens to be not reachable in any of the units.
This is just problem when the class is keyed or constructed outside of LTO world
that seems resonable case to drop a bit of optimization.

But obvoiusly for me it is just code quality issue, we need to figure out what to
do for correctness.
> 
> foo.h:
> struct A { virtual ~A() {} };
> void g(A*);
> 
> bar.h:
> #include "foo.h"
> struct B: A { virtual void f(); };
> 
> one.C:
> #include "foo.h"
> void g(A* p) { delete p; }
> 
> two.C:
> #include "bar.h"
> int main() { g(new B); }
> 
> three.C:
> #include "bar.h"
> void B::f() {}
> 
> $ gcc -fpic -shared -fvisibility-inlines-hidden three.C -o libfoo.so
> $ gcc -O2 -flto -fvisibility-inlines-hidden one.C two.C libfoo.so
> /home/jason/gtt/foo/one.C:2: undefined reference to `B::~B()'
> 
> To deal with this I guess we can either keep -fuse-all-virtuals or
> not devirtualize in LTO to something that is hidden and not defined.
> Thoughts?

We decide what we can devirtualize in can_refer_decl_in_current_unit_p.
So the problem is that we have functions that should be COMDAT but they are not
because we decide to not produce their body (believing it is not reachable).  Can't
we arrange them havin a special flag (CAN_NOT_REFFER) or perhaps just COMDAT flag?
COMDAT flag will prevent us from introducing a direct referencebecause we know we don't
have body....

I will take a look at the patch later today

Thanks!
Honza
> 

> commit ac04d4b08190593299c8c94431d5e43384514096
> Author: Jason Merrill <jason@redhat.com>
> Date:   Fri Jul 25 09:15:02 2014 -0400
> 
>     mark_virtual_overrides
> 
> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> index c318cad..75711a5 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -1272,10 +1272,6 @@ funsigned-char
>  C ObjC C++ ObjC++ LTO Var(flag_signed_char, 0)
>  Make \"char\" unsigned by default
>  
> -fuse-all-virtuals
> -C++ ObjC++ Var(flag_use_all_virtuals) Init(1)
> -Treat all virtual functions as odr-used
> -
>  fuse-cxa-atexit
>  C++ ObjC++ Var(flag_use_cxa_atexit) Init(DEFAULT_USE_CXA_ATEXIT)
>  Use __cxa_atexit to register destructors
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index 4d37c65..969e730 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -7364,6 +7364,8 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
>  				ba_any, NULL, complain);
>        gcc_assert (binfo && binfo != error_mark_node);
>  
> +      note_fn_called_virtually (fn);
> +
>        /* Warn about deprecated virtual functions now, since we're about
>  	 to throw away the decl.  */
>        if (TREE_DEPRECATED (fn))
> diff --git a/gcc/cp/class.c b/gcc/cp/class.c
> index 0f611e1..1401069 100644
> --- a/gcc/cp/class.c
> +++ b/gcc/cp/class.c
> @@ -817,6 +817,9 @@ get_vtable_decl (tree type, int complete)
>    decl = build_vtable (type, get_vtable_name (type), vtbl_type_node);
>    CLASSTYPE_VTABLES (type) = decl;
>  
> +  /* Make the vtable visible to build_type_inheritance_graph.  */
> +  varpool_node::get_create (decl);
> +
>    if (complete)
>      {
>        DECL_EXTERNAL (decl) = 1;
> @@ -6408,7 +6411,7 @@ finish_struct_1 (tree t)
>  	 in every translation unit where the class definition appears.  If
>  	 we're devirtualizing, we can look into the vtable even if we
>  	 aren't emitting it.  */
> -      if (CLASSTYPE_KEY_METHOD (t) == NULL_TREE || flag_use_all_virtuals)
> +      if (CLASSTYPE_KEY_METHOD (t) == NULL_TREE)
>  	keyed_classes = tree_cons (NULL_TREE, t, keyed_classes);
>      }
>  
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 0c0d804..c9f248a 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -109,6 +109,7 @@ c-common.h, not after.
>        BIND_EXPR_BODY_BLOCK (in BIND_EXPR)
>        DECL_NON_TRIVIALLY_INITIALIZED_P (in VAR_DECL)
>        CALL_EXPR_LIST_INIT_P (in CALL_EXPR, AGGR_INIT_EXPR)
> +      FNDECL_CALLED_VIRTUALLY (in FUNCTION_DECL)
>     4: TREE_HAS_CONSTRUCTOR (in INDIRECT_REF, SAVE_EXPR, CONSTRUCTOR,
>  	  or FIELD_DECL).
>        IDENTIFIER_TYPENAME_P (in IDENTIFIER_NODE)
> @@ -3222,6 +3223,11 @@ more_aggr_init_expr_args_p (const aggr_init_expr_arg_iterator *iter)
>  #define FNDECL_USED_AUTO(NODE) \
>    TREE_LANG_FLAG_2 (FUNCTION_DECL_CHECK (NODE))
>  
> +/* True if NODE was called through the vtable; used to avoid duplicates in
> +   fns_called_virtually.  */
> +#define FNDECL_CALLED_VIRTUALLY(NODE) \
> +  TREE_LANG_FLAG_3 (FUNCTION_DECL_CHECK (NODE))
> +
>  /* Nonzero if NODE is a DECL which we know about but which has not
>     been explicitly declared, such as a built-in function or a friend
>     declared inside a class.  In the latter case DECL_HIDDEN_FRIEND_P
> @@ -5381,6 +5387,7 @@ extern tree get_tls_wrapper_fn			(tree);
>  extern void mark_needed				(tree);
>  extern bool decl_needed_p			(tree);
>  extern void note_vague_linkage_fn		(tree);
> +extern void note_fn_called_virtually		(tree);
>  extern tree build_artificial_parm		(tree, tree);
>  extern bool possibly_inlined_p			(tree);
>  extern int parm_index                           (tree);
> diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
> index 8fa3145..a448103 100644
> --- a/gcc/cp/decl2.c
> +++ b/gcc/cp/decl2.c
> @@ -47,6 +47,8 @@ along with GCC; see the file COPYING3.  If not see
>  #include "c-family/c-common.h"
>  #include "c-family/c-objc.h"
>  #include "cgraph.h"
> +/* For build_type_inheritance_graph and possible_polymorphic_call_targets.  */
> +#include "ipa-utils.h"
>  #include "tree-inline.h"
>  #include "c-family/c-pragma.h"
>  #include "dumpfile.h"
> @@ -103,6 +105,10 @@ static GTY(()) vec<tree, va_gc> *deferred_fns;
>     sure are defined.  */
>  static GTY(()) vec<tree, va_gc> *no_linkage_decls;
>  
> +/* A list of functions called through the vtable, so we can mark their
> +   overriders as used.  */
> +static GTY(()) vec<tree, va_gc> *fns_called_virtually;
> +
>  /* Nonzero if we're done parsing and into end-of-file activities.  */
>  
>  int at_eof;
> @@ -791,6 +797,19 @@ note_vague_linkage_fn (tree decl)
>    vec_safe_push (deferred_fns, decl);
>  }
>  
> +/* DECL is a function being called through the vtable.  Remember it so that
> +   at the end of the translation unit we can mark as used any functions
> +   that override it, for devirtualization.  */
> +
> +void
> +note_fn_called_virtually (tree decl)
> +{
> +  if (FNDECL_CALLED_VIRTUALLY (decl))
> +    return;
> +  FNDECL_CALLED_VIRTUALLY (decl) = true;
> +  vec_safe_push (fns_called_virtually, decl);
> +}
> +
>  /* We have just processed the DECL, which is a static data member.
>     The other parameters are as for cp_finish_decl.  */
>  
> @@ -4281,6 +4300,39 @@ dump_tu (void)
>      }
>  }
>  
> +/* Now that we've seen all the types in the translation unit, go back over
> +   the list of functions called through the vtable and mark any overriders
> +   as used so they're available for devirtualization.  */
> +
> +static bool
> +mark_virtual_overrides (void)
> +{
> +  if (!fns_called_virtually)
> +    return false;
> +
> +  build_type_inheritance_graph ();
> +
> +  bool reconsider = false;
> +  size_t i; tree fn;
> +  FOR_EACH_VEC_SAFE_ELT (fns_called_virtually, i, fn)
> +    {
> +      vec<cgraph_node*> targets
> +	= possible_polymorphic_call_targets (fn);
> +
> +      size_t j; cgraph_node *n;
> +      FOR_EACH_VEC_ELT (targets, j, n)
> +	{
> +	  if (!DECL_ODR_USED (n->decl))
> +	    reconsider = true;
> +	  mark_used (n->decl);
> +	}
> +    }
> +
> +  release_tree_vector (fns_called_virtually);
> +  fns_called_virtually = NULL;
> +  return reconsider;
> +}
> +
>  /* This routine is called at the end of compilation.
>     Its job is to create all the code needed to initialize and
>     destroy the global aggregates.  We do the destruction
> @@ -4349,8 +4401,6 @@ cp_write_global_declarations (void)
>       instantiated, etc., etc.  */
>  
>    emit_support_tinfos ();
> -  int errs = errorcount + sorrycount;
> -  bool explained_devirt = false;
>  
>    do
>      {
> @@ -4396,6 +4446,9 @@ cp_write_global_declarations (void)
>  	    }
>  	}
>  
> +      if (mark_virtual_overrides ())
> +	reconsider = true;
> +
>        /* Write out needed type info variables.  We have to be careful
>  	 looping through unemitted decls, because emit_tinfo_decl may
>  	 cause other variables to be needed. New elements will be
> @@ -4583,27 +4636,6 @@ cp_write_global_declarations (void)
>  					 pending_statics->length ()))
>  	reconsider = true;
>  
> -      if (flag_use_all_virtuals)
> -	{
> -	  if (!reconsider && !mark_all_virtuals)
> -	    {
> -	      mark_all_virtuals = true;
> -	      reconsider = true;
> -	      errs = errorcount + sorrycount;
> -	    }
> -	  else if (mark_all_virtuals
> -		   && !explained_devirt
> -		   && (errorcount + sorrycount > errs))
> -	    {
> -	      inform (global_dc->last_location, "this error is seen due to "
> -		      "instantiation of all virtual functions, which the C++ "
> -		      "standard says are always considered used; this is done "
> -		      "to support devirtualization optimizations, but can be "
> -		      "disabled with -fno-use-all-virtuals");
> -	      explained_devirt = true;
> -	    }
> -	}
> -
>        retries++;
>      }
>    while (reconsider);
> diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
> index 7c4151a..2bb6461 100644
> --- a/gcc/ipa-devirt.c
> +++ b/gcc/ipa-devirt.c
> @@ -2592,6 +2592,17 @@ possible_polymorphic_call_targets (tree otr_type,
>    return nodes;
>  }
>  
> +/* Return vector containing possible targets of polymorphic call to
> +   function FN.  */
> +
> +vec <cgraph_node *>
> +possible_polymorphic_call_targets (tree fn)
> +{
> +  return possible_polymorphic_call_targets
> +    (DECL_CONTEXT (fn), tree_to_uhwi (DECL_VINDEX (fn)),
> +     ipa_dummy_polymorphic_call_context);
> +}
> +
>  /* Dump all possible targets of a polymorphic call.  */
>  
>  void
> diff --git a/gcc/ipa-utils.h b/gcc/ipa-utils.h
> index bb2e0d5..1023794 100644
> --- a/gcc/ipa-utils.h
> +++ b/gcc/ipa-utils.h
> @@ -78,6 +78,7 @@ possible_polymorphic_call_targets (tree, HOST_WIDE_INT,
>  				   bool *final = NULL,
>  				   void **cache_token = NULL,
>  				   int *nonconstruction_targets = NULL);
> +vec <cgraph_node *> possible_polymorphic_call_targets (tree fn);
>  odr_type get_odr_type (tree, bool insert = false);
>  void dump_possible_polymorphic_call_targets (FILE *, tree, HOST_WIDE_INT,
>  					     const ipa_polymorphic_call_context &);