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: build -gcflags all="-N -l" nil pointer dereference #46386

Closed
MJDrgn opened this issue May 26, 2021 · 11 comments
Closed

cmd/compile: build -gcflags all="-N -l" nil pointer dereference #46386

MJDrgn opened this issue May 26, 2021 · 11 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@MJDrgn
Copy link

MJDrgn commented May 26, 2021

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

Reproduced on two developer machines, one Ubuntu and one Mac.

$ go version
go version go1.16.4 linux/amd64

$ go version
go version go1.16 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

Two outputs, one Ubuntu and one Mac:

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/xxxxx.xxxxx/.cache/go-build"
GOENV="/home/xxxxx.xxxxx/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/xxxxx.xxxxx/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/xxxxx.xxxxx/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.16"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.16/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.4"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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-build1372741891=/tmp/go-build -gno-record-gcc-switches"

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/matt.johnston/Library/Caches/go-build"
GOENV="/Users/matt.johnston/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/matt.johnston/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/matt.johnston/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/yh/jp8vznl910d4w8kb8gs2v4wc0000gq/T/go-build1600909367=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Archive.zip

I've attached as small of an example as I can get - the error seems to be extremely picky to whitespace - sometimes adding or removing a comment breaks things. I cannot reproduce the error with all code in one file.
The go:noinline are part of debugging what turned out to be a display glitch in GoLand's debugger, trying to create a minimal example in order to reproduce that bug, however now it just breaks the Go compiler.
The gcflags in the following example are what GoLand uses when producing an executable for debugging.

go build -gcflags all="-N -l" *.go

What did you expect to see?

Successful compilation

What did you see instead?

$ go build -gcflags all="-N -l" *.go
# command-line-arguments
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x16f6a7e]

goroutine 1 [running]:
cmd/compile/internal/types.(*Fields).Index(...)
	/usr/local/go/src/cmd/compile/internal/types/type.go:422
cmd/compile/internal/gc.typesByString.Less(0xc000878800, 0x2f, 0x2f, 0x1b, 0x26, 0x1)
	/usr/local/go/src/cmd/compile/internal/gc/reflect.go:1677 +0x19e
sort.doPivot(0x19bfd98, 0xc0005a10c8, 0x1b, 0x2f, 0xa, 0x1b)
	/usr/local/go/src/sort/sort.go:136 +0x4cd
sort.quickSort(0x19bfd98, 0xc0005a10c8, 0x1b, 0x2f, 0xa)
	/usr/local/go/src/sort/sort.go:203 +0x97
sort.Sort(0x19bfd98, 0xc0005a10c8)
	/usr/local/go/src/sort/sort.go:231 +0x76
cmd/compile/internal/gc.dumpsignats()
	/usr/local/go/src/cmd/compile/internal/gc/reflect.go:1548 +0x2b2
cmd/compile/internal/gc.dumpdata()
	/usr/local/go/src/cmd/compile/internal/gc/obj.go:124 +0x8a
cmd/compile/internal/gc.Main(0x18c4978)
	/usr/local/go/src/cmd/compile/internal/gc/main.go:803 +0x382b
main.main()
	/usr/local/go/src/cmd/compile/main.go:52 +0xb1
@ALTree
Copy link
Member

ALTree commented May 26, 2021

I can reproduce this even without -N -l, but it's nondeterministic. After a while it stops crashing but after a go clean -cache it crashes again.

The good news is that it never crashes the tip compiler (or at least is significantly less frequent there, I could never make it crash).

Can you reproduce this on tip?

@ALTree ALTree changed the title go build -gcflags all="-N -l" nil pointer dereference cmd/compile: build -gcflags all="-N -l" nil pointer dereference May 26, 2021
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 26, 2021
@randall77
Copy link
Contributor

Looks like there is an interface type that the compiler thinks is an empty interface, but still has a method. That looks bad.

The type is an empty interface with a HookImpl method. Which doesn't make a whole lot of sense, as that's an embedded interface, not a method.

It's basically

type T interface {}
type U interface { T } // This type has a "method"

