gccgo: go test -compiler gccgo github.com/golang/protobuf/proto fails

[mwhudson]

Please answer these questions before submitting your issue. Thanks!

1. What version of Go are you using (go version)?

mwhudson@aeglos:~$ gccgo-5 --version
gccgo-5 (Ubuntu 5.3.1-14ubuntu2) 5.3.1 20160413
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

mwhudson@aeglos:~$ gccgo-6 --version
gccgo-6 (Ubuntu 6.0.1-0ubuntu1) 6.0.0 20160414 (experimental) [trunk revision 234994]
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

1. What operating system and processor architecture are you using (go env)?

Ubuntu 16.04

1. What did you do?

go get github.com/golang/protobuf/...
go test -compiler gccgo github.com/golang/protobuf/proto

1. What did you expect to see?

ok github.com/golang/protobuf/proto 0.028s

1. What did you see instead?

mwhudson@aeglos:~$ go test -compiler gccgo github.com/golang/protobuf/proto
panic: runtime error: assignment to entry in nil map

goroutine 16 [running]:
runtime_dopanic
    ../../../src/libgo/runtime/panic.c:131
__go_panic
    ../../../src/libgo/runtime/go-panic.c:111
runtime_panicstring
    ../../../src/libgo/runtime/panic.c:216
__go_map_index
    ../../../src/libgo/runtime/go-map-index.c:94
github_com_golang_protobuf_proto.RegisterType
    /opt/opensource/gopath/src/github.com/golang/protobuf/proto/properties.go:838
any.$init0
    /opt/opensource/gopath/src/github.com/golang/protobuf/ptypes/any/any.pb.go:94
github_com_golang_protobuf_ptypes_any..import
    /opt/opensource/gopath/src/github.com/golang/protobuf/ptypes/any/any.pb.go:93
__go_init_main
    /tmp/go-build202436886/github.com/golang/protobuf/proto/_test/_testmain.go:2
runtime_main
    ../../../src/libgo/runtime/proc.c:604

goroutine 17 [runnable]:
kickoff
    ../../../src/libgo/runtime/proc.c:232
created by runtime_main
    ../../../src/libgo/runtime/proc.c:598

goroutine 18 [runnable]:
kickoff
    ../../../src/libgo/runtime/proc.c:232
created by runtime_createfing
    ../../../src/libgo/runtime/mgc0.c:2577
FAIL    github.com/golang/protobuf/proto    0.066s

The failing code is from "github.com/golang/protobuf/proto":

// RegisterType is called from generated code and maps from the fully qualified
// proto name to the type (pointer to struct) of the protocol buffer.
func RegisterType(x Message, name string) {
    if _, ok := protoTypes[name]; ok {
        // TODO: Some day, make this a panic.
        log.Printf("proto: duplicate proto type registered: %s", name)
        return
    }
    t := reflect.TypeOf(x)
    protoTypes[name] = t // <---- crashing here!
    revProtoTypes[t] = name
}

protoTypes is initialized in a global var block:

// A registry of all linked message types.
// The string is a fully-qualified proto name ("pkg.Message").
var (
    protoTypes    = make(map[string]reflect.Type)
    revProtoTypes = make(map[reflect.Type]string)
)

It is called from an init function in "github.com/golang/protobuf/ptypes/any/":

func init() {
    proto.RegisterType((*Any)(nil), "google.protobuf.Any")
}

