diff mbox

Avoid double mangling at WHOPR

Message ID 20111009180504.GK13389@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka Oct. 9, 2011, 6:05 p.m. UTC
Hi,
whopr currently produce local_static.1234.43124 type symbols. This is because
everything gets mangled at WPA time and then again at ltrans time.  This simply
avoids the second mangling. This save some space & makes WHOPR/non_WHOPR symbol
tables comparable more directly.

Bootstrapped/regtested x86_64-linux, also tested with Mozilla LTO, OK?

Honza

	* lto.c (lto_register_var_decl_in_symtab,
	lto_register_function_decl_in_symtab): Do not mangle at ltrans time.
	* lto-lang.c (lto_set_decl_assembler_name): Likewise.

Comments

Richard Biener Oct. 10, 2011, 9:09 a.m. UTC | #1
On Sun, 9 Oct 2011, Jan Hubicka wrote:

> Hi,
> whopr currently produce local_static.1234.43124 type symbols. This is because
> everything gets mangled at WPA time and then again at ltrans time.  This simply
> avoids the second mangling. This save some space & makes WHOPR/non_WHOPR symbol
> tables comparable more directly.
> 
> Bootstrapped/regtested x86_64-linux, also tested with Mozilla LTO, OK?

Hmmm.  I'd really like to defer mangling to LTRANS at one point, as
we will know if there will be conflicts of local symbols at that point
(another point would be the partitioning code).  So this patch goes
backward ... (ideally a first step would move it to symbol resolution
time, another place where we can check for conflicts, and then only
apply it a LTRANs stage).

Richard.