(But that by itself doesn't reproduce the problem - there's something more to it.)

I'm pretty sure this is not a problem at tip. Looks like @danscales fixed it (probably by accident) at https://go-review.googlesource.com/c/go/+/307209 The empty interface check changed from:

func (t *Type) IsEmptyInterface() bool {
	return t.IsInterface() && t.NumFields() == 0
}

to

func (t *Type) IsEmptyInterface() bool {
	return t.IsInterface() && t.AllMethods().Len() == 0
}

I think "fixed it" is probably too strong - maybe it is just papering over an invariant that should hold but doesn't.

@mdempsky

@cuonglm
Copy link
Member

cuonglm commented May 26, 2021

Shorter reproducible:

package main

type I interface {
	M() interface{}
}

type S1 struct{}

func (s *S1) M() interface{} {
	return nil
}

type EI interface{}

type S struct{}

func (S) M(as interface{ I }) {}

func f() interface{ EI } {
	return &S{}
}

func main() {
	var i interface{ I }
	(&S{}).M(i)
}

@randall77
Copy link
Contributor

If I add this code to cmd/compile/internal/types/type.go:IsEmptyInterface

	if t.IsInterface() && t.NumFields() == 0 && t.Methods().Len() > 0 {
		panic("phantom method")
	}

Then all.bash almost passes. It dies with test/fixedbugs/bug419.go, which is very close to this, which also triggers that assertion:

type T interface{}
type U interface{ T }

@cuonglm
Copy link
Member

cuonglm commented May 26, 2021

@randall77 and still problem at tip

@mdempsky
Copy link
Member

@cuonglm I'm not able to reproduce with your test case. Can you give more details on repro'ing? I'm using Go 1.16.3, and trying to run go tool compile -N -l x.go. I ran it in a loop for a while and didn't get any failures.

A bit more broadly, typecheck has certainly had issues with interface cycles in the past, and it sounds like this isn't a regression in behavior. If we can get a reliable repro, that would be great. But for the time being, I think it makes sense to leave this for Go 1.18 and revisit iff it's still an issue after switching to types2.

@mdempsky mdempsky added this to the Go1.18 milestone May 26, 2021
@cuonglm
Copy link
Member

cuonglm commented May 26, 2021

@mdempsky you have to use go build -gcflags="-N", or use go build -x for how to use go tool compile.

Without -N, interface { EI } in:

func f() interface{ EI } {
	return &S{}
}

isn't appeared in signatslice.

@mdempsky
Copy link
Member

@cuonglm I see, thanks. I can repro with go build instead of go tool compile.

@cuonglm
Copy link
Member

cuonglm commented May 28, 2021

@mdempsky problem also happens with types2.

With go version devel go1.17-22f5ece3b1 Thu May 27 23:54:29 2021 +0000 darwin/arm64, I can reproduce with:

go tool compile -G=3 -c 1 -p main main.go

Interesting that using -c 2 (or any higher than 1) will make it non-deterministic, sometimes it failed, sometimes it succeeded. It seems to me that the order that we put the type to signatset to generate runtime type descriptors is important.

@gopherbot
Copy link

Change https://golang.org/cl/323649 mentions this issue: [dev.typeparams] cmd/compile: use t.AllMethods when sorting typesByString

@gopherbot
Copy link

Change https://golang.org/cl/326029 mentions this issue: cmd/compile: prevent duplicated works in WriteRuntimeTypes

gopherbot pushed a commit that referenced this issue Aug 19, 2021
While processing signatset, the entry is deleted immediately after being
pushed to signatslice. Then calling writeType may add the same type
to signatset again. That would add more works, though not a big impact
to the performace, since when writeType is guarded by s.Siggen() check.

Instead, we should keep the entry in signatset, so written type will
never be added again.

This change does not affect compiler performace, but help debugging
issue like one in #46386 easier.

Change-Id: Iddafe773885fa21cb7003ba27ddf9554fc3f297d
Reviewed-on: https://go-review.googlesource.com/c/go/+/326029
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@golang golang locked and limited conversation to collaborators Jun 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants