cmd/compile: internal compiler error: Type.Elem UNSAFEPTR

[kirillDanshin]

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

$ go version
go version devel +4091cf972a Sun Mar 31 23:35:35 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

reproducible only on tip

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

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/travis/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/travis/gopath"
GOPROXY=""
GORACE=""
GOROOT="/home/travis/.gimme/versions/go"
GOTMPDIR=""
GOTOOLDIR="/home/travis/.gimme/versions/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build848806544=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I have a pretty old package with some hacks for optimization.
I use some of its features in my web framework.
In travis, I check if the framework pass the tests on tip.
One of the steps in travis script is the go get -v.
When go get tries to precompile that old package, it crashes.

So, one of the files looks like this:

package runtimer

import (
	"unsafe" // #nosec
)

func PtrToString(ptr unsafe.Pointer) string {
	return *(*string)(ptr)
}

func PtrToStringPtr(ptr unsafe.Pointer) *string {
	return (*string)(ptr)
}

func PtrPtrToStringPtr(ptr *unsafe.Pointer) *string {
	return (*string)(*ptr) // <- compiler complains about this line
}

Then I've commented out the last func.

Then the file looks like this:

package runtimer

import (
	"unsafe" // #nosec
)

func PtrToString(ptr unsafe.Pointer) string {
	return *(*string)(ptr)
}

func PtrToStringPtr(ptr unsafe.Pointer) *string {
	return (*string)(ptr) // <- and then it complains about this line
}
/*
func PtrPtrToStringPtr(ptr *unsafe.Pointer) *string {
	return (*string)(*ptr)
}
*/

What did you expect to see?

unsafe.Pointer works as it work two years ago

What did you see instead?

# github.com/gramework/runtimer
../runtimer/utils.go:16:2: internal compiler error: Type.Elem UNSAFEPTR
goroutine 34 [running]:
runtime/debug.Stack(0x1039ae0, 0xc00000e018, 0x0)
	/home/travis/.gimme/versions/go/src/runtime/debug/stack.go:24 +0x9d
cmd/compile/internal/gc.Fatalf(0xe8ef7d, 0xc, 0xc0007c7550, 0x1, 0x1)
	/home/travis/.gimme/versions/go/src/cmd/compile/internal/gc/subr.go:190 +0x292
cmd/compile/internal/types.(*Type).Elem(0xc000061e60, 0xc000538460)
	/home/travis/.gimme/versions/go/src/cmd/compile/internal/types/type.go:801 +0xff
cmd/compile/internal/ssa.(*Func).computeZeroMap(0xc000804000, 0xe12f01)
	/home/travis/.gimme/versions/go/src/cmd/compile/internal/ssa/writebarrier.go:391 +0x101
cmd/compile/internal/ssa.writebarrier(0xc000804000)
	/home/travis/.gimme/versions/go/src/cmd/compile/internal/ssa/writebarrier.go:80 +0x6b
cmd/compile/internal/ssa.Compile(0xc000804000)
	/home/travis/.gimme/versions/go/src/cmd/compile/internal/ssa/compile.go:90 +0x476
cmd/compile/internal/gc.buildssa(0xc0003c9b80, 0x0, 0x0)
	/home/travis/.gimme/versions/go/src/cmd/compile/internal/gc/ssa.go:288 +0xbf3
cmd/compile/internal/gc.compileSSA(0xc0003c9b80, 0x0)
	/home/travis/.gimme/versions/go/src/cmd/compile/internal/gc/pgen.go:297 +0x4d
cmd/compile/internal/gc.compileFunctions.func2(0xc000404de0, 0xc000407200, 0x0)
	/home/travis/.gimme/versions/go/src/cmd/compile/internal/gc/pgen.go:362 +0x49
created by cmd/compile/internal/gc.compileFunctions
	/home/travis/.gimme/versions/go/src/cmd/compile/internal/gc/pgen.go:360 +0x128

[cuonglm]

git bisect points to ca36af2

[cuonglm]

@randall77 A quick fix seems to check if v Type is unsafe pointer:

