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?

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]