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: internal compiler error: no function definition for [0xc42181a5a0] #23701

Closed
tamird opened this issue Feb 5, 2018 · 24 comments

Comments

Projects
None yet
10 participants
@tamird
Copy link
Contributor

commented Feb 5, 2018

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

go version go1.10rc1 linux/amd64

Does this issue reproduce with the latest release?

This affects the release candidate.

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/tduberstein/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/tduberstein/local/go"
GORACE=""
GOROOT="/home/tduberstein/local/go1.10"
GOTMPDIR=""
GOTOOLDIR="/home/tduberstein/local/go1.10/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build297022879=/tmp/go-build -gno-record-gcc-switches"

What did you do?

go get -d github.com/cockroachdb/cockroach
cd $(go env GOPATH)/src/github.com/cockroachdb/cockroach
make build PKG=./pkg/sql/pgwire

What did you expect to see?

Tests run.

What did you see instead?

# github.com/cockroachdb/cockroach/pkg/sql/pgwire_test
pkg/sql/pgwire/pgwire_test.go:317:45: internal compiler error: no function definition for [0xc424862960] FUNC-method(*pgwire.Server) func() []context.CancelFunc


Please file a bug report including a short program that triggers the error.
https://golang.org/issue/new
FAIL    github.com/cockroachdb/cockroach/pkg/sql/pgwire [build failed]
make: *** [Makefile:773: test] Error 2
@mvdan

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

I assume this does not affect 1.9?

@mvdan mvdan added this to the Go1.10 milestone Feb 5, 2018

@tamird

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2018

Correct.

@mvdan

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

Have you by any chance been able to come up with a small program/package to reproduce the same crash? That project is large and a bit complex to build, so it would help the compiler developers.

@tamird

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2018

@dgryski

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

Note that adding methods in a _test.go file is #6204

@tamird

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2018

@dgryski that's incorrect - in this case we're not going across packages, and this pattern works in Go 1.9.

@remyoudompheng

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

Same issue on my side with the following minimal example:

$ cat a/a.go
package a

import "b"

type Type struct {}

func (*Type) M() b.T { return 0 }
$ cat b/export_test.go
package b

func (*T) Method() *T { return nil }
$ cat b/b.go
package b

type T int

type I interface { M() T }

$ cat b/b_test.go
package b_test

import (
"testing"
"a"
. "b"
)

func TestBroken(*testing.T) {
x := new(T)
x.Method()
_ = new(a.Type)
}

@tamird

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2018

a/a.go

package a

import "b"

type Type struct {}

func (*Type) M() b.T { return 0 }

b/export_test.go

package b

func (*T) Method() *T { return nil }

b/b.go

package b

type T int

type I interface { M() T }

b/b_test.go

package b_test

import (
  "testing"
  "a"
  . "b"
)

func TestBroken(*testing.T) {
  x := new(T)
  x.Method()
  _ = new(a.Type)
}

Ah, I guess a second package is required.

@dgryski

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

Bisecting seems to point at c8e9fd5 , which reverts a revert of f0b3626

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

Issue #6204 says that cmd/go is supposed to reject adding methods in a _test.go file, but that it currently does not. And indeed the code in #6204 does compile today. I don't understand how this issue differs from #6204.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

Oh, never mind, I do get the same error with the test case in #6204 when I use go test p2.

But I still don't see why this is different than #6204.

@dgryski

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

@ianlancetaylor I think the problem is 1.10 breaks an unsupported feature.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

I think it's OK to break this in Go 1.10 but we need a better error message.

@tamird

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2018

Going to be incovenient for CockroachDB.

$ find . -name helpers_test.go
./pkg/storage/idalloc/helpers_test.go
./pkg/storage/helpers_test.go
./pkg/sql/jobs/helpers_test.go
./pkg/sql/pgwire/helpers_test.go
./pkg/sql/helpers_test.go

/cc @bdarnell @petermattis @spencerkimball

@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

I still think that it's OK to reject methods in tests if we end up needing to do that, but that doesn't look fundamental here. This looks to me like the go command is not rebuilding dependencies of test packages correctly. There was a related bug in this code a few weeks ago, and this report looks like a variant of that. It should be possible to fix this new bug and leave the disposition of #6204 for a less hurried time. I'll take a look at this for 1.10rc2, which we are thinking about doing on Wednesday.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 6, 2018

Change https://golang.org/cl/92215 mentions this issue: cmd/go: rebuild as needed for tests of packages that add methods

@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2018

This was easy to make work after the simplifications done for the build cache: basically a 1/2-line change to cmd/go, simplifying an if condition. Given that we can now rebuild perfectly, I see no reason to go out of our way to disallow adding methods in test files, even though I might wish I had done that 5+ years ago.

@dgryski

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2018

Should the release notes be updated to mention adding methods in a test file is now "officially" supported?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2018

Before we put it in the release notes we need to put it in the docs. Actually I'm not sure what the release notes would say. "This thing that used to work still works." Personally I think this is doc-worthy but not release-notes-worthy.

