code review 8602044: cmd/ld: fix inconsistency in internal linking of data s... (issue 8602044)

[remyoud...@gmail.com]

Reviewers: golang-dev1,

Message:
Hello golan...@googlegroups.com (cc: golan...@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
cmd/ld: fix inconsistency in internal linking of data symbols.

Some variables declared in C could end up as undefined symbols
in the final binary and have null address.

Fixes issue 5227.

Please review this at https://codereview.appspot.com/8602044/

Affected files:
M misc/cgo/test/cgo_test.go
A misc/cgo/test/issue5227.go
M src/cmd/ld/lib.c


Index: misc/cgo/test/cgo_test.go
===================================================================
--- a/misc/cgo/test/cgo_test.go
+++ b/misc/cgo/test/cgo_test.go
@@ -37,5 +37,6 @@
func Test3775(t *testing.T) { test3775(t) }
func TestCthread(t *testing.T) { testCthread(t) }
func TestCallbackCallers(t *testing.T) { testCallbackCallers(t) }
+func Test5227(t *testing.T) { test5227(t) }

func BenchmarkCgoCall(b *testing.B) { benchCgoCall(b) }
Index: misc/cgo/test/issue5227.go
===================================================================
new file mode 100644
--- /dev/null
+++ b/misc/cgo/test/issue5227.go
@@ -0,0 +1,38 @@
+// Copyright 2013 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Issue 5227: linker incorrectly treats data symbols and
+// leaves them undefined.
+
+package cgotest
+
+/*
+typedef struct {
+ int Count;
+} Fontinfo;
+
+Fontinfo SansTypeface;
+
+extern void init();
+
+Fontinfo loadfont() {
+ Fontinfo f;
+ return f;
+}
+
+void init() {
+ SansTypeface = loadfont();
+}
+*/
+import "C"
+
+import "testing"
+
+func test5227(t *testing.T) {
+ C.init()
+}
+
+func selectfont() C.Fontinfo {
+ return C.SansTypeface
+}
Index: src/cmd/ld/lib.c
===================================================================
--- a/src/cmd/ld/lib.c
+++ b/src/cmd/ld/lib.c
@@ -311,6 +311,9 @@
// Switch to internal.
if(linkmode == LinkAuto) {
linkmode = LinkInternal;
+ }
+
+ if(linkmode == LinkInternal) {
// Drop all the cgo_import_static declarations.
// Turns out we won't be needing them.
for(s = allsym; s != S; s = s->allsym)

[remyoud...@gmail.com]

I may not know the appropriate vocabulary, feel free to suggest an
appropriate rewording.

The new test fails on amd64 and arm with linkmode=internal, and it
passes after the patch.

https://codereview.appspot.com/8602044/

[ia...@golang.org]

LGTM

In the change description, s/data/common/

Thanks.



https://codereview.appspot.com/8602044/diff/4004/misc/cgo/test/issue5227.go
File misc/cgo/test/issue5227.go (right):

https://codereview.appspot.com/8602044/diff/4004/misc/cgo/test/issue5227.go#newcode5
misc/cgo/test/issue5227.go:5: // Issue 5227: linker incorrectly treats
data symbols and
s/treats data/handles common/

https://codereview.appspot.com/8602044/

[Anthony Starks]

Also fixes issue 5114

[da...@cheney.net]

This built fine on my pandaboard linux/arm

https://codereview.appspot.com/8602044/

[remyoud...@gmail.com]

https://codereview.appspot.com/8602044/

[remyoud...@gmail.com]

Done.

I need a LGTM from r or adg right?

https://codereview.appspot.com/8602044/

[r...@golang.org]

LGTM

https://codereview.appspot.com/8602044/

[Rob Pike]

And now you have it. Thanks.

-rob

[remyoud...@gmail.com]

*** Submitted as
https://code.google.com/p/go/source/detail?r=b27b1ff18f39 ***

cmd/ld: fix inconsistency in internal linking of common symbols.

Some variables declared in C could end up as undefined symbols
in the final binary and have null address.

Fixes issue 5114.
Fixes issue 5227.

R=golang-dev, iant, ajstarks, dave, r
CC=golang-dev
https://codereview.appspot.com/8602044


https://codereview.appspot.com/8602044/

[capnm]

Thanks to all,

nice to wake up and see this fixed.

Now I can move on to my next puzzle:

How to convince a million children to play with Go.

Martin

http://www.raspberrypi.org/archives/3011

[minux]

Thank you for fixing this!