Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: duplicate anonymous interface declarations cause non-determinism #30202

Open
jellevandenhooff opened this issue Feb 13, 2019 · 11 comments

Comments

Projects
None yet
8 participants
@jellevandenhooff
Copy link

commented Feb 13, 2019

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

$ go 1.11.5

Does this issue reproduce with the latest release?

This happens on both go 1.11.5 and go 1.12rc1

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/jelle/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jelle/hack/backend/go"
GOPROXY=""
GORACE=""
GOROOT="/Users/jelle/hack/backend/opt/go1.11.5"
GOTMPDIR=""
GOTOOLDIR="/Users/jelle/hack/backend/opt/go1.11.5/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/7n/4n_h9yg109b8kkpk1974y86h0000gp/T/go-build173276175=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Similar to #27013 but with the addition of concurrency, the go compiler produces a non-deterministic small.a output when running on the following minimized test program.

package small

func A(x interface {
        X() int
}) int {
        return x.X()
}

func B(x interface {
        X() int
}) int {
        return x.X()
}

To demonstrate:

for i in {1..20}; do
  go tool compile -c 2 -p small -linkobj small.a small.go &&
  md5 small.a
done | sort | uniq -c

What did you expect to see?

The go compiler produces the same output each time it is run, since it receives the same inputs.

The little script above should only see one hash from the 20 small.a files.

What did you see instead?

The go compiler produces non-deterministic output

  12 MD5 (small.a) = bc3a55c714f25041576fe53690c16732
   8 MD5 (small.a) = fae691f5253d0dcb5991b1689c3c6006

Notes

This issue is related to #27013. The fix for that issue (https://go-review.googlesource.com/c/go/+/129515/) does not work for concurrent builds. It imposes an ordering
based on invocation order of addsignat in src/cmd/compile/internal/gc.go, but that invocation order is non-deterministic.

This problems occurs in the wild in https://github.com/gogo/protobuf/blob/master/types/duration_gogo.go.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 13, 2019

Change https://golang.org/cl/162240 mentions this issue: cmd/compile: make duplicate anonymous interface output deterministic

@odeke-em

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Thank you for reporting this issue @jellevandenhooff!

I shall page some folks about it @josharian @mdempsky @randall77

@jellevandenhooff

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

I have a bit of a dream as a follow-up... It would be fantastic if there was some job scraping Github for go packages, build them a couple times, and find packages exhibiting non-deterministic builds.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

I did that in Go 1.9, as part of thoroughly testing concurrent compilation. :)

@cuonglm

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@josharian @mdempsky @randall77 sorry to bother you in this issue, but related to #29612

Should 2 anonymous interface in different package go to the same bucket in itable?

@randall77

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@Gnouc I'm not sure entirely what you are asking.

Are you talking about the runtime itabTable in runtime/iface.go? I'm going to assume you are.

That table is a hash table. The general rule is that two equal keys must map to the same bucket. Different keys should map to different buckets, but they don't have to. Different keys mapping to the same bucket make the code slower, but not wrong.

So ideally two otherwise identical anonymous interfaces in different packages should map to different buckets, but they don't have to. Looking at the code, it seems like they would map to the same bucket, as we use a hash of the string representation of the interface type. Not ideal, but this case probably doesn't happen often enough to warrant doing anything about it.

@cuonglm

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@randall77 Thanks for the information. Can you take a look at #29612 when you have time.

@josharian

This comment has been minimized.

Copy link
Contributor

commented May 11, 2019

Bisecting #31975, which is a duplicate of this issue, yielded https://go-review.googlesource.com/106796, which was the introduction of the indexed export format. Not a surprise, in retrospect, but good to confirm.

Of note: https://go-review.googlesource.com/c/go/+/170037 added anonymous interfaces to the standard library (internal/oserror), so this now directly causes toolstash-check to have spurious failures.

@andybons

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@randall77 any updates on this?

@josharian

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

@andybons https://golang.org/cl/162240 needs finishing up

@gopherbot

This comment has been minimized.

Copy link

commented Jun 26, 2019

Change https://golang.org/cl/183852 mentions this issue: cmd/compile: make duplicate anonymous interface output deterministic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.