@gopherbot gopherbot closed this in 85bdd05 Feb 6, 2018

bboozzoo added a commit to bboozzoo/snapd that referenced this issue Mar 14, 2018

overlord/ifacestate: workaround broken go vet in go 1.10
Workaround go vet 'internal compiler error' when methods are defined in test
files and cross package imports are used. The original ticket is:
  golang/go#23701

Although it is supposed to be fixed in 1.10 it does not appear to be so.

Error reported by go vet:

  ./ifacestate_test.go:120:29: internal compiler error: no function definition for [0xc420a2fc80] FUNC-method(*ifacestate.InterfaceManager) func()

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo added a commit to bboozzoo/snapd that referenced this issue Mar 14, 2018

overlord/devicestate: workaround broken go vet in go 1.10
Workaround go vet 'internal compiler error' when methods are defined in test
files and cross package imports are used. The original ticket is:
  golang/go#23701

Although it is supposed to be fixed in 1.10 it does not appear to be so.

Error reported by go vet:

./devicestate_test.go:403:38: internal compiler error: no function definition for [0xc4212363c0] FUNC-method(*devicestate.DeviceManager) func() asserts.KeypairManager

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@bboozzoo

This comment has been minimized.

Copy link

commented Mar 14, 2018

The test case provided in #23701 (comment) can still be reproduced with go1.10 linux/amd64.

With master (go version devel +d32018a500 Wed Mar 14 08:36:15 2018 +0000 linux/amd64) I get this:

# b_test
<unknown line number>: internal compiler error: no function definition for [0xc0003706c0] FUNC-method(*b.T) func() *b.T


goroutine 1 [running]:
runtime/debug.Stack(0x0, 0x0, 0x0)
        /home/maciek/code/go/go-git/src/runtime/debug/stack.go:24 +0xa7
cmd/compile/internal/gc.Fatalf(0xc50b01, 0x24, 0xc0003bb498, 0x2, 0x2)
        /home/maciek/code/go/go-git/src/cmd/compile/internal/gc/subr.go:182 +0x1f7
cmd/compile/internal/gc.(*hairyVisitor).visit(0xc0003bb650, 0xc000412500, 0xc000074000)
        /home/maciek/code/go/go-git/src/cmd/compile/internal/gc/inl.go:291 +0x7b1
cmd/compile/internal/gc.(*hairyVisitor).visitList(0xc0003bb650, 0xc0004013c0, 0xc000407860)
        /home/maciek/code/go/go-git/src/cmd/compile/internal/gc/inl.go:238 +0x74
cmd/compile/internal/gc.caninl(0xc000001200)
        /home/maciek/code/go/go-git/src/cmd/compile/internal/gc/inl.go:160 +0x186
cmd/compile/internal/gc.Main.func6(0xc00000d7c8, 0x1, 0x1, 0xc000324100)
        /home/maciek/code/go/go-git/src/cmd/compile/internal/gc/main.go:557 +0x164
cmd/compile/internal/gc.(*bottomUpVisitor).visit(0xc0003bb810, 0xc000001200, 0x1)
        /home/maciek/code/go/go-git/src/cmd/compile/internal/gc/esc.go:107 +0x230
cmd/compile/internal/gc.visitBottomUp(0xc00000d7a8, 0x1, 0x1, 0xc5bcb8)
        /home/maciek/code/go/go-git/src/cmd/compile/internal/gc/esc.go:63 +0x95
cmd/compile/internal/gc.Main(0xc5bb98)
        /home/maciek/code/go/go-git/src/cmd/compile/internal/gc/main.go:554 +0x2e05
main.main()
        /home/maciek/code/go/go-git/src/cmd/compile/main.go:49 +0x96

mvo5 added a commit to mvo5/snappy that referenced this issue Mar 15, 2018

overlord/ifacestate: workaround broken go vet in go 1.10
Workaround go vet 'internal compiler error' when methods are defined in test
files and cross package imports are used. The original ticket is:
  golang/go#23701

Although it is supposed to be fixed in 1.10 it does not appear to be so.

Error reported by go vet:

  ./ifacestate_test.go:120:29: internal compiler error: no function definition for [0xc420a2fc80] FUNC-method(*ifacestate.InterfaceManager) func()

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

mvo5 added a commit to mvo5/snappy that referenced this issue Mar 15, 2018

overlord/devicestate: workaround broken go vet in go 1.10
Workaround go vet 'internal compiler error' when methods are defined in test
files and cross package imports are used. The original ticket is:
  golang/go#23701

Although it is supposed to be fixed in 1.10 it does not appear to be so.

Error reported by go vet:

./devicestate_test.go:403:38: internal compiler error: no function definition for [0xc4212363c0] FUNC-method(*devicestate.DeviceManager) func() asserts.KeypairManager

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>
@gopherbot

This comment has been minimized.

Copy link

commented Apr 3, 2018

