[Bug lto/54095] New: Unnecessary static variable renaming

[Bug lto/54095] New: Unnecessary static variable renaming

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

             Bug #: 54095
           Summary: Unnecessary static variable renaming
    Classification: Unclassified
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: enhancement
          Priority: P3
         Component: lto
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: rth@gcc.gnu.org


For example:

    # cat hello.c
    static int foo = 5;
    extern int bar(int *);
    main() {
      return bar(&foo);
    }
    # cc -flto -O hello.c -r -o hello.o -nostdlib
    # nm hello.o
                     U bar
    0000000000000000 d foo.2353.2353
    0000000000000000 T main

There is no other symbol foo within the partition, so there's no need to
rename the static symbol within the partition.

The linux kernel modversion and CRC checking is done using static symbols
in the kernel for each exported symbol.  Given that the exported symbols
must of course be unique, the kernel programmers know that the static symbols
holding the data that controls the exports must also be unique.  It would
be very nice to not have to adjust the modversion logic to deal with the
renames that lto currently induces.

[Bug lto/54095] Unnecessary static variable renaming

[andi-gcc at firstfloor dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

Andi Kleen <andi-gcc at firstfloor dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andi-gcc at firstfloor dot
                   |                            |org

--- Comment #1 from Andi Kleen <andi-gcc at firstfloor dot org> 2012-07-25 17:44:05 UTC ---
I usually use the workaround of making the symbol non static and visible
But there's still some other problem with modversions/crc that breaks even with
that.

[Bug lto/54095] Unnecessary static variable renaming

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-07-26
                 CC|                            |hubicka at gcc dot gnu.org
         AssignedTo|unassigned at gcc dot       |rguenth at gcc dot gnu.org
                   |gnu.org                     |
     Ever Confirmed|0                           |1

--- Comment #2 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-07-26 08:46:04 UTC ---
Confirmed.  My overall plan was to delay assigning an assembler name (or just
delay the mangling) until LTRANS phase where we will know if partitioning
resulted in a conflict of two static decls from different TUs.

It's been on my todo list for some time ... ;)

Btw, this re-naming also confuses debugging - you can no longer do

(gdb) p foo

but have to do

(gdb) p foo.2353.2353

ugh.

Note that you'll not be able to rely on statics not being renamed
apart from when you use 1to1 partitioning (well, hopefully - I think
we still pull in inlines which might pull in conflicts).

[Bug lto/54095] Unnecessary static variable renaming

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

--- Comment #3 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-07-26 14:48:36 UTC ---
Created attachment 27879
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27879
incomplete prototype patch

Something like this.  But I'll rather wait for the symtab reorg to reach
cgraph/varpool_node_set (and for the partitioning rewrite to go in).

The patch simply, at current lto_promote_cross_file_statics time (which
can still alter the sets), does the mangling of statics.  Which means it
works at WPA time and thus will not work with -flto-partition=none.  LTRANS
file generation is confused because of conflicting section names if we try
to postpone it to LTRANS phase.  For -flto-partition=none we need a "simpler"
hack elsewhere.

[Bug lto/54095] Unnecessary static variable renaming

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |lto
             Blocks|                            |43038

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-07-26 14:53:58 UTC ---
PR43038 is related.  We should be able to impose more constraints to the
partitioning algorithm.

[Bug lto/54095] Unnecessary static variable renaming

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27879|0                           |1
        is obsolete|                            |

--- Comment #5 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-09-18 14:20:45 UTC ---
Created attachment 28215
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28215
patch

More complete patch, still has issues.  We rely on the name mapping to find
LTO data but that for some reason is confused when we try to rename late.
Which you can see from LTO testsuite fails caused by this patch.

At least with the partitioning reorg the patch doesn't look as bad as before ;)

[Bug lto/54095] Unnecessary static variable renaming

[hubicka at ucw dot cz]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

--- Comment #6 from Jan Hubicka <hubicka at ucw dot cz> 2012-09-19 08:23:45 UTC ---
> --- Comment #5 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-09-18 14:20:45 UTC ---
> Created attachment 28215
>   --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28215
> patch
> 
> More complete patch, still has issues.  We rely on the name mapping to find
> LTO data but that for some reason is confused when we try to rename late.
> Which you can see from LTO testsuite fails caused by this patch.
> 
> At least with the partitioning reorg the patch doesn't look as bad as before ;)

Hehe, the partioning reorg was intended to give us some room to insert new
uglyness
into the area ;)

I suppose that is because you rename late and the renaming code do not work
with presence
of inline clones.  I will debug that.

Thanks,
Honza

[Bug lto/54095] Unnecessary static variable renaming

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28215|0                           |1
        is obsolete|                            |

--- Comment #7 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-09-25 09:04:20 UTC ---
Created attachment 28264
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28264
new patch

Next try using symtab_real_symbol_p.  Works better, but I now fail to
rename multiple uses of

