Patchwork gcc 4.7 libgo patch committed: Set libgo version number

login
register
mail settings
Submitter Ian Taylor
Date Oct. 23, 2012, 4:55 a.m.
Message ID <mcrzk3dokii.fsf@google.com>
Download mbox | patch
Permalink /patch/193352/
State New
Headers show

Comments

Ian Taylor - Oct. 23, 2012, 4:55 a.m.
PR 54918 points out that libgo is not using version numbers as it
should.  At present none of libgo in 4.6, 4.7 and mainline are
compatible with each other.  This patch to the 4.7 branch sets the
version number for libgo there.  Bootstrapped and ran Go testsuite on
x86_64-unknown-linux-gnu.  Committed to 4.7 branch.

Ian
Jan Kratochvil - Oct. 23, 2012, 7:37 a.m.
Hello Ian,

On Tue, 23 Oct 2012 06:55:01 +0200, Ian Lance Taylor wrote:
> PR 54918 points out that libgo is not using version numbers as it
> should.  At present none of libgo in 4.6, 4.7 and mainline are
> compatible with each other.  This patch to the 4.7 branch sets the
> version number for libgo there.  Bootstrapped and ran Go testsuite on
> x86_64-unknown-linux-gnu.  Committed to 4.7 branch.

it has regressed GDB testsuite:
	-PASS: gdb.go/handcall.exp: print add (1, 2)
	+FAIL: gdb.go/handcall.exp: print add (1, 2)

GNU gdb (GDB) 7.5.50.20121022-cvs

before:
(gdb) print add (1, 2)
$1 = 3
(gdb) ptype add
type = int32 (int, int)
(gdb) info line add
Line 219 of "../../../libgo/runtime/cpuprof.c" starts at address 0x7ffff55c0884 <tick+52> and ends at 0x7ffff55c0898 <tick+72>.

now:
(gdb) print add (1, 2)
Too few arguments in function call.
(gdb) ptype add
type = void (Profile *, uintptr *, int32)
(gdb) info line add
Line 212 of "../../../gcc47/libgo/runtime/cpuprof.c" starts at address 0x7ffff55b05fe <add> and ends at 0x7ffff55b0609 <add+11>.


Regards,
Jan
Ian Taylor - Oct. 23, 2012, 1:21 p.m.
On Tue, Oct 23, 2012 at 12:37 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
>
> On Tue, 23 Oct 2012 06:55:01 +0200, Ian Lance Taylor wrote:
>> PR 54918 points out that libgo is not using version numbers as it
>> should.  At present none of libgo in 4.6, 4.7 and mainline are
>> compatible with each other.  This patch to the 4.7 branch sets the
>> version number for libgo there.  Bootstrapped and ran Go testsuite on
>> x86_64-unknown-linux-gnu.  Committed to 4.7 branch.
>
> it has regressed GDB testsuite:
>         -PASS: gdb.go/handcall.exp: print add (1, 2)
>         +FAIL: gdb.go/handcall.exp: print add (1, 2)
>
> GNU gdb (GDB) 7.5.50.20121022-cvs
>
> before:
> (gdb) print add (1, 2)
> $1 = 3
> (gdb) ptype add
> type = int32 (int, int)
> (gdb) info line add
> Line 219 of "../../../libgo/runtime/cpuprof.c" starts at address 0x7ffff55c0884 <tick+52> and ends at 0x7ffff55c0898 <tick+72>.
>
> now:
> (gdb) print add (1, 2)
> Too few arguments in function call.
> (gdb) ptype add
> type = void (Profile *, uintptr *, int32)
> (gdb) info line add
> Line 212 of "../../../gcc47/libgo/runtime/cpuprof.c" starts at address 0x7ffff55b05fe <add> and ends at 0x7ffff55b0609 <add+11>.

In both the before and after, gdb seems to be picking up the wrong
version of add, according to the line number information.  It's
picking up the static function add defined in the libgo library, not
the function defined in the user code.  The type information is
different but in fact the type of the static function in the libgo
library has not changed since it was introduced in March, 2011.  So
somehow in the before case gdb is displaying the line number of the
static function add but the type of the Go function add, but in the
after case gdb is displaying the line number and the type of the
static function.

I don't have any explanation for this difference but it's hard for me
to believe that the root cause is in libgo.  In effect there are two
functions named add: one is in libgo with C mangling, and one is in
the user code with Go mangling.  gdb is not picking the one that the
testsuite expects it to pick.

Ian
Matthias Klose - Oct. 23, 2012, 4:16 p.m.
On 23.10.2012 06:55, Ian Lance Taylor wrote:
> PR 54918 points out that libgo is not using version numbers as it
> should.  At present none of libgo in 4.6, 4.7 and mainline are
> compatible with each other.  This patch to the 4.7 branch sets the
> version number for libgo there.  Bootstrapped and ran Go testsuite on
> x86_64-unknown-linux-gnu.  Committed to 4.7 branch.

changing the soname of a runtime library on the branch? I don't like this idea
at all.

  Matthias
