This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Make cdtor names stable for LTO (PR lto/91307).


> On Fri, Aug 16, 2019 at 10:53 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Fri, Aug 16, 2019 at 10:47 AM Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > >
> > > On Thu, Aug 15, 2019 at 4:25 PM Jan Hubicka <hubicka@ucw.cz> wrote:
> > > >
> > > > > On Thu, Aug 1, 2019 at 3:10 PM Martin Liška <mliska@suse.cz> wrote:
> > > > > >
> > > > > > Hi.
> > > > > >
> > > > > > In LTO WPA mode we don't have to append temp file name
> > > > > > to the global cdtor function names.
> > > > >
> > > > > Is that true?  You can link with -r -flinker-output=rel and use
> > > > > multiple WPA phases whose results you then finally link.
> > > > >
> > > > > So I don't think it's that easy.  You might be able to look at
> > > > > all_translation_units, pick the one with a sensible name
> > > > > (hmm, not sure if we actually have a name there) and the lowest
> > > > > UID and use that?  Thus, make the set of involved source files
> > > > > known and pick one.  Ah,
> > > > >
> > > > > struct GTY(()) tree_translation_unit_decl {
> > > > >   struct tree_decl_common common;
> > > > >   /* Source language of this translation unit.  Used for DWARF output.  */
> > > > >   const char * GTY((skip(""))) language;
> > > > >   /* TODO: Non-optimization used to build this translation unit.  */
> > > > >   /* TODO: Root of a partial DWARF tree for global types and decls.  */
> > > > > };
> > > > >
> > > > > so you might be able to get at a filename via the decls location,
> > > > > I'm not sure.  Do we have any LTO records per "input file" where we
> > > > > can stream main_input_filename to?
> > > >
> > > > This is all bit sloppy.  If you incrmentally link into .o file and then
> > > > use LTO again to add more code, you will very likely run into conflict
> > > > with .lto_priv clones as well. Especially now when we made them more
> > > > stable.
> > > >
> > > > I wondered if Linker should not provide us also with list of symbols
> > > > that are used in the unit, so we can safely produce more local ones?
> > >
> > > OTOH those are all local symbols so the only effect is that the
> > > user may see duplicates when debugging.
> > >
> > > So I wonder why we even try to invent these fancy names for
> > > targets where we have targetm.have_ctors_dtors and thus those
> > > end up being local...  My theory is that either gdb has some
> > > fancy way to break on global initializers from file X (doesn't work
> > > with LTO merging anyways) or it's just the users will  be able to
> > > pick up the correct one with tab-completion more easily!?
> > >
> > > So I believe choosing any name is fine for targetm_have_ctors_dtors
> > > like with the following change, local to the IPA logic:
> > >
> > > Index: gcc/ipa.c
> > > ===================================================================
> > > --- gcc/ipa.c   (revision 274536)
> > > +++ gcc/ipa.c   (working copy)
> > > @@ -836,13 +836,15 @@ cgraph_build_static_cdtor_1 (char which,
> > >    /* The priority is encoded in the constructor or destructor name.
> > >       collect2 will sort the names and arrange that they are called at
> > >       program startup.  */
> > > -  if (final)
> > > -    sprintf (which_buf, "%c_%.5d_%d", which, priority, counter++);
> > > +  if (!targetm.have_ctors_dtors && final)
> > > +    {
> > > +      sprintf (which_buf, "%c_%.5d_%d", which, priority, counter++);
> > > +      name = get_file_function_name (which_buf);
> > > +    }
> > >    else
> > > -  /* Proudce sane name but one not recognizable by collect2, just for the
> > > -     case we fail to inline the function.  */
> > > +    /* Proudce sane name but one not recognizable by collect2, just for the
> > > +       case we fail to inline the function.  */
> > >      sprintf (which_buf, "sub_%c_%.5d_%d", which, priority, counter++);
> > > -  name = get_file_function_name (which_buf);
> > >
> > >    decl = build_decl (input_location, FUNCTION_DECL, name,
> > >                      build_function_type_list (void_type_node, NULL_TREE));
> > >
> > > Is that change OK with you?  If so I'll test & commit it.
> >
> > Better the following which actually works ;)
> 
> Bootstrapped/tested on x86_64-unknown-linux-gnu, OK?

OK, Thanks!
Honza
> 
> Thanks,
> Richard.
> 
> > Richard.
> >
> > 2019-08-16  Richard Biener  <rguenther@suse.de>
> >
> >         PR lto/91307
> >         * ipa.c (cgraph_build_static_cdtor_1): Use names not recognizable
> >         by collect2 when targetm.have_ctors_dtors which avoids dragging
> >         in temporary filenames from LTO input objects.
> >
> > Index: gcc/ipa.c
> > ===================================================================
> > --- gcc/ipa.c   (revision 274536)
> > +++ gcc/ipa.c   (working copy)
> > @@ -836,13 +836,18 @@ cgraph_build_static_cdtor_1 (char which,
> >    /* The priority is encoded in the constructor or destructor name.
> >       collect2 will sort the names and arrange that they are called at
> >       program startup.  */
> > -  if (final)
> > -    sprintf (which_buf, "%c_%.5d_%d", which, priority, counter++);
> > +  if (!targetm.have_ctors_dtors && final)
> > +    {
> > +      sprintf (which_buf, "%c_%.5d_%d", which, priority, counter++);
> > +      name = get_file_function_name (which_buf);
> > +    }
> >    else
> > -  /* Proudce sane name but one not recognizable by collect2, just for the
> > -     case we fail to inline the function.  */
> > -    sprintf (which_buf, "sub_%c_%.5d_%d", which, priority, counter++);
> > -  name = get_file_function_name (which_buf);
> > +    {
> > +      /* Proudce sane name but one not recognizable by collect2, just for the
> > +        case we fail to inline the function.  */
> > +      sprintf (which_buf, "_sub_%c_%.5d_%d", which, priority, counter++);
> > +      name = get_identifier (which_buf);
> > +    }
> >
> >    decl = build_decl (input_location, FUNCTION_DECL, name,
> >                      build_function_type_list (void_type_node, NULL_TREE));


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]