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 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.

Thanks,
Richard.

> Honza
> >
> > > It helps to have a reproducible
> > > builds with LTO mode.
> > >
> > > Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> > >
> > > Ready to be installed?
> > > Thanks,
> > > Martin
> > >
> > > gcc/ChangeLog:
> > >
> > > 2019-08-01  Martin Liska  <mliska@suse.cz>
> > >
> > >         PR lto/91307
> > >         * tree.c (get_file_function_name): Use "wpa" when
> > >         we are in WPA LTO mode.
> > > ---
> > >  gcc/tree.c | 17 +++++++++++------
> > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > >
> > >


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