code review 58410043: cmd/ld: fix bug with "runtime/cgo" in external link mode

cmd/ld: fix bug with "runtime/cgo" in external link mode

In external link mode the linker explicitly adds the string
constant "runtime/cgo".  It adds the string constant using the
same symbol name as the compiler, but a different format.  The
compiler assumes that the string data immediately follows the
string header, but the linker puts the two in different
sections.  The result is bad string data when the compiler
sees "runtime/cgo" used as a string constant.

The compiler assumption is in datastring in [568]g/gobj.c.
The linker layout is in addstrdata in ld/data.c.  The compiler
assumption is valid for string literals.  The linker is not
creating a string literal, so its assumption is also valid.

There are a few ways to avoid this problem.  This patch fixes
it by only doing the fake import of runtime/cgo if necessary,
and by only creating the string symbol if necessary.

Fixes issue 7234.

[iant, 2014-01-29 23:09:08]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[dvyukov, 2014-01-30 06:44:45]

LGTM

[bradfitz, 2014-01-30 09:03:02]

s/it's/its/



On Thu, Jan 30, 2014 at 12:09 AM, <iant@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> cmd/ld: fix bug with "runtime/cgo" in external link mode
>
> In external link mode the linker explicitly adds the string
> constant "runtime/cgo".  It adds the string constant using the
> same symbol name as the compiler, but a different format.  The
> compiler assumes that the string data immediately follows the
> string header, but the linker puts the two in different
> sections.  The result is bad string data when the compiler
> sees "runtime/cgo" used as a string constant.
>
> The compiler assumption is in datastring in [568]g/gobj.c.
> The linker layout is in addstrdata in ld/data.c.  The compiler
> assumption is valid for string literals.  The linker is not
> creating a string literal, so it's assumption is also valid.
>
> There are a few ways to avoid this problem.  This patch fixes
> it by only doing the fake import of runtime/cgo if necessary,
> and by only creating the string symbol if necessary.
>
> Fixes issue 7234.
>
> Please review this at https://codereview.appspot.com/58410043/
>
> Affected files (+42, -18 lines):
>   A misc/cgo/test/issue7234_test.go
>   M src/cmd/ld/lib.c
>
>
> Index: misc/cgo/test/issue7234_test.go
> ===================================================================
> new file mode 100644
> --- /dev/null
> +++ b/misc/cgo/test/issue7234_test.go
> @@ -0,0 +1,21 @@
> +// Copyright 2014 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.
> +
> +package cgotest
> +
> +import "testing"
> +
> +// This test actually doesn't have anything to do with cgo.  It is a
> +// test of http://golang.org/issue/7234, a compiler/linker bug in
> +// handling string constants when using -linkmode=external.  The test
> +// is in this directory because we routinely test -linkmode=external
> +// here.
> +
> +var v7234 = [...]string{"runtime/cgo"}
> +
> +func TestIssue7234(t *testing.T) {
> +       if v7234[0] != "runtime/cgo" {
> +               t.Errorf("bad string constant %q", v7234[0])
> +       }
> +}
> Index: src/cmd/ld/lib.c
> ===================================================================
> --- a/src/cmd/ld/lib.c
> +++ b/src/cmd/ld/lib.c
> @@ -164,6 +164,7 @@
>  {
>         int i, w, x;
>         LSym *s, *gmsym;
> +       char* cgostrsym;
>
>         if(flag_shared) {
>                 s = linklookup(ctxt, "runtime.islibrary", 0);
> @@ -176,24 +177,6 @@
>                 loadinternal("math");
>         if(flag_race)
>                 loadinternal("runtime/race");
> -       if(linkmode == LinkExternal) {
> -               // This indicates a user requested -linkmode=external.
> -               // The startup code uses an import of runtime/cgo to decide
> -               // whether to initialize the TLS.  So give it one.  This
> could
> -               // be handled differently but it's an unusual case.
> -               loadinternal("runtime/cgo");
> -
> -               // Pretend that we really imported the package.
> -               // This will do no harm if we did in fact import it.
> -               s = linklookup(ctxt, "go.importpath.runtime/cgo.", 0);
> -               s->type = SDATA;
> -               s->dupok = 1;
> -               s->reachable = 1;
> -
> -               // Provided by the code that imports the package.
> -               // Since we are simulating the import, we have to provide
> this string.
> -               addstrdata("go.string.\"runtime/cgo\"", "runtime/cgo");
> -       }
>
>         for(i=0; i<ctxt->libraryp; i++) {
>                 if(debug['v'] > 1)
> @@ -202,6 +185,26 @@
>                 objfile(ctxt->library[i].file, ctxt->library[i].pkg);
>         }
>
> +       if(linkmode == LinkExternal && !iscgo) {
> +               // This indicates a user requested -linkmode=external.
> +               // The startup code uses an import of runtime/cgo to decide
> +               // whether to initialize the TLS.  So give it one.  This
> could
> +               // be handled differently but it's an unusual case.
> +               loadinternal("runtime/cgo");
> +
> +               // Pretend that we really imported the package.
> +               s = linklookup(ctxt, "go.importpath.runtime/cgo.", 0);
> +               s->type = SDATA;
> +               s->dupok = 1;
> +               s->reachable = 1;
> +
> +               // Provided by the code that imports the package.
> +               // Since we are simulating the import, we have to provide
> this string.
> +               cgostrsym = "go.string.\"runtime/cgo\"";
> +               if(linkrlookup(ctxt, cgostrsym, 0) == nil)
> +                       addstrdata(cgostrsym, "runtime/cgo");
> +       }
> +
>         if(linkmode == LinkAuto) {
>                 if(iscgo && externalobj)
>                         linkmode = LinkExternal;
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[iant, 2014-01-30 17:25:55]

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

cmd/ld: fix bug with "runtime/cgo" in external link mode

In external link mode the linker explicitly adds the string
constant "runtime/cgo".  It adds the string constant using the
same symbol name as the compiler, but a different format.  The
compiler assumes that the string data immediately follows the
string header, but the linker puts the two in different
sections.  The result is bad string data when the compiler
sees "runtime/cgo" used as a string constant.

The compiler assumption is in datastring in [568]g/gobj.c.
The linker layout is in addstrdata in ld/data.c.  The compiler
assumption is valid for string literals.  The linker is not
creating a string literal, so its assumption is also valid.

There are a few ways to avoid this problem.  This patch fixes
it by only doing the fake import of runtime/cgo if necessary,
and by only creating the string symbol if necessary.

Fixes issue 7234.

LGTM=dvyukov
R=golang-codereviews, dvyukov, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/58410043

[gobot, 2014-01-30 17:29:23]

This CL appears to have broken the darwin-amd64 builder.

[iant, 2014-01-30 18:52:03]

On Thu, Jan 30, 2014 at 9:29 AM,  <gobot@golang.org> wrote:
> This CL appears to have broken the darwin-amd64 builder.
>
> https://codereview.appspot.com/58410043/

I'm skeptical, as this CL should only affect external linking, and
http://build.golang.org/log/840d6baa24bd3e4c3760cdc809e8fc55f73b7ac5
is showing a segmentation violation when testing the reflect package.
Let's see what happens after the next commit.

Ian

[bradfitz, 2014-01-30 19:14:12]

That looks like the failure Keith filed bugs about.
On Jan 30, 2014 7:52 PM, "Ian Lance Taylor" <iant@golang.org> wrote:

> On Thu, Jan 30, 2014 at 9:29 AM,  <gobot@golang.org> wrote:
> > This CL appears to have broken the darwin-amd64 builder.
> >
> > https://codereview.appspot.com/58410043/
>
> I'm skeptical, as this CL should only affect external linking, and
> http://build.golang.org/log/840d6baa24bd3e4c3760cdc809e8fc55f73b7ac5
> is showing a segmentation violation when testing the reflect package.
> Let's see what happens after the next commit.
>
> Ian
>

[dave_cheney.net, 2014-01-31 00:24:08]

I believe the build failure was,
https://code.google.com/p/go/issues/detail?id=7205, which is unrelated to
the commit.


On Fri, Jan 31, 2014 at 5:52 AM, Ian Lance Taylor <iant@golang.org> wrote:

> On Thu, Jan 30, 2014 at 9:29 AM,  <gobot@golang.org> wrote:
> > This CL appears to have broken the darwin-amd64 builder.
> >
> > https://codereview.appspot.com/58410043/
>
> I'm skeptical, as this CL should only affect external linking, and
> http://build.golang.org/log/840d6baa24bd3e4c3760cdc809e8fc55f73b7ac5
> is showing a segmentation violation when testing the reflect package.
> Let's see what happens after the next commit.
>
> Ian
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>