> Honza
> 
> 	* lto.c (lto_register_var_decl_in_symtab,
> 	lto_register_function_decl_in_symtab): Do not mangle at ltrans time.
> 	* lto-lang.c (lto_set_decl_assembler_name): Likewise.
> Index: lto/lto.c
> ===================================================================
> --- lto/lto.c	(revision 179664)
> +++ lto/lto.c	(working copy)
> @@ -604,7 +604,7 @@ lto_register_var_decl_in_symtab (struct
>  
>    /* Variable has file scope, not local. Need to ensure static variables
>       between different files don't clash unexpectedly.  */
> -  if (!TREE_PUBLIC (decl)
> +  if (!TREE_PUBLIC (decl) && !flag_ltrans
>        && !((context = decl_function_context (decl))
>  	   && auto_var_in_fn_p (decl, context)))
>      {
> @@ -646,7 +646,7 @@ lto_register_function_decl_in_symtab (st
>  {
>    /* Need to ensure static entities between different files
>       don't clash unexpectedly.  */
> -  if (!TREE_PUBLIC (decl))
> +  if (!TREE_PUBLIC (decl) && !flag_ltrans)
>      {
>        /* We must not use the DECL_ASSEMBLER_NAME macro here, as it
>  	 may set the assembler name where it was previously empty.  */
> Index: lto/lto-lang.c
> ===================================================================
> --- lto/lto-lang.c	(revision 179664)
> +++ lto/lto-lang.c	(working copy)
> @@ -954,7 +954,7 @@ lto_set_decl_assembler_name (tree decl)
>       TREE_PUBLIC, to avoid conflicts between individual files.  */
>    tree id;
>  
> -  if (TREE_PUBLIC (decl))
> +  if (TREE_PUBLIC (decl) || flag_ltrans)
>      id = targetm.mangle_decl_assembler_name (decl, DECL_NAME (decl));
>    else
>      {
> 
>
Jan Hubicka Oct. 10, 2011, 12:58 p.m. UTC | #2
> On Sun, 9 Oct 2011, Jan Hubicka wrote:
> 
> > Hi,
> > whopr currently produce local_static.1234.43124 type symbols. This is because
> > everything gets mangled at WPA time and then again at ltrans time.  This simply
> > avoids the second mangling. This save some space & makes WHOPR/non_WHOPR symbol
> > tables comparable more directly.
> > 
> > Bootstrapped/regtested x86_64-linux, also tested with Mozilla LTO, OK?
> 
> Hmmm.  I'd really like to defer mangling to LTRANS at one point, as
> we will know if there will be conflicts of local symbols at that point
> (another point would be the partitioning code).  So this patch goes
> backward ... (ideally a first step would move it to symbol resolution
> time, another place where we can check for conflicts, and then only
> apply it a LTRANs stage).

Actually it seems to me that mangling at WPA makes more sense - you don't get
symbol name sensitive on partitioning decisions so things go a bit more
consistently. Partitioning depends on global properties of program so any code
that depends on particular partitioning decisions will have tendency to
randomly break and unbreak.

We can not really make any promises about our ability to not mangle particular
symbol (for use in asm code or whatever):  we need to mangle on the occasion of
conflict with another static symbol but also when we decide to promote it
hidden. We have no information about another hidden symbol in the non-LTO
world.  Either linker plugin API needs to be extended by providing us with list
of forbidden names or ability to have "hidden in LTO world" visibility or we
probably need to start using random seeds on all promoted symbols.
(in fact I already do sort of mangling to avoid conflict on comdats that has been
brought local and then again promoted global, I just did not noticed it is a general
problem back then when I first saw the linker complaining).

Honza
Richard Biener Oct. 10, 2011, 1:08 p.m. UTC | #3
On Mon, 10 Oct 2011, Jan Hubicka wrote:

> > On Sun, 9 Oct 2011, Jan Hubicka wrote:
> > 
> > > Hi,
> > > whopr currently produce local_static.1234.43124 type symbols. This is because
> > > everything gets mangled at WPA time and then again at ltrans time.  This simply
> > > avoids the second mangling. This save some space & makes WHOPR/non_WHOPR symbol
> > > tables comparable more directly.
> > > 
> > > Bootstrapped/regtested x86_64-linux, also tested with Mozilla LTO, OK?
> > 
> > Hmmm.  I'd really like to defer mangling to LTRANS at one point, as
> > we will know if there will be conflicts of local symbols at that point
> > (another point would be the partitioning code).  So this patch goes
> > backward ... (ideally a first step would move it to symbol resolution
> > time, another place where we can check for conflicts, and then only
> > apply it a LTRANs stage).
> 
> Actually it seems to me that mangling at WPA makes more sense - you don't get
> symbol name sensitive on partitioning decisions so things go a bit more
> consistently. Partitioning depends on global properties of program so any code
> that depends on particular partitioning decisions will have tendency to
> randomly break and unbreak.
> 
> We can not really make any promises about our ability to not mangle particular
> symbol (for use in asm code or whatever):  we need to mangle on the occasion of
> conflict with another static symbol but also when we decide to promote it
> hidden. We have no information about another hidden symbol in the non-LTO
> world.  Either linker plugin API needs to be extended by providing us with list
> of forbidden names or ability to have "hidden in LTO world" visibility or we
> probably need to start using random seeds on all promoted symbols.
> (in fact I already do sort of mangling to avoid conflict on comdats that has been
> brought local and then again promoted global, I just did not noticed it is a general
> problem back then when I first saw the linker complaining).

Ok, I see why it makes sense on WPA time.  But then why not mangle
during partitioning?  I think it still makes sense to avoid mangling local
decls, if not for debugging experience.

We do mangle late when we bring symbols local anyway, no?  I also
seem to remember we mangle at LTO time, too ...

Richard.
Jan Hubicka Oct. 10, 2011, 1:22 p.m. UTC | #4
> > Actually it seems to me that mangling at WPA makes more sense - you don't get
> > symbol name sensitive on partitioning decisions so things go a bit more
> > consistently. Partitioning depends on global properties of program so any code
> > that depends on particular partitioning decisions will have tendency to
> > randomly break and unbreak.
> > 
> > We can not really make any promises about our ability to not mangle particular
> > symbol (for use in asm code or whatever):  we need to mangle on the occasion of
> > conflict with another static symbol but also when we decide to promote it
> > hidden. We have no information about another hidden symbol in the non-LTO
> > world.  Either linker plugin API needs to be extended by providing us with list
> > of forbidden names or ability to have "hidden in LTO world" visibility or we
> > probably need to start using random seeds on all promoted symbols.
> > (in fact I already do sort of mangling to avoid conflict on comdats that has been
> > brought local and then again promoted global, I just did not noticed it is a general
> > problem back then when I first saw the linker complaining).
> 
> Ok, I see why it makes sense on WPA time.  But then why not mangle
> during partitioning?  I think it still makes sense to avoid mangling local
> decls, if not for debugging experience.

Yeah, we could do that. Debugging experience is quite good reason (though it will
also make bogus asm statements magically work and break on random basis. In a way
just breaking them seems more sensible behaviour to me ;) ).
> 
> We do mangle late when we bring symbols local anyway, no?  I also
> seem to remember we mangle at LTO time, too ...

Hmm, I think we mangle at stream in since we do make hashtables based on symbol names
that are supposed to be unique, but perhaps I am wrong.
LTO time you mean at compilation time when streaming out? I am not aware of that.

Honza
> 
> Richard.
Richard Biener Oct. 10, 2011, 1:30 p.m. UTC | #5
On Mon, 10 Oct 2011, Jan Hubicka wrote:

> > > Actually it seems to me that mangling at WPA makes more sense - you don't get
> > > symbol name sensitive on partitioning decisions so things go a bit more
> > > consistently. Partitioning depends on global properties of program so any code
> > > that depends on particular partitioning decisions will have tendency to
> > > randomly break and unbreak.
> > > 
> > > We can not really make any promises about our ability to not mangle particular
> > > symbol (for use in asm code or whatever):  we need to mangle on the occasion of
> > > conflict with another static symbol but also when we decide to promote it
> > > hidden. We have no information about another hidden symbol in the non-LTO
> > > world.  Either linker plugin API needs to be extended by providing us with list
> > > of forbidden names or ability to have "hidden in LTO world" visibility or we
> > > probably need to start using random seeds on all promoted symbols.
> > > (in fact I already do sort of mangling to avoid conflict on comdats that has been
> > > brought local and then again promoted global, I just did not noticed it is a general
> > > problem back then when I first saw the linker complaining).
> > 
> > Ok, I see why it makes sense on WPA time.  But then why not mangle
> > during partitioning?  I think it still makes sense to avoid mangling local
> > decls, if not for debugging experience.
> 
> Yeah, we could do that. Debugging experience is quite good reason (though it will
> also make bogus asm statements magically work and break on random basis. In a way
> just breaking them seems more sensible behaviour to me ;) ).

;)

> > We do mangle late when we bring symbols local anyway, no?  I also
> > seem to remember we mangle at LTO time, too ...
> 
> Hmm, I think we mangle at stream in since we do make hashtables based on symbol names
> that are supposed to be unique, but perhaps I am wrong.

I think we do not hash local symbols, so that shouldn't be an issue.

> LTO time you mean at compilation time when streaming out? I am not aware of that.

Maybe that changed then or I misremember.

Can you try the "obvious" and simply mangle all local statics at
partitioning time?  (leaving the non-conflict case for a further
improvement)

Richard.
Jan Hubicka Oct. 10, 2011, 1:38 p.m. UTC | #6
> Can you try the "obvious" and simply mangle all local statics at
> partitioning time?  (leaving the non-conflict case for a further
> improvement)

Hmm, it needs to be done with non-WHOPR too, but sure I can give it a try.
I should however finally push out the weakref bits.  (here one can produce
weakref of static that is bit nonsential but possible and that breaks at
renaming time).  Will try to look into that today.

Will lto-symtab be happy about this?

Honza
> 
> Richard.
diff mbox

Patch

Index: lto/lto.c
===================================================================
--- lto/lto.c	(revision 179664)
+++ lto/lto.c	(working copy)
@@ -604,7 +604,7 @@  lto_register_var_decl_in_symtab (struct
 
   /* Variable has file scope, not local. Need to ensure static variables
      between different files don't clash unexpectedly.  */
-  if (!TREE_PUBLIC (decl)
+  if (!TREE_PUBLIC (decl) && !flag_ltrans
       && !((context = decl_function_context (decl))
 	   && auto_var_in_fn_p (decl, context)))
     {
@@ -646,7 +646,7 @@  lto_register_function_decl_in_symtab (st
 {
   /* Need to ensure static entities between different files
      don't clash unexpectedly.  */
-  if (!TREE_PUBLIC (decl))
+  if (!TREE_PUBLIC (decl) && !flag_ltrans)
     {
       /* We must not use the DECL_ASSEMBLER_NAME macro here, as it
 	 may set the assembler name where it was previously empty.  */
Index: lto/lto-lang.c
===================================================================
--- lto/lto-lang.c	(revision 179664)
+++ lto/lto-lang.c	(working copy)
@@ -954,7 +954,7 @@  lto_set_decl_assembler_name (tree decl)
      TREE_PUBLIC, to avoid conflicts between individual files.  */
   tree id;
 
-  if (TREE_PUBLIC (decl))
+  if (TREE_PUBLIC (decl) || flag_ltrans)
     id = targetm.mangle_decl_assembler_name (decl, DECL_NAME (decl));
   else
     {