So it looks like this init function is being called before that of "github.com/golang/protobuf/proto", which seems reasonably impossible (and indeed, I can't reproduce this in a trivial test case).

[mwhudson]

Paging @ianlancetaylor

[mwhudson]

Ok, I have some small idea what is happening here. gccgo appears to have a concept of a "package priority" where if package A that imports package B then package B will have a higher priority than A. In fact a package's priority is defined to be one greater than that of all the packages it imports.

Init functions are then called in low to high priority order.

Buuuuuut when testing a package there are two versions of a package in play, the "usual" one and the "test" one, and they don't necessarily have the same priorities:

$ go test -work -c -a -compiler gccgo github.com/golang/protobuf/proto
WORK=/tmp/go-build010939228
$ WORK=/tmp/go-build010939228
$ objcopy  --dump-section .go_export=>(cat) $WORK/github.com/golang/protobuf/libproto.a | grep priority
priority 8;
$ objcopy  --dump-section .go_export=>(cat) $WORK/github.com/golang/protobuf/proto/_test/github.com/golang/protobuf/libproto.a | grep priority
priority 9;

(as it happens, package "testing" has priority 8).

So when package A's external tests depend on a package B that depends on package A, package B's priority is set based on the non-testy version of A, which means its priority can be lower than that of the testy version of A. Hilarity ensues.

[mwhudson]

Ping? My github account got flagged for a while so it's possible notifications for this issue didn't go out.

[ianlancetaylor]

I saw your comment. I thought the gccgo code handled this priority inversion somehow but I do not remember how. I haven't looked into the problem yet--I'm looking at the gc 1.7 release and internal stuff before I return to working on gccgo.

OK, I took a very quick look. The comment for Package::set_priority says

// Set the priority.  We may see multiple priorities for an imported
// package; we want to use the largest one.

So why isn't that working?

I also see this comment, earlier, so I think that this did work at one time:

      // If a test of package P1, built as part of package P1,
      // imports package P2, and P2 imports P1 (perhaps
      // indirectly), then we will see the same import name with
      // different import priorities.  That is OK, so don't give
      // an error about it.

[mwhudson]

I saw your comment. I thought the gccgo code handled this priority inversion somehow but I do not remember how. I haven't looked into the problem yet--I'm looking at the gc 1.7 release and internal stuff before I return to working on gccgo.

Ah, sorry for the noise. Sensible priorities!

OK, I took a very quick look. The comment for Package::set_priority says

// Set the priority. We may see multiple priorities for an imported
// package; we want to use the largest one.
So why isn't that working?

Because it's not the priority of the package being tested (let's call it A) that's the problem, it's the priority of some other package (say B) that (a) depends on A (b) is depended on by A's external tests that's the problem. B's priority was fixed when it was built, without knowing that A's priority can change in this situation. The only approach to fixing this that I can think of is to walk the import graph at link time, not compute some depth measurement at compile time (or take imports in internal test files into account at compile time for priority purposes, but that sounds bonkers).

I also see this comment, earlier, so I think that this did work at one time:

  // If a test of package P1, built as part of package P1,
  // imports package P2, and P2 imports P1 (perhaps
  // indirectly), then we will see the same import name with
  // different import priorities.  That is OK, so don't give
  // an error about it.

I think I probably rendered this bit unecessary in https://go-review.googlesource.com/#/c/21692/ -- the link only sees one version of the package now.

[thanm]

Created a reduced test case.

[thanm]

Updated test case is at
files_rev2.zip

[thanm]

The key thing triggering the bug is the fact that the protobuf dir contains both internal and external tests. This in turn creates a scenario where you have two copies of a single package, each with different priorities, which is where things go wobbly.

I created a smaller test case to encapsulate the bug:

$GOPATH/issue15738/foo/foo.go:
  package foo
  var globmap = make(map[string]string)
  func Modmap(s1 string, s2 string) {
    globmap[s1] = s2
  }
  func AddOne(x int) int {
    return x + 1
  }
  func addThree(x int) int {
    return x + 3
  }

$GOPATH/issue15738/bar/bar.go:
  package bar
  import "issue15738/foo"
  func init() {
    foo.Modmap("a", "b")
  }
  func AddTwo(x int) int {
    return x + 2
  }

$GOPATH/issue15738/foo/foo_internal_test.go:
  package foo
  import "testing"
  func TestLocal(t *testing.T) {
    if addThree(0) != 3 {
       t.Errorf("fail")
    }
  }

$GOPATH/issue15738/foo/foo_external_test.go:
  package foo_external_test
  import (
    "issue15738/bar"
    "issue15738/foo"
    "testing"
  )
  func TestExternal(t *testing.T) {
    if foo.AddOne(bar.AddTwo(0)) != 3 {
      t.Errorf("fail")
    }
  }

During the compilation phase of "go test", the builder first builds an object containing the internal test and foo, e.g.

gccgo ... -c -o foo_it1.o ./foo.go ./foo_internal_test.go
ar rc libfoo_itest.a foo_it1.o

