public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Make cdtor names stable for LTO (PR lto/91307).
@ 2019-08-01 13:10 Martin Liška
  2019-08-01 14:21 ` Richard Biener
  0 siblings, 1 reply; 8+ messages in thread
From: Martin Liška @ 2019-08-01 13:10 UTC (permalink / raw)
  To: gcc-patches; +Cc: Richard Biener

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

Hi.

In LTO WPA mode we don't have to append temp file name
to the global cdtor function names. 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(-)



[-- Attachment #2: 0001-Make-cdtor-names-stable-for-LTO-PR-lto-91307.patch --]
[-- Type: text/x-patch, Size: 846 bytes --]

diff --git a/gcc/tree.c b/gcc/tree.c
index 8cf75f22220..56c0fd450f1 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -9817,12 +9817,17 @@ get_file_function_name (const char *type)
 	   || (strncmp (type, "sub_", 4) == 0
 	       && (type[4] == 'I' || type[4] == 'D')))
     {
-      const char *file = main_input_filename;
-      if (! file)
-	file = LOCATION_FILE (input_location);
-      /* Just use the file's basename, because the full pathname
-	 might be quite long.  */
-      p = q = ASTRDUP (lbasename (file));
+      if (flag_wpa)
+	p = q = ASTRDUP ("wpa");
+      else
+	{
+	  const char *file = main_input_filename;
+	  if (! file)
+	    file = LOCATION_FILE (input_location);
+	  /* Just use the file's basename, because the full pathname
+	     might be quite long.  */
+	  p = q = ASTRDUP (lbasename (file));
+	}
     }
   else
     {


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Make cdtor names stable for LTO (PR lto/91307).
  2019-08-01 13:10 [PATCH] Make cdtor names stable for LTO (PR lto/91307) Martin Liška
@ 2019-08-01 14:21 ` Richard Biener
  2019-08-02  6:08   ` Martin Liška
  2019-08-15 14:52   ` Jan Hubicka
  0 siblings, 2 replies; 8+ messages in thread
From: Richard Biener @ 2019-08-01 14:21 UTC (permalink / raw)
  To: Martin Liška, Jan Hubicka; +Cc: GCC Patches

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?

> 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(-)
>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Make cdtor names stable for LTO (PR lto/91307).
  2019-08-01 14:21 ` Richard Biener
@ 2019-08-02  6:08   ` Martin Liška
  2019-08-15 14:52   ` Jan Hubicka
  1 sibling, 0 replies; 8+ messages in thread
From: Martin Liška @ 2019-08-02  6:08 UTC (permalink / raw)
  To: Richard Biener, Jan Hubicka; +Cc: GCC Patches

On 8/1/19 4:21 PM, Richard Biener 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

Ah, I see.

> (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?

Honza, can you help us with this?
Thanks,
Martin

> 
>> 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(-)
>>
>>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Make cdtor names stable for LTO (PR lto/91307).
  2019-08-01 14:21 ` Richard Biener
  2019-08-02  6:08   ` Martin Liška
@ 2019-08-15 14:52   ` Jan Hubicka
  2019-08-16  8:55     ` Richard Biener
  1 sibling, 1 reply; 8+ messages in thread
From: Jan Hubicka @ 2019-08-15 14:52 UTC (permalink / raw)
  To: Richard Biener; +Cc: Martin Liška, GCC Patches

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

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(-)
> >
> >

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Make cdtor names stable for LTO (PR lto/91307).
  2019-08-15 14:52   ` Jan Hubicka
@ 2019-08-16  8:55     ` Richard Biener
  2019-08-16  9:01       ` Richard Biener
  0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2019-08-16  8:55 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Martin Liška, GCC Patches

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(-)
> > >
> > >

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Make cdtor names stable for LTO (PR lto/91307).
  2019-08-16  8:55     ` Richard Biener
@ 2019-08-16  9:01       ` Richard Biener
  2019-08-20 11:36         ` Richard Biener
  0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2019-08-16  9:01 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Martin Liška, GCC Patches

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 ;)

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));

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Make cdtor names stable for LTO (PR lto/91307).
  2019-08-16  9:01       ` Richard Biener
@ 2019-08-20 11:36         ` Richard Biener
  2019-08-20 12:39           ` Jan Hubicka
  0 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2019-08-20 11:36 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Martin Liška, GCC Patches

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));

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] Make cdtor names stable for LTO (PR lto/91307).
  2019-08-20 11:36         ` Richard Biener
@ 2019-08-20 12:39           ` Jan Hubicka
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Hubicka @ 2019-08-20 12:39 UTC (permalink / raw)
  To: Richard Biener; +Cc: Martin Liška, GCC Patches

> 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));

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-08-20 11:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-01 13:10 [PATCH] Make cdtor names stable for LTO (PR lto/91307) Martin Liška
2019-08-01 14:21 ` Richard Biener
2019-08-02  6:08   ` Martin Liška
2019-08-15 14:52   ` Jan Hubicka
2019-08-16  8:55     ` Richard Biener
2019-08-16  9:01       ` Richard Biener
2019-08-20 11:36         ` Richard Biener
2019-08-20 12:39           ` Jan Hubicka

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).