Change https://golang.org/cl/104315 mentions this issue: cmd/go: rebuild as needed when vetting test packages

benesch added a commit to benesch/cockroach that referenced this issue Apr 3, 2018

lint: use 'go vet' instead of 'go tool vet'
'go tool vet' uses out-of-date/incorrect type information, so, since Go
1.10, the Go team recommends using 'go vet' instead. Take them up on
their recommendation.

This unfortunately trips across an ICE (golang/go#23701), but I opened a
CL to fix that: https://go-review.googlesource.com/c/go/+/104315. For
now, just ignore the ICE.

Release note: None

@gopherbot gopherbot closed this in 804d032 Apr 4, 2018

@rsc rsc modified the milestones: Go1.10, Go1.10.2 Apr 4, 2018

@rsc rsc reopened this Apr 4, 2018

craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Apr 10, 2018

Merge #24454 #24573 #24577 #24605 #24619
24454: lint: various fixes r=nvanbenschoten a=benesch

Fill in a deep, deep linter rabbit hole. In order for #24104 to pass the linter, we need to run `go vet ./pkg/...` instead of `cd pkg && go tool vet .` so that intentionally-malformatted Go files in a testdata directory are ignored. That required reorganizing the lint rules. Then it required fixing a `go vet` violation that was previously missed, probably because `go tool vet` was using incorrect type information. It also required a workaround for an internal compiler error (golang/go#23701).

@nvanbenschoten, any chance I can toss this your way?

24573: distsql: Refactor aggregator into a state machine r=couchand a=abhimadan

This lays the groundwork for extending the aggregator to stream results
if some of the group columns have an ordering.

Release note: None

----

This is the first commit from #24113, extracted into its own PR.

24577: distsql: improve hashjoin performance r=jordanlewis a=jordanlewis

- add a benchmark for the spilling hashjoiner
- reset the disk iterator rather than creating a new one every probeRow
- fix small flushing bug that caused a flush on an empty batch

```
name                                old time/op    new time/op    delta
HashJoiner/spill=true/rows=256-8      11.7ms ± 3%     2.2ms ± 3%   -81.43%  (p=0.000 n=10+8)
HashJoiner/spill=true/rows=4096-8     89.9ms ± 2%    18.4ms ± 1%   -79.58%  (p=0.000 n=10+9)
HashJoiner/spill=true/rows=65536-8     1.02s ± 2%     0.28s ± 2%   -72.16%  (p=0.000 n=9+10)

name                                old speed      new speed      delta
HashJoiner/spill=true/rows=256-8     175kB/s ± 3%   941kB/s ± 3%  +437.86%  (p=0.000 n=10+8)
HashJoiner/spill=true/rows=4096-8    364kB/s ± 2%  1780kB/s ± 2%  +389.01%  (p=0.000 n=10+10)
HashJoiner/spill=true/rows=65536-8   517kB/s ± 1%  1850kB/s ± 2%  +258.06%  (p=0.000 n=9+10)

name                                old alloc/op   new alloc/op   delta
HashJoiner/spill=true/rows=256-8       500kB ± 0%      46kB ± 0%   -90.82%  (p=0.000 n=9+9)
HashJoiner/spill=true/rows=4096-8     8.04MB ± 0%    0.79MB ± 0%   -90.16%  (p=0.000 n=9+9)
HashJoiner/spill=true/rows=65536-8     129MB ± 0%      13MB ± 0%   -90.09%  (p=0.000 n=10+9)

name                                old allocs/op  new allocs/op  delta
HashJoiner/spill=true/rows=256-8       2.88k ± 0%     0.83k ± 0%   -71.10%  (p=0.000 n=10+10)
HashJoiner/spill=true/rows=4096-8      45.4k ± 0%     12.7k ± 0%   -72.13%  (p=0.000 n=10+10)
HashJoiner/spill=true/rows=65536-8      726k ± 0%      202k ± 0%   -72.19%  (p=0.000 n=10+10)
```


24605: stats: invalidate table statistic caches through gossip r=RaduBerinde a=RaduBerinde

This change makes the TableStatisticsCache stay up-to-date
automatically: whenever we compute a stat, we gossip a "table stat
added" key for that table, which triggers an invalidation in the
cache.

In the current form, this will add a key per table when we enable
statistics automatically. A possible improvement would be to set a TTL
on these keys and change the cache to always invalidate very old
entries automatically.

Fixes #24585.

Release note: None

24619: storage: remove MVCCStats SpanSet declaration and assertion r=nvanbenschoten a=nvanbenschoten

See #22317 (comment).

Release note: None

Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>
Co-authored-by: Abhishek Madan <abhishek@cockroachlabs.com>
Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>

@FiloSottile FiloSottile modified the milestones: Go1.10.2, Go1.11 Apr 24, 2018

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

@gopherbot please open a 1.10 backport tracking issue. We'll need to cherry-pick both CL 92215 by @rsc and CL 104315 by @benesch.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 24, 2018

Backport issue(s) opened: #25039 (for 1.10).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

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