foo_internal_test.go imports only "testing", but "testing" has a priority of 8, this means that the priority for foo_it1.o is set to 9.

Next it reruns the foo compile without the internal test:

gccgo ... -c -o foo.o ./foo.go
ar rc libfoo.a foo.o

Since foo.go has no imports, foo.o gets a priority of 1. Moving on:

gccgo ... -c -o bar.o ./bar.go
ar rc libbar.a bar.o

Since bar imports only foo, bar.o is assigned a priority of 2.
Next the external test gets compiled:

gccgo ... -c -o foo_ext.o ./foo_external_test.go
ar rc libfoo_etest.a foo_ext.o

Here foo_external_test.go imports foo, bar, and testing. Again, since testing has a priority of 8, this means that the priority for foo_it1.o is set to 9.

The final link looks like:

gccgo -o foo.test testmain.o -Wl,-( -Wl,--whole-archive
libfoo_itest.a libfoo_etest.a libbar.a -Wl,--no-whole-archive -Wl,-)

The problem here is that we built "libbar.a" against the vanilla version of the foo package (libfoo.a), which contained a version of foo that had priority 1. However when we do to do the final link, we're using a different copy of foo (contained in libfoo_itest.a) that has priority 9. As a result the bar initializer runs before the foo initializer, which is incorrect.

My instinct is to try to fix this in "go test" as opposed to trying to change gccgo -- gccgo's priority scheme works fine, it's simply the fact that we're compiling the "bar" package based on one version of "foo", then in the final link we link in a different verison of "foo". I will spend some time looking over the "go build" and "go test" srcs.

[thanm]

Possible solutions:

1. change the build scheme for "go test" to be single-build-per-package

   When compiling a package P, don't do two builds one with the internal test code and one without the internal test code -- instead just produce a single archive library for P that contains the package itself plus the test code, then link other packages against that archive.

   This scheme has the disadvantage that we could have naming collisions or conflicts as a result of things exported from the test code; for this reason I think it's pretty much a non-starter. Example:

   p.go:
   package p
   func PFunc() { .. }

   p_test.go:
   package p
   func Helper() { .. }

If some other package "q" imports "p" with a dot import, this will introduce "Helper" into the file scope for "q" (meaning that it could collide with some other function).

1. Create a command line option for gccgo that allows adding priority skew

   Since the problem boils down to the fact that importing "testing" bumps up the priority of a given package by a constant (somewhere between 0 and 8), then we could add a new "-fpriority-skew=N" option that tells gccgo to start with an initial priority of N (not 1) for any package that it is compiling. This option could then be set by the "go test" builder.

   Another possibility along these lines would be something like "-finject-import=XYZ", which would tell the compiler "Hey, I didn't import package XYZ in my source code, but please act as if I did". This would have the effect of bumping up the priority of any package K dependent on package J that happens to have internal test code.

   These proposals seem hacky (any solution that involves adding a new obscure command line option is automatically distasteful in my mind) but it does seem as though it would take care of the issue.

   A second drawback here is that "go test" has smarts to detect cases where packages have already been compiled and don't need to be rebuilt -- this would need to be defeated somehow in order for this scheme to work (which could in some cases greatly increase the time needed to run tests).

2. Silently inject ref to testing package during "go build/test" compilation

   Here the idea is that for any random package P we have to compile as part of the build process for "go test", we generate a tiny code fragment that looks like

   package P
   import "testing"

   and then tack that onto the end of the gccgo command line. This has an effect similar to "-finject-import=XYZ" described above, but doesn't require any compiler changes or new command line options.

   Same drawback here as option net: LookupHost is returning odd values and crashing net tests #2 above with respect to staleness check -- we can't reuse the existing installed copy of package K if K depend on package J that has internal test code.

3. Change gccgo to initialize testing first

   Since this bug revolves around the "testing" package, one avenue for fixing it would be to add special-case code to gccgo relating to that package. Specifically:

   A] when importing the package "testing", we ignore the priority it exports (currently 8) and assign it a priority of zero (at least for the purposes of computing the priority of the package we're actually compiling)

   B] at the point when we're emitting init calls (for the main package), if main imports the testing package (which testmain.go does) then we insure that the call to initialize "testing" goes first, before any other package.

   Taking the "testing" package out of the equation when it comes to priorities means that the special "package plus internal test" blob that we create will have the same priority as the regular package.

