Re: [PATCH] pr61324 pr 63649 - fix crash in ipa_comdats

[Trevor Saunders <tsaunders at mozilla dot com>]

On Tue, Nov 25, 2014 at 09:59:46PM +0100, Jan Hubicka wrote:
> > From: Trevor Saunders <tsaunders@mozilla.com>
> > 
> > Hi,
> > 
> > the interesting symbol in the test case for pr61324 is __GLOBAL__sub_I_s.  It
> > refers to nothing, and is called by nothing, however it is kept (I believe
> > because of -fkeep-inline-functions).  That means ipa_comdats never tries to put
> 
> Aha, that explans why it is around.

 Ok, so pr 63649 is more interesting.  We start out with static ctors for
 a and b.  ipa-pure-const decides the static ctor for b is const, and so
 calls cgraph_node::set_const_flag ().  That function sets
 DECL_STATIC_CONSTRUCTOR on the static ctor for b to 0, but doesn't
 actually remove the function (I guess it relies on something else doing
 that).  Then ipa-comdats comes along and the story continues more or
 less the same as pr 61324.

> > it in a comdat, and so it never ends up in the hash table.  It seems like the
> > simplest solution is to just check if symbol is not in the map before trying to
> > get the comdat it should go in, but another approach might be to keep separate
> > hash maps for comdat functions and functions that can't be in any comdat, and
> > then iterate over only the functions that belong in a comdat.
> 
> Well, -fkeep-inline-functions promise you that you can call any inline function from debugger.
> I suppose in this case you also want to be able to call static functions. Comdat pass may
> bundle the function into comdat that is later optimized away by linker, so I would say we
> just want to disable the whole comdat pass when -fkeep-inline-functions is used?

So we might still want to do that, but I think fixing the above issue
may resolve this one too?

Trev

> 
> Patch for that is preapproved.
> Honza
> > 
> > boottstrapped + regtested x86_64-unknown-linux-gnu, ok?
> > 
> > Trev
> > 
> > gcc/
> > 
> > 	* ipa-comdats.c (ipa_commdats): check if map contains symbol before
> > 	trying to put symbol in a comdat.
> > 
> > diff --git a/gcc/ipa-comdats.c b/gcc/ipa-comdats.c
> > index af2aef8..8695a7e 100644
> > --- a/gcc/ipa-comdats.c
> > +++ b/gcc/ipa-comdats.c
> > @@ -327,18 +327,18 @@ ipa_comdats (void)
> >  	  && !symbol->alias
> >  	  && symbol->real_symbol_p ())
> >  	{
> > -	  tree group = *map.get (symbol);
> > +	  tree *group = map.get (symbol);
> >  
> > -	  if (group == error_mark_node)
> > +	  if (!group || *group == error_mark_node)
> >  	    continue;
> >  	  if (dump_file)
> >  	    {
> >  	      fprintf (dump_file, "Localizing symbol\n");
> >  	      symbol->dump (dump_file);
> > -	      fprintf (dump_file, "To group: %s\n", IDENTIFIER_POINTER (group));
> > +	      fprintf (dump_file, "To group: %s\n", IDENTIFIER_POINTER (*group));
> >  	    }
> >  	  symbol->call_for_symbol_and_aliases (set_comdat_group,
> > -					     *comdat_head_map.get (group),
> > +					     *comdat_head_map.get (*group),
> >  					     true);
> >  	}
> >      }
> > diff --git a/gcc/testsuite/g++.dg/pr61324.C b/gcc/testsuite/g++.dg/pr61324.C
> > new file mode 100644
> > index 0000000..6102574
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/pr61324.C
> > @@ -0,0 +1,13 @@
> > +// { dg-do compile }
> > +// { dg-options "-O -fkeep-inline-functions -fno-use-cxa-atexit" }
> > +void foo ();
> > +
> > +struct S
> > +{
> > +  ~S ()
> > +  {
> > +    foo ();
> > +  }
> > +};
> > +
> > +S s;
> > -- 
> > 2.1.3