| Message ID | CAKOQZ8yi5MNgqu9MSC6MCyOAZDrM6tHBYGC_+=n4ds8DX4BxNg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Ian Lance Taylor <iant@google.com> writes: > On Fri, Jun 6, 2014 at 2:12 AM, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: >> Ian Lance Taylor <iant@google.com> writes: >> >>> I have committed a patch to libgo to merge from revision >>> 18783:00cce3a34d7e of the master library. This revision was committed >>> January 7. I picked this revision to merge to because the next revision >>> deleted a file that is explicitly merged in by the libgo/merge.sh >>> script. >>> >>> Among other things, this patch changes type descriptors to add a new >>> pointer to a zero value. In gccgo this is implemented as a common >>> variable, and that requires some changes to the compiler and a small >>> change to go-gcc.cc. >> >> This change introduced many failures on Solaris with /bin/ld, e.g. >> >> FAIL: go.test/test/bom.go -O (test for excess errors) >> >> ld: warning: symbol 'go$zerovalue' has differing sizes: >> (file bom.o value=0x8; file >> /var/gcc/regression/trunk/11-gcc/build/i386-pc-solaris2.11/./libgo/.libs/libgo.so >> value=0x800); >> bom.o definition taken and updated with larger size > > Interesting. This is working as intended, except for the warning. > > go$zerovalue is a common symbol, and the linker is supposed to use the > larger version. From the error message I'm guessing the Solaris > linker supports this when linking object files, but not when linking > an object file against a shared library. > > I wonder if we could avoid this warning by giving go$zerovalue hidden > visibility. That would mean something like this patch. > > Ian > > Index: go-gcc.cc > =================================================================== > --- go-gcc.cc (revision 211315) > +++ go-gcc.cc (working copy) > @@ -2521,6 +2521,8 @@ Gcc_backend::implicit_variable(const std > DECL_COMMON(decl) = 1; > TREE_PUBLIC(decl) = 1; > gcc_assert(init_tree == NULL_TREE); > + DECL_VISIBILITY(decl) = VISIBILITY_HIDDEN; > + DECL_VISIBILITY_SPECIFIED(decl) = 1; > } > else if (is_constant) > { Unfortunately, this doesn't make a difference. I've now found that ld supports -t Turns off the warning for multiply-defined tentative (common block) data symbols that have different sizes or different alignments. This option is equivalent to specifying the -z relax=common option. But I'm reluctant to enable this globally. Since Go uses no specs file, support for target-specific (linker) options would have to go into gccgo somehow. Rainer
On Wed, Jun 11, 2014 at 5:01 AM, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: > Ian Lance Taylor <iant@google.com> writes: > >> On Fri, Jun 6, 2014 at 2:12 AM, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: >>> Ian Lance Taylor <iant@google.com> writes: >>> >>>> I have committed a patch to libgo to merge from revision >>>> 18783:00cce3a34d7e of the master library. This revision was committed >>>> January 7. I picked this revision to merge to because the next revision >>>> deleted a file that is explicitly merged in by the libgo/merge.sh >>>> script. >>>> >>>> Among other things, this patch changes type descriptors to add a new >>>> pointer to a zero value. In gccgo this is implemented as a common >>>> variable, and that requires some changes to the compiler and a small >>>> change to go-gcc.cc. >>> >>> This change introduced many failures on Solaris with /bin/ld, e.g. >>> >>> FAIL: go.test/test/bom.go -O (test for excess errors) >>> >>> ld: warning: symbol 'go$zerovalue' has differing sizes: >>> (file bom.o value=0x8; file >>> /var/gcc/regression/trunk/11-gcc/build/i386-pc-solaris2.11/./libgo/.libs/libgo.so >>> value=0x800); >>> bom.o definition taken and updated with larger size >> >> Interesting. This is working as intended, except for the warning. >> >> go$zerovalue is a common symbol, and the linker is supposed to use the >> larger version. From the error message I'm guessing the Solaris >> linker supports this when linking object files, but not when linking >> an object file against a shared library. >> >> I wonder if we could avoid this warning by giving go$zerovalue hidden >> visibility. That would mean something like this patch. >> >> Ian >> >> Index: go-gcc.cc >> =================================================================== >> --- go-gcc.cc (revision 211315) >> +++ go-gcc.cc (working copy) >> @@ -2521,6 +2521,8 @@ Gcc_backend::implicit_variable(const std >> DECL_COMMON(decl) = 1; >> TREE_PUBLIC(decl) = 1; >> gcc_assert(init_tree == NULL_TREE); >> + DECL_VISIBILITY(decl) = VISIBILITY_HIDDEN; >> + DECL_VISIBILITY_SPECIFIED(decl) = 1; >> } >> else if (is_constant) >> { > > Unfortunately, this doesn't make a difference. That is peculiar. Can you verify that in libgo.so the symbol go$zerovalue is marked as hidden? I don't understand why the linker would say anything about a symbol in a .o file not matching a hidden symbol in a .so file. That seems like a linker bug, since the symbols are not being linked together. > I've now found that ld > supports > > -t > > Turns off the warning for multiply-defined tentative > (common block) data symbols that have different sizes or > different alignments. This option is equivalent to > specifying the -z relax=common option. > > But I'm reluctant to enable this globally. Since Go uses no specs file, > support for target-specific (linker) options would have to go into gccgo > somehow. Does using that option fix the problem? The place to add a target-specific linker option is gcc/go/gospec.c. See, for example, the ways it adds -Wl,-u,pthread_create on some systems. Ian
Ian Lance Taylor <iant@google.com> writes: > On Wed, Jun 11, 2014 at 5:01 AM, Rainer Orth > <ro@cebitec.uni-bielefeld.de> wrote: >> Ian Lance Taylor <iant@google.com> writes: >> >>> On Fri, Jun 6, 2014 at 2:12 AM, Rainer Orth >>> <ro@cebitec.uni-bielefeld.de> wrote: >>>> Ian Lance Taylor <iant@google.com> writes: >>>> >>>>> I have committed a patch to libgo to merge from revision >>>>> 18783:00cce3a34d7e of the master library. This revision was committed >>>>> January 7. I picked this revision to merge to because the next revision >>>>> deleted a file that is explicitly merged in by the libgo/merge.sh >>>>> script. >>>>> >>>>> Among other things, this patch changes type descriptors to add a new >>>>> pointer to a zero value. In gccgo this is implemented as a common >>>>> variable, and that requires some changes to the compiler and a small >>>>> change to go-gcc.cc. >>>> >>>> This change introduced many failures on Solaris with /bin/ld, e.g. >>>> >>>> FAIL: go.test/test/bom.go -O (test for excess errors) >>>> >>>> ld: warning: symbol 'go$zerovalue' has differing sizes: >>>> (file bom.o value=0x8; file >>>> /var/gcc/regression/trunk/11-gcc/build/i386-pc-solaris2.11/./libgo/.libs/libgo.so >>>> value=0x800); >>>> bom.o definition taken and updated with larger size >>> >>> Interesting. This is working as intended, except for the warning. >>> >>> go$zerovalue is a common symbol, and the linker is supposed to use the >>> larger version. From the error message I'm guessing the Solaris >>> linker supports this when linking object files, but not when linking >>> an object file against a shared library. >>> >>> I wonder if we could avoid this warning by giving go$zerovalue hidden >>> visibility. That would mean something like this patch. >>> >>> Ian >>> >>> Index: go-gcc.cc >>> =================================================================== >>> --- go-gcc.cc (revision 211315) >>> +++ go-gcc.cc (working copy) >>> @@ -2521,6 +2521,8 @@ Gcc_backend::implicit_variable(const std >>> DECL_COMMON(decl) = 1; >>> TREE_PUBLIC(decl) = 1; >>> gcc_assert(init_tree == NULL_TREE); >>> + DECL_VISIBILITY(decl) = VISIBILITY_HIDDEN; >>> + DECL_VISIBILITY_SPECIFIED(decl) = 1; >>> } >>> else if (is_constant) >>> { >> >> Unfortunately, this doesn't make a difference. > > That is peculiar. Can you verify that in libgo.so the symbol > go$zerovalue is marked as hidden? I don't understand why the linker > would say anything about a symbol in a .o file not matching a hidden > symbol in a .so file. That seems like a linker bug, since the symbols > are not being linked together. It's not about libgo.so. E.g. for the bug191 testcase, I get ro@arenal 118 > /var/gcc/regression/trunk/11-gcc/build/gcc/testsuite/go1/../../gccgo -B/var/gcc/regression/trunk/11-gcc/build/gcc/testsuite/go1/../../ /vol/gcc/src/hg/trunk/local/gcc/testsuite/go.test/test/fixedbugs/bug191.dir/main.go -fno-diagnostics-show-caret -fdiagnostics-color=never -I/var/gcc/regression/trunk/11-gcc/build/i386-pc-solaris2.11/./libgo -w -O2 -g a.o b.o -L/var/gcc/regression/trunk/11-gcc/build/i386-pc-solaris2.11/./libgo -L/var/gcc/regression/trunk/11-gcc/build/i386-pc-solaris2.11/./libgo/.libs -lm -o main.x -save-temps ld: warning: symbol 'go$zerovalue' has differing sizes: (file main.o value=0x8; file a.o value=0x4); main.o definition taken ld: warning: symbol 'go$zerovalue' has differing sizes: (file main.o value=0x8; file b.o value=0x4); main.o definition taken ro@arenal 120 > elfdump -s main.o a.o b.o|egrep 'index|zerov' index value size type bind oth ver shndx name [38] 0x4 0x8 OBJT GLOB H 0 COMMON go$zerovalue index value size type bind oth ver shndx name [27] 0x4 0x4 OBJT GLOB H 0 COMMON go$zerovalue index value size type bind oth ver shndx name [27] 0x4 0x4 OBJT GLOB H 0 COMMON go$zerovalue >> I've now found that ld >> supports >> >> -t >> >> Turns off the warning for multiply-defined tentative >> (common block) data symbols that have different sizes or >> different alignments. This option is equivalent to >> specifying the -z relax=common option. >> >> But I'm reluctant to enable this globally. Since Go uses no specs file, >> support for target-specific (linker) options would have to go into gccgo >> somehow. > > Does using that option fix the problem? Yup, it does, I tried linking the testcase above with -Wl,-t added. > The place to add a target-specific linker option is gcc/go/gospec.c. > See, for example, the ways it adds -Wl,-u,pthread_create on some > systems. Ok, I'll have a look unless some other solution can be found. Rainer
On Wed, Jun 11, 2014 at 6:57 AM, Rainer Orth <ro@cebitec.uni-bielefeld.de> wrote: > > Ok, I'll have a look unless some other solution can be found. There are other, less efficient, ways that this could be compiled, but a common symbol seems like the right solution. Basically, every type descriptor now points to a zero value for the type. The zero value in Go is always the all-bytes-zero value. So we can have every type descriptor point to the same block of memory, as long as we can ensure that that memory is large enough. Using a common symbol does exactly what we want: the linker sees all the go$zerovalue symbols, and sets the final one to the largest go$zerovalue that it sees. I can't think of any other solution that doesn't involve some sort of runtime initialization. I do wonder what gfortran does on Solaris. Ian
Ian Lance Taylor <iant@google.com> writes: > On Wed, Jun 11, 2014 at 6:57 AM, Rainer Orth > <ro@cebitec.uni-bielefeld.de> wrote: >> >> Ok, I'll have a look unless some other solution can be found. > > There are other, less efficient, ways that this could be compiled, but > a common symbol seems like the right solution. > > Basically, every type descriptor now points to a zero value for the > type. The zero value in Go is always the all-bytes-zero value. So > we can have every type descriptor point to the same block of memory, > as long as we can ensure that that memory is large enough. Using a > common symbol does exactly what we want: the linker sees all the > go$zerovalue symbols, and sets the final one to the largest > go$zerovalue that it sees. > > I can't think of any other solution that doesn't involve some sort of > runtime initialization. > > I do wonder what gfortran does on Solaris. This case only occurs in a few LTO testcases, where they are pruned by lto.exp (lto_prune_warns). Apart from that, the only testcase I could find is gfortran.dg/bind_c_coms.f90 with gfortran.dg/bind_c_coms_driver.c, and there all common symbols are the same size in the C and F90 cases. Rainer
Index: go-gcc.cc =================================================================== --- go-gcc.cc (revision 211315) +++ go-gcc.cc (working copy) @@ -2521,6 +2521,8 @@ Gcc_backend::implicit_variable(const std DECL_COMMON(decl) = 1; TREE_PUBLIC(decl) = 1; gcc_assert(init_tree == NULL_TREE); + DECL_VISIBILITY(decl) = VISIBILITY_HIDDEN; + DECL_VISIBILITY_SPECIFIED(decl) = 1; } else if (is_constant) {