Jakub Jelinek - Oct. 23, 2012, 4:27 p.m.
On Tue, Oct 23, 2012 at 06:16:25PM +0200, Matthias Klose wrote:
> On 23.10.2012 06:55, Ian Lance Taylor wrote:
> > PR 54918 points out that libgo is not using version numbers as it
> > should.  At present none of libgo in 4.6, 4.7 and mainline are
> > compatible with each other.  This patch to the 4.7 branch sets the
> > version number for libgo there.  Bootstrapped and ran Go testsuite on
> > x86_64-unknown-linux-gnu.  Committed to 4.7 branch.
> 
> changing the soname of a runtime library on the branch? I don't like this idea
> at all.

Me neither.  You should just make sure you increment the soname on the trunk
whenever new major release has ABI changes, and don't do ABI changes on the release
branches.

	Jakub
Ian Taylor - Oct. 23, 2012, 4:57 p.m.
On Tue, Oct 23, 2012 at 9:27 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Oct 23, 2012 at 06:16:25PM +0200, Matthias Klose wrote:
>> On 23.10.2012 06:55, Ian Lance Taylor wrote:
>> > PR 54918 points out that libgo is not using version numbers as it
>> > should.  At present none of libgo in 4.6, 4.7 and mainline are
>> > compatible with each other.  This patch to the 4.7 branch sets the
>> > version number for libgo there.  Bootstrapped and ran Go testsuite on
>> > x86_64-unknown-linux-gnu.  Committed to 4.7 branch.
>>
>> changing the soname of a runtime library on the branch? I don't like this idea
>> at all.
>
> Me neither.  You should just make sure you increment the soname on the trunk
> whenever new major release has ABI changes, and don't do ABI changes on the release
> branches.

The problem is that I forgot to do that when the 4.7 branch was
created.  So the 4.7 branch and the 4.6 branch were using the same
SONAME although they had completely different ABIs.

That is, there is no ABI change on the 4.7 branch.  I'm setting the
SONAME to distinguish it from the 4.6 branch.

I agree is not ideal but it seems like the best approach under the
circumstances.

I'll roll it back if y'all continue to think it is a bad idea.

Ian
Jakub Jelinek - Oct. 23, 2012, 4:59 p.m.
On Tue, Oct 23, 2012 at 09:57:21AM -0700, Ian Lance Taylor wrote:
> The problem is that I forgot to do that when the 4.7 branch was
> created.  So the 4.7 branch and the 4.6 branch were using the same
> SONAME although they had completely different ABIs.
> 
> That is, there is no ABI change on the 4.7 branch.  I'm setting the
> SONAME to distinguish it from the 4.6 branch.
> 
> I agree is not ideal but it seems like the best approach under the
> circumstances.

I think it is too late for such a change on the 4.7 branch, better
just say that the go support in 4.6 is experimental, without stable ABI,
otherwise there will be ABI incompatibility also on the 4.7 branch
between patchlevel versions thereof.

	Jakub
Ian Taylor - Oct. 23, 2012, 6:13 p.m.
On Tue, Oct 23, 2012 at 9:59 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Oct 23, 2012 at 09:57:21AM -0700, Ian Lance Taylor wrote:
>> The problem is that I forgot to do that when the 4.7 branch was
>> created.  So the 4.7 branch and the 4.6 branch were using the same
>> SONAME although they had completely different ABIs.
>>
>> That is, there is no ABI change on the 4.7 branch.  I'm setting the
>> SONAME to distinguish it from the 4.6 branch.
>>
>> I agree is not ideal but it seems like the best approach under the
>> circumstances.
>
> I think it is too late for such a change on the 4.7 branch, better
> just say that the go support in 4.6 is experimental, without stable ABI,
> otherwise there will be ABI incompatibility also on the 4.7 branch
> between patchlevel versions thereof.

OK, I reverted the patch on the 4.7 branch.

Ian

Patch

Index: configure.ac
===================================================================
--- configure.ac	(revision 191576)
+++ configure.ac	(working copy)
@@ -11,7 +11,7 @@  AC_INIT(package-unused, version-unused,,
 AC_CONFIG_SRCDIR(Makefile.am)
 AC_CONFIG_HEADER(config.h)
 
-libtool_VERSION=1:0:0
+libtool_VERSION=2:1:0
 AC_SUBST(libtool_VERSION)
 
 AM_ENABLE_MULTILIB(, ..)
Index: Makefile.am
===================================================================
--- Makefile.am	(revision 192024)
+++ Makefile.am	(working copy)
@@ -1753,7 +1753,8 @@  libgo_go_objs = \
 
 libgo_la_SOURCES = $(runtime_files)
 
-libgo_la_LDFLAGS = $(PTHREAD_CFLAGS) $(AM_LDFLAGS)
+libgo_la_LDFLAGS = \
+	-version-info $(libtool_VERSION) $(PTHREAD_CFLAGS) $(AM_LDFLAGS)
 
 libgo_la_LIBADD = \
 	$(libgo_go_objs) $(LIBFFI) $(PTHREAD_LIBS) $(MATH_LIBS) $(NET_LIBS)