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

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.

[remyoudompheng, 2013-04-09 22:42:05]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

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

[remyoudompheng, 2013-04-09 22:42:58]

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.

[iant, 2013-04-10 00:04:20]

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#n...
misc/cgo/test/issue5227.go:5: // Issue 5227: linker incorrectly treats data
symbols and
s/treats data/handles common/

[ajstarks, 2013-04-10 02:29:15]

Also fixes issue 5114

On Tuesday, April 9, 2013 6:42:05 PM UTC-4, Rémy wrote:
>
> Reviewers: golang-dev1, 
>
> Message: 
> Hello golan...@googlegroups.com <javascript:> (cc: 
> golan...@googlegroups.com <javascript:>), 
>
> 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) 
>
>
>

[dave_cheney.net, 2013-04-10 04:42:55]

This built fine on my pandaboard linux/arm

[remyoudompheng, 2013-04-10 05:08:11]

Done.

I need a LGTM from r or adg right?

[r, 2013-04-10 05:11:10]

LGTM

[r, 2013-04-10 05:11:39]

And now you have it. Thanks.

-rob

[remyoudompheng, 2013-04-10 05:16:12]

*** 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

[capnm, 2013-04-10 07:47:04]

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

On Wednesday, 10 April 2013 00:42:05 UTC+2, Rémy wrote:
>
> Reviewers: golang-dev1, 
>
> Message: 
> Hello golan...@googlegroups.com <javascript:> (cc: 
> golan...@googlegroups.com <javascript:>), 
>
> 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/ 
>
 

[minux1, 2013-04-10 09:18:49]

Thank you for fixing this!

On Wed, Apr 10, 2013 at 1:16 PM,  <remyoudompheng@gmail.com> wrote:
> *** 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.