[Bug go/61232] New: [4.10 Regression] link errors building libgo

[Bug go/61232] New: [4.10 Regression] link errors building libgo

[doko at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

            Bug ID: 61232
           Summary: [4.10 Regression] link errors building libgo
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: go
          Assignee: ian at airs dot com
          Reporter: doko at gcc dot gnu.org

libgo fails to link, at least on x86*, powerpc*, arm*, aarch64* -linux with
trunk 20140519:

.libs/bytes.o:(.data.rel.C237[__go_td_FppN12_bytes.ReaderN9_io.WritererN5_int64N5_erroree]+0x0):
multiple definition of `C237'
.libs/bufio.o:(.bss.C237[__go_td_FpN16_bufio.ReadWriteree]+0x0): first defined
here
.libs/bytes.o: In function `bytes.Len.pN12_bytes.Buffer':
/build/buildd/gcc-snapshot-20140519/build/aarch64-linux-gnu/libgo/../../../src/libgo/go/bytes/buffer.go:57:
multiple definition of `C217'
.libs/bufio.o:/build/buildd/gcc-snapshot-20140519/build/aarch64-linux-gnu/libgo/../../../src/libgo/go/bufio/bufio.go:71:
first defined here
/usr/bin/aarch64-linux-gnu-ld: Warning: size of symbol `C217' changed from 16
in .libs/bufio.o to 8 in .libs/bytes.o
.libs/bytes.o: In function `bytes.Len.pN12_bytes.Buffer':
[some hundred messages like these follow]

complete build logs at
https://launchpad.net/ubuntu/+source/gcc-snapshot/20140519-0ubuntu1

[Bug go/61232] [4.10 Regression] link errors building libgo

[doko at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

--- Comment #1 from Matthias Klose <doko at gcc dot gnu.org> ---
this was r210608

[Bug go/61232] [4.10 Regression] link errors building libgo

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|4.9.0                       |4.10.0
   Target Milestone|---                         |4.10.0

[Bug go/61232] [4.10 Regression] link errors building libgo

[ian at airs dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

Ian Lance Taylor <ian at airs dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at ucw dot cz

--- Comment #2 from Ian Lance Taylor <ian at airs dot com> ---
Something is setting the comdat group of a static (TREE_PUBLIC == DECL_EXTERNAL
== 0) variable.  That fails when the static variable does not have a unique
name, as in this case.

It's not revision 210608, that only affects the SH backend.

[Bug go/61232] [4.10 Regression] link errors building libgo

[ian at airs dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

--- Comment #3 from Ian Lance Taylor <ian at airs dot com> ---
It's almost certainly the new ipa-comdats pass.  When ipa-comdats converts a
variable from static to comdat, it needs to give it a unique name.

[Bug go/61232] [4.10 Regression] link errors building libgo

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2014-05-19
                 CC|                            |hubicka at gcc dot gnu.org
           Assignee|ian at airs dot com                |hubicka at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #4 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
I am going to reproduce it, but I am bit confused here.  I intend to make
static symbols inside a comdat group.  That is I have a comdat group with
external comdat symbol (i.e. the variable the comdat was created for) and I
find a variable that is referenced only from its constructor.  I intend to
introduce static symbol inside that section and I do not want to export it out.
We do that for thunks, so I assumed it works for variables, too.  I will check
what exactly goes wrong here.

Honza

[Bug go/61232] [4.10 Regression] link errors building libgo

[ian at airs dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

Ian Lance Taylor <ian at airs dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ian at airs dot com

--- Comment #5 from Ian Lance Taylor <ian at airs dot com> ---
I haven't been able to figure out how to recreate this in C/C++.

The case in Go is straightforward: a comdat weak variable (specifically, a Go
type descriptor for an unnamed type) has an initializer that refers to the
address of a static variable.  That static variable does not have a unique
name.  There are hundreds of cases in the standard Go library.

I have a patch for the Go frontend that simply gives the static variables a
unique name.  That seems to fix the problem for Go.

It doesn't fix the general problem for the pass, which is: the pass assumes
that when a comdat variable refers to a static variable, the static variable
has a unique name.  It happens that for C/C++, as far as I can tell, they
always do have a unique name.  To fix the pass properly I think you'll need to
give those static variables a unique asm name without disturbing the debug
info.

[Bug go/61232] [4.10 Regression] link errors building libgo

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

--- Comment #6 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Well, is there a reason why I can not place static symbol into the comdat
section, just as we do for labels within functions or thunks? Those also do not
have unique name.
It seems to me that there is a problem in the output machinery confusing comdat
locals with comdats in the case of variables. Let me see if I can fix it easily
or I will disable the pass for variables for time being.

[Bug go/61232] [4.10 Regression] link errors building libgo

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

--- Comment #7 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Created attachment 32821
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=32821&action=edit
patch I am testing

This patch seems to do what I want - it makes the comdat local variables to be
in the comdat group section but not declared as unique_object (and thus they
should not land the symbol table).

Iant, since I do have your attention and you know a lot more about the topic,
does it seem sane?

[Bug go/61232] [4.10 Regression] link errors building libgo

[ian at airs dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

--- Comment #8 from Ian Lance Taylor <ian at airs dot com> ---
The naming is confusing and but the idea seems sound.  It doesn't make sense
for a static symbol to be DECL_ONE_ONLY.  But currently DECL_ONE_ONLY just
means that the symbol has a comdat group.  So given that, I guess it makes
sense to speak about DECL_ONE_ONLY && !TREE_PUBLIC meaning a static symbol in a
comdat group, where the signature for the comdat group must obviously be some
other symbol.

[Bug go/61232] [4.10 Regression] link errors building libgo

[hubicka at ucw dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

--- Comment #9 from Jan Hubicka <hubicka at ucw dot cz> ---
> The naming is confusing and but the idea seems sound.  It doesn't make sense
> for a static symbol to be DECL_ONE_ONLY.  But currently DECL_ONE_ONLY just
> means that the symbol has a comdat group.  So given that, I guess it makes
> sense to speak about DECL_ONE_ONLY && !TREE_PUBLIC meaning a static symbol in a
> comdat group, where the signature for the comdat group must obviously be some
> other symbol.

I agree that DECL_ONE_ONLY is confusing in this respect - perhaps we can change
its
definition to contain && (TREE_PUBLIC (decl) || DECL_EXTERNAL (decl)) and
update
uses where we really care about COMDAT_GROUP being non-null to use
DECL_COMDAT_GROUP () != NULL.
I think those are basically the output machinery bits in varasm.c setting up
the proper
comdat section.

If you think it is cleaner, I will prepare patch.

Honza

[Bug go/61232] [4.10 Regression] link errors building libgo

[hubicka at ucw dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

--- Comment #11 from Jan Hubicka <hubicka at ucw dot cz> ---
> Yes, I think that would be clearer.
> 
> Your patch does seem to fix the problem building libgo.  Thanks.

Thanks for help!

I am testing updated patch - it turns out that I needed to revisit about every
use of DECL_ONE_ONLY and think what to do with comdat locals. While many of
uses does not matter for now since the locals are mostly introduced relatively
late in compilation (except for Jason's declonning), it is probably desirable
to get them right.

Honza

[Bug go/61232] [4.10 Regression] link errors building libgo

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61232

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #12 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Souhld be fixed by commit
2014-05-20  Jan Hubicka  <hubicka@ucw.cz>

        * tree.h (DECL_ONE_ONLY): Return true only for externally visible
        symbols.
        * except.c (switch_to_exception_section, resolve_unique_section,
        get_named_text_section, default_function_rodata_section, 
        align_variable, get_block_for_decl, default_section_type_flags):
        Use DECL_COMDAT_GROUP instead of DECL_ONE_ONLY.
        * symtab.c (symtab_add_to_same_comdat_group,
        symtab_make_decl_local, fixup_same_cpp_alias_visibility,
        symtab_nonoverwritable_alias, symtab_get_symbol_partitioning_class):
        Likewise.
        * cgraphclones.c (cgraph_create_virtual_clone): Likewise.
        * bb-reorder.c (pass_partition_blocks::gate): Likewise.
        * config/c6x/c6x.c (c6x_elf_unique_section): Likewise.
        (c6x_function_in_section_p): Likewise.
        * config/darwin.c (machopic_select_section): Likewise.
        * config/arm/arm.c (arm_function_in_section_p): Likewise.
        * config/mips/mips.c (mips_function_rodata_section): Likewise.
        * config/mep/mep.c (mep_select_section): LIkewise.
        * config/i386/i386.c (x86_64_elf_unique_section): Likewise.