diff --git a/src/cmd/compile/internal/ssa/writebarrier.go b/src/cmd/compile/internal/ssa/writebarrier.go
index 3c64da20a7..7eedd96885 100644
--- a/src/cmd/compile/internal/ssa/writebarrier.go
+++ b/src/cmd/compile/internal/ssa/writebarrier.go
@@ -526,6 +526,9 @@ func IsReadOnlyGlobalAddr(v *Value) bool {
 
 // IsNewObject reports whether v is a pointer to a freshly allocated & zeroed object at memory state mem.
 func IsNewObject(v *Value, mem *Value) bool {
+       if v.Type.IsUnsafePtr() {
+               return false
+       }
        if v.Op != OpLoad {
                return false
        }

If you're ok with it, I will send a CL.

[cherrymui]

@Gnouc this looks ok to me. Thanks!

[gopherbot]

Change https://golang.org/cl/170159 mentions this issue: cmd/compile: fix write barrier removal with unsafe pointer

[randall77]

I'm confused by this as well. I can believe the crash, but the provided code doesn't trigger that crash. We need a standalone reproducer (or any reproducer, really, from which we could extract a standalone one).

[kirillDanshin]

@randall77 https://travis-ci.org/gramework/gramework/jobs/513914890

[cuonglm]

@randall77 https://go-review.googlesource.com/c/go/+/170159/3/test/fixedbugs/issue31174.go

@cherrymui found it, the error message is a bit confused.

[randall77]

Thanks, that example crashes for me.

But that example uses linkname to access runtime internals. I'm not inclined to fix the Go toolchain to permit this use.

[cuonglm]

@randall77

But that example uses linkname to access runtime internals. I'm not inclined to fix the Go toolchain to permit this use.

Yes.

But I think it's worth to make the change, even not for fixing this issue. IsNewObject indicates it reports whether Value is a pointer to something, but without checking its Type is a pointer first.

If not, then the function doc should be updated.

[randall77]

That's because the compiler has control over all calls to runtime.newobject (except for linkname shenanigans, of course), and ensures that the type of the result of that call is always correct - a pointer to a base type.

[cherrymui]

Yes, if it only crashes with linkname into the runtime, I'm ok with not fixing it.

[kirillDanshin]

Honestly, I don't think that changing the compiler behavior in such a way that folks can't use basically one of the unsafe features is a good idea for a release. Previously, I reported #31121, a bug that silently broke about a thousand of my services by changing Map behavior, which changed ToLower behavior. This bug broke a package, which I need to do #17043, which as stated in #17043 (comment) will not be supported in Go (altho it is supported as a sync.Map, but sync.Map doesn't fit my needs due to its performance and algorithm). For me it's weird to see that go1.12 just changed really common function's behavior and 1.13 is going to break several technics that was allowed before, and I'm trying to understand where it's going when the current decision process kinda conflicts with ideas from @ianlancetaylor talk (watch it, if you haven't yet, it's really awesome talk).
I understand when some feature will never be available out-of-box, but I don't see any reasons to not add a three line fix to keep old compiler behavior, that was broken by another change. At least, if for some reason this issue will not be fixed, please, document that breaking change, and if possible, suggest a workaround so developers can migrate to the new Go version.

[cherrymui]

Linkname with unexported symbols is not "basically one of the unsafe feature". It may just work, but I would not call it "supported". For one, other implementations of the Go toolchain/runtime may not define the same symbol or have it visible externally. So this is already not a portable Go program. If it chooses to only work for a specific Go implementation, fine, but then it is also fine if it only works for a specific version of Go.

[randall77]

Honestly, I don't think that changing the compiler behavior in such a way that folks can't use basically one of the unsafe features is a good idea for a release.

We do this all the time for unexported API. The fact that the function named runtime.newobject even exists from one release to the next is surprising. For example, all of these functions disappeared from package runtime from 1.11 to 1.12:

< acquirep1
< casp
< clearScheduledCallback
< convT2E16
< convT2E32
< convT2E64
< convT2Eslice
< convT2Estring
< convT2I16
< convT2I32
< convT2I64
< convT2Islice
< convT2Istring
< endcgo
< gchelper
< gchelperstart
< gcprocs
< genasm
< getfull
< gosweepdone
< gosweepone
< hasprefix
< helpgc
< internal_cpu_initialize
< maxSliceCap
< mhelpgc
< needaddgcproc
< pauseSchedulerUntilCallback
< pauseSchedulerUntilCallback
< pauseSchedulerUntilCallback
< poll_runtimeNano
< poll_runtime_pollServerDescriptor
< scavengelist
< scavengetreap
< scavengeTreapNode
< scheduleCallback
< sleepUntilCallback
< time_runtimeNano

linkname is not even an unsafe feature. It's not documented in the unsafe package or the unsafe section of the spec. It's doubly unsafe. We never intended it to be used outside the stdlib (which needs it to, e.g., allow reflect to access runtime services).

If you're using linkname, the onus is on you to adapt to changes in the compiler and runtime.

#31121 is about exported API, and that's a totally different ballgame. For that we have the Go 1 compatibility guarantee. I think the relevant clause is

Bugs. If a compiler or library has a bug that violates the specification, a program that depends on the buggy behavior may break if the bug is fixed. We reserve the right to fix such bugs.

But I agree that it's a grey area, and whether #31121 fits in that category isn't as clear cut as this issue is.

[josharian]

You asked about a workaround. For now, you should be able to use linkname to call mallocgc. newobject is just a very thin wrapper around mallocgc anyway. But to reiterate what Keith and Cherry said, that may also break with no warning at any point in the future.

[kirillDanshin]

ok, I see. if no one want to fix this bug, let's at least do something with the error message. it points to a far different place in sources and first I was thinking that it really don't like my unsafe.Pointer conversion wrappers for some reason, but turns out that it's really just a runtime.newobject linking bug.

[cuonglm]

@kirillDanshin I'm afraid we can't.

As we're in SSA step, the lex and parse step were done, lineno will point to the start of last lexed token. If you added this function at the end of utils.go:

func f() string {
    return "f"
}

error message will point to start of return "f"

Is that right? @randall77 @cherrymui

[randall77]

I don't think that is right. This code (the example from the CL, with your f added) reports the right line number:

package p

import "unsafe"

type Type int // dummy

func Newobject(typ *Type) unsafe.Pointer {
	return newobject(typ)
}

//go:linkname newobject runtime.newobject
func newobject(typ *Type) unsafe.Pointer

func f() string {
	return "f"
}

We'd need a self-contained example demonstrating bad behavior to be able to fix it.

[cuonglm]

@randall77 Are you sure?

With the code above, it reports line 15 for me, which is return "f":

$ GO111MODULE=off go-tip build
# _/home/cuonglm/bin/go/runtimer
./runtimer.go:15:2: internal compiler error: Type.Elem UNSAFEPTR
$ cat -n  runtimer.go 
     1	package p
     2	
     3	import "unsafe"
     4	
     5	type Type int // dummy
     6	
     7	func Newobject(typ *Type) unsafe.Pointer {
     8		return newobject(typ)
     9	}
    10	
    11	//go:linkname newobject runtime.newobject
    12	func newobject(typ *Type) unsafe.Pointer
    13	
    14	func f() string {
    15		return "f"
    16	}

[randall77]

Very strange. Using your runttimer.go file:

MacPro:src khr$ ../bin/go tool compile ~/gowork/runtimer.go 
/Users/khr/gowork/runtimer.go:8:2: internal compiler error: Type.Elem UNSAFEPTR

goroutine 1 [running]:
runtime/debug.Stack(0x1c36740, 0xc000086000, 0x0)
	/Users/khr/sandbox/readonly/src/runtime/debug/stack.go:24 +0x9d
cmd/compile/internal/gc.Fatalf(0x1a8b4b7, 0xc, 0xc00008e910, 0x1, 0x1)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/gc/subr.go:190 +0x292
cmd/compile/internal/types.(*Type).Elem(0xc000089ce0, 0xc000390460)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/types/type.go:801 +0xff
cmd/compile/internal/ssa.(*Func).computeZeroMap(0xc0000dc9a0, 0x1)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/ssa/writebarrier.go:391 +0x101
cmd/compile/internal/ssa.writebarrier(0xc0000dc9a0)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/ssa/writebarrier.go:80 +0x6b
cmd/compile/internal/ssa.Compile(0xc0000dc9a0)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/ssa/compile.go:90 +0x476
cmd/compile/internal/gc.buildssa(0xc0000dc2c0, 0x0, 0x0)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/gc/ssa.go:288 +0xbf3
cmd/compile/internal/gc.compileSSA(0xc0000dc2c0, 0x0)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/gc/pgen.go:297 +0x4d
cmd/compile/internal/gc.compile(0xc0000dc2c0)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/gc/pgen.go:276 +0x59d
cmd/compile/internal/gc.funccompile(0xc0000dc2c0)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/gc/pgen.go:221 +0xc1
cmd/compile/internal/gc.Main(0x1aab4e0)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/gc/main.go:667 +0x3022
main.main()
	/Users/khr/sandbox/readonly/src/cmd/compile/main.go:51 +0xad

MacPro:src khr$ ../bin/go build ~/gowork/runtimer.go 
# command-line-arguments
../../../gowork/runtimer.go:15:2: internal compiler error: Type.Elem UNSAFEPTR

goroutine 7 [running]:
runtime/debug.Stack(0x1c36740, 0xc00009e000, 0x0)
	/Users/khr/sandbox/readonly/src/runtime/debug/stack.go:24 +0x9d
cmd/compile/internal/gc.Fatalf(0x1a8b4b7, 0xc, 0xc0000740b0, 0x1, 0x1)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/gc/subr.go:190 +0x292
cmd/compile/internal/types.(*Type).Elem(0xc0000a1da0, 0xc0003e4fd0)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/types/type.go:801 +0xff
cmd/compile/internal/ssa.(*Func).computeZeroMap(0xc000522000, 0x1)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/ssa/writebarrier.go:391 +0x101
cmd/compile/internal/ssa.writebarrier(0xc000522000)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/ssa/writebarrier.go:80 +0x6b
cmd/compile/internal/ssa.Compile(0xc000522000)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/ssa/compile.go:90 +0x476
cmd/compile/internal/gc.buildssa(0xc0000f42c0, 0x1, 0x0)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/gc/ssa.go:288 +0xbf3
cmd/compile/internal/gc.compileSSA(0xc0000f42c0, 0x1)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/gc/pgen.go:297 +0x4d
cmd/compile/internal/gc.compileFunctions.func2(0xc000070ea0, 0xc000016280, 0x1)
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/gc/pgen.go:362 +0x49
created by cmd/compile/internal/gc.compileFunctions
	/Users/khr/sandbox/readonly/src/cmd/compile/internal/gc/pgen.go:360 +0x128

So it looks like it depends on whether you use go build or go tool compile.

Looks like it is the -c=4 option passed from go build to the compiler. Adding that to the tool compile command line reproduces the issue.

So yes, it would be nice if an ICE at least reported a line number somewhere in the function that caused the compiler to ICE. (Which may not always be possible, but should generally work.)

[cuonglm]

@randall77 It seems not possible with current design.

In phase 8, top level functions are compiled:

go/src/cmd/compile/internal/gc/main.go

Line 660 in ce17481

// Phase 8: Compile top level functions.

But with c == 1, it's compiled immediately, with c >= 2, it's pushed to compilequeue:

go/src/cmd/compile/internal/gc/pgen.go

Line 275 in ce17481

if compilenow() {

and drain later by:

go/src/cmd/compile/internal/gc/main.go

Line 677 in ce17481

compileFunctions()

With more than 4 concurrent compilation, all of them will modify the global lineno variable concurrently, so we won't have the right lineno where error occurs.

[randall77]

With more than 4 concurrent compilation, all of them will modify the global lineno variable concurrently, so we won't have the right ineno where error occurs.

Right. We'd have to plumb the line number to the error reporter some other way, so that it uses the function-currently-being-compiled line number instead of the global one.

We're on a slow burn to remove all the global variables in the compiler anyway. This would be one step in that direction as well.