static inline void
unread_char (int ch)
{
  if (ch == '\n')
    read_md_lineno--;
  ungetc (ch, read_md_file);
}

from read-md.h during bootstrap (thus, with C++).  symtab_real_symbol_p returns
false for

_ZL11unread_chari/2124 (unread_char) @0x7ffff5feb9c0
  Type: function
  Visibility:
  next sharing asm name: 2108
  previous sharing asm name: 2125
  References: read_md_lineno/1445 (read)read_md_lineno/1445
(write)read_md_file/1443 (read)
  Referring: 
  Function unread_char/2124 is inline copy in read_rtx_code/460
  Clone of _ZL11unread_chari/428
  Availability: local
  Function flags: analyzed local finalized
  Called by: _ZL17read_rtx_variadicP7rtx_def/462 (0.01 per call) (inlined) 
  Calls: ungetc/515 (0.01 per call) 

but we still stream its IL (and thus I get section conflicts)?

We also have non-inlined copies:

_ZL11unread_chari/1437 (unread_char) @0x7ffff600b3a8
  Type: function
  Visibility: prevailing_def_ironly
  previous sharing asm name: 428
  References: read_md_lineno/1445 (read)read_md_lineno/1445
(write)read_md_file/1443 (read)
  Referring: 
  Read from file: build/read-md.o
  Availability: local
  Function flags: analyzed local finalized
  Called by: _Z16read_skip_spacesv/1471 _Z24fatal_with_file_and_linePKcz/1469
(0.39 per call) 
  Calls: ungetc/515 (1.00 per call) 

Not sure how I can make a reproducer, the following works

static inline int foo (int i) { asm("#"); asm("#"); asm ("#"); asm
("#");asm("#"); asm("#"); asm ("#"); asm ("#");asm("#"); asm("#"); asm ("#");
asm ("#");asm("#"); asm("#"); asm ("#"); asm ("#");asm("#"); asm("#"); asm
("#"); asm ("#" : : "g" (&i));     asm("#"); asm("#"); asm ("#"); asm ("#");
return i/10; }

int __attribute__((flatten)) bar (int j)
{
  return foo (j);
}
int main()
{
  return foo (0) + bar (0) + foo (0);
}

[Bug lto/54095] Unnecessary static variable renaming

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

--- Comment #8 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-09-25 09:38:44 UTC ---
Including the boundary as with

          /* If there is a symbol in the set that shares the same
             asm name than NODE, rename NODE.  */
          if (!TREE_PUBLIC (decl)
              && symtab_real_symbol_p (node))
            for (s = symtab_node_for_asm (DECL_ASSEMBLER_NAME (decl));
                 s; s = s->symbol.next_sharing_asm_name)
              if (s != node
                  && symtab_real_symbol_p (s)
                  && (lto_symtab_encoder_in_partition_p (encoder, s)
                      || (symtab_function_p (s)
                          && lto_symtab_encoder_encode_body_p (encoder, cgraph
(s)))))

doesn't fix it (but is probably needed?).  Looking in how lto-streamer-out.c
iterates over bodies to stream I notice it doesn't use the iterator (cosmetic)
and it doesn't check symtab_real_symbol_p but lto_symtab_encoder_encode_body_p
plus !node->alias && !node->thunk.thunk_p.  So I should probably use that
check instead of what I used above (well, for functions at least).  Trying
an assert in lto-streamer-out.c that we only stream symtab_real_symbol_p's.

[Bug lto/54095] Unnecessary static variable renaming

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

--- Comment #9 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-09-25 09:53:36 UTC ---
Indeed:

Index: gcc/lto-streamer-out.c
===================================================================
--- gcc/lto-streamer-out.c      (revision 191694)
+++ gcc/lto-streamer-out.c      (working copy)
@@ -1001,6 +1001,7 @@ lto_output (void)
          gcc_assert (!bitmap_bit_p (output, DECL_UID (node->symbol.decl)));
          bitmap_set_bit (output, DECL_UID (node->symbol.decl));
 #endif
+         gcc_assert (symtab_real_symbol_p ((symtab_node) node));
          decl_state = lto_new_out_decl_state ();
          lto_push_out_decl_state (decl_state);
          if (gimple_has_body_p (node->symbol.decl))

produces

#1  0x0000000000822316 in lto_output ()
    at /space/rguenther/src/svn/trunk/gcc/lto-streamer-out.c:1004
1004              gcc_assert (symtab_real_symbol_p ((symtab_node) node));
(gdb) call debug_cgraph_node (node)
_ZL14get_md_ptr_locPKv/171 (get_md_ptr_loc) @0x7ffff6274888
  Type: function
  Visibility: prevailing_def_ironly
  previous sharing asm name: 781
  References: _ZL8ptr_locs/151 (read)
  Referring: 
  Read from file: build/read-md.o
  Function get_md_ptr_loc/171 is inline copy in fprint_md_ptr_loc/173
  Availability: local
  Function flags: analyzed local finalized
  Called by: _Z17fprint_md_ptr_locP8_IO_FILEPKv/173 (1.00 per call) (inlined) 
  Calls: htab_find/544 (1.00 per call) 

Trying

          /* If there is a symbol in the set that shares the same
             asm name than NODE, rename NODE.  */
          if (!TREE_PUBLIC (decl)
              && (!symtab_function_p (node)
                  || lto_symtab_encoder_encode_body_p (encoder, cgraph
(node))))
            for (s = symtab_node_for_asm (DECL_ASSEMBLER_NAME (decl));
                 s; s = s->symbol.next_sharing_asm_name)
              if (s != node
                  && lto_symtab_encoder_lookup (encoder, s) != LCC_NOT_FOUND
                  && (!symtab_function_p (s)
                      || lto_symtab_encoder_encode_body_p (encoder, cgraph
(s))))
                {

[Bug lto/54095] Unnecessary static variable renaming

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28264|0                           |1
        is obsolete|                            |

--- Comment #10 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-09-25 10:13:32 UTC ---
Created attachment 28265
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28265
newer patch

With that we get to

/space/rguenther/src/svn/trunk/gcc/hash-table.c:74:0: error: type of
'prime_tab' does not match original declaration [-Werror]
 struct prime_ent const prime_tab[] = {
 ^
/space/rguenther/src/svn/trunk/gcc/hash-table.h:145:31: note: previously
declared here
 extern struct prime_ent const prime_tab[];
                               ^
lto1: fatal error: errors during merging of translation units

thus it seems better (but to make sure I'll add the !node->alias/thunk.thunk_p
checks as in the new attachment).

[Bug lto/54095] Unnecessary static variable renaming

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

--- Comment #11 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-09-27 12:46:08 UTC ---
Created attachment 28290
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28290
no-op patch

This tries to only delay renaming all statics, not avoid any renaming.  Passes
LTO bootstrap at least.

[Bug lto/54095] Unnecessary static variable renaming

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

--- Comment #12 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-09-28 08:37:56 UTC ---
Ok, I give up ;)  Honza, I think we should go with your patch to re-name
before LTO symtab merging as a first step, we can try improving later ...

[Bug lto/54095] Unnecessary static variable renaming

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|rguenth at gcc dot gnu.org  |hubicka at gcc dot gnu.org

--- Comment #13 from Jan Hubicka <hubicka at gcc dot gnu.org> 2012-10-04 14:28:03 UTC ---
Mine now ;)

[Bug lto/54095] Unnecessary static variable renaming

[andi-gcc at firstfloor dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

--- Comment #14 from Andi Kleen <andi-gcc at firstfloor dot org> 2012-10-25 14:20:31 UTC ---
Is there a chance to fix this in 4.8? What remains to be done?

[Bug lto/54095] Unnecessary static variable renaming

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

--- Comment #15 from Jan Hubicka <hubicka at gcc dot gnu.org> 2013-01-16 13:18:51 UTC ---
Well, we slipped the 4.8 window :( But I will make the patch soon so it goes
into early 4.9 at least.

[Bug lto/54095] Unnecessary static variable renaming

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

--- Comment #16 from Jan Hubicka <hubicka at gcc dot gnu.org> 2013-04-08 19:49:49 UTC ---
Created attachment 29830
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29830
Complette patchet

I am attaching patch to reduce renaming.  It hits few can of worms, so I will
decompose it into smaller pieces and submit

[Bug lto/54095] Unnecessary static variable renaming

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
           Assignee|hubicka at gcc dot gnu.org         |unassigned at gcc dot gnu.org

--- Comment #17 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
I've comitted all the patches needed for late renaming of symbols.  We still do
bit more renaming than needed to deal with the fact that inline function
sections do get section names derived from symbol name of the function, but
that should be a minor problem.

jh@gcc10:~/trunk/build/gcc$ ./xgcc -B ./ -O2 -flto hello.c  -r  -nostdlib
jh@gcc10:~/trunk/build/gcc$ nm a.out
                 U bar
0000000000000000 d foo
0000000000000000 T main

I do not have immediate plans to track the inline problems, but lets keep the
PR open to remind that?

[Bug lto/54095] Unnecessary static variable renaming

[izamyatin at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54095

Igor Zamyatin <izamyatin at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |izamyatin at gmail dot com

--- Comment #18 from Igor Zamyatin <izamyatin at gmail dot com> ---
Fix seems to break compilation of 176.gcc from spec2000 with -O2 -flto on x86:

`n_occurrences' referenced in section `.text' of
/tmp/ccnjpEMk.ltrans14.ltrans.o: defined in discarded section `.text' of
reload.o (symbol from plugin)
collect2: error: ld returned 1 exit status