[ianlancetaylor]

There is another reason your first suggestion (single build per package) won't work: in some cases the _test.go files have init functions that set things up for the tests, but will break things for the real package. For example, the time package does this--see the init function in time/internal_test.go.

If you force the testing package to have a priority of zero, I'm a little worried about correct initialization of the testing package itself vs. packages that the testing package imports. The testing package doesn't currently have any init functions but it does have some flags. I think it's important that the flag package be initialized before the testing package's initialization function is run.

The package import graph is a DAG, and we need to run the initialization functions from the leafs up. We actually have all the information we need to build that DAG, by reading the import statements in the export data. Maybe we should just build the DAG and use it to sort the initialization functions, rather than trying to use the priority as an overly-simple workaround.

[thanm]

OK, sure enough, your concern is indeed valid -- I prototyped the priority zero approach(which does fix my testcase) but it fails the package test for the predefined "strconv" package (presumably because the init functions are not being run in the right order). I'll look into rewriting the init order walk using the export data, that sounds like a more robust option.

[thanm]

I have a tentative fix for this problem in https://go-review.googlesource.com/25301.

One thing that I ran into is that you can have pseudo-cycles in the import graph when building tests for go core packages. For example, $GOROOT/src/regexp/onepass_test.go is part of package "regexp" and also imports "testing", but "testing" imports "regexp" (hmm). My code handles this, but I am wondering if I need to worry about cycles elsewhere as well.

The other weird thing is that for gccgo there are references to the "internal/race" package but as far as I can tell there is no exports file for that package produced as part of the regular build:

$ dump-gccgo-obj-goexports.py /s/gcc-trunk/cross/lib64/go/7.0.0/x86_64-pc-linux-gnu/sync.gox | head
v1;
package sync;
pkgpath sync;
priority 2;
import race internal/race "internal/race";
import runtime runtime "runtime";
import atomic sync/atomic "sync/atomic";
import unsafe unsafe "unsafe";
init sync sync..import 2 runtime runtime..import 1;
$ find /s/gcc-trunk/cross/lib64/go/7.0.0/x86_64-pc-linux-gnu -name "race.gox" -print
$

I have a special case to handle this at the moment but I'm not sure what the best long-term solution should be.

[gopherbot]

CL https://golang.org/cl/25301 mentions this issue.

[ianlancetaylor]

In your build directory you should see a file TARGET/libgo/internal/race.gox, which is the export file for the internal/race package. That file is not installed, because subdirectories of internal may not be imported from outside the standard library (see https://golang.org/cmd/go/#hdr-Internal_Directories), but race.gox is built and may be imported by packages in the standard library.

[ianlancetaylor]

The testing package can have cycles as you describe, which for the gc toolchain is handled specially by go test. I don't think you need to worry about cycles in any other case.

From cmd/go/test.go:

    // The ptest package needs to be importable under the
    // same import path that p has, but we cannot put it in
    // the usual place in the temporary tree, because then
    // other tests will see it as the real package.
    // Instead we make a _test directory under the import path
    // and then repeat the import path there. We tell the
    // compiler and linker to look in that _test directory first.
    //
    // That is, if the package under test is unicode/utf8,
    // then the normal place to write the package archive is
    // $WORK/unicode/utf8.a, but we write the test package archive to
    // $WORK/unicode/utf8/_test/unicode/utf8.a.
    // We write the external test package archive to
    // $WORK/unicode/utf8/_test/unicode/utf8_test.a.

[thanm]

Hmm. My approach seems to work well in the cases where all of the exports data is available, but falls down in cases where something includes packages that indirectly include an internal package like "golang_org/x/net/http2/hpack" -- hpack has a long list of other imported packages, and it's difficult to stitch/interleave those init calls into the dependence graph without being able to read its export data.

Alternate proposal: change the libgo makefile to export *.gox files for hidden/internal packages (so that we can have a single mechanism for computing init priority), but mark them in some way so that the compiler can refuse to allow things in them to be imported. For example, the ".go_export" section could be renamed ".go_export_h", or something other such scheme.