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: -race does not obey go:nocheckptr #42880

Open
chewxy opened this issue Nov 30, 2020 · 8 comments
Open

cmd/compile: -race does not obey go:nocheckptr #42880

chewxy opened this issue Nov 30, 2020 · 8 comments
Milestone

Comments

@chewxy
Copy link

@chewxy chewxy commented Nov 30, 2020

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

$ go version
go version go1.15.5 linux/amd64

Does this issue reproduce with the latest release?

yes (even with gotip)

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

go env Output
$ go env
GO111MODULE="off"
GOARCH="amd64"
GOBIN=""
GOCACHE="PATH/TO/go-build"
GOENV="PATH/TO/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="PATH/TO/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="GOPATH"
GOPRIVATE="PRIVATE PROXY HERE"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
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-build533267197=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I ran a test to test racy behaviours - This function had a checkptr error:

https://github.com/gorgonia/tensor/blob/9166554e0f40794485c199a8ecd4d692d24dbe73/consopt.go#L136-L171

The test is this: https://github.com/gorgonia/tensor/blob/9166554e0f40794485c199a8ecd4d692d24dbe73/consopt_test.go#L19-L33

Most notably, the function had a //go:nocheckptr directive attached to it. It's been there for the purpose of CUDA - since CUDA memory is not accessible by the Go runtime (technically it can, but it has to be run on the same thread as the CUDA runtime), I omitted the checkptr tests because they are likely to fail.

In the past I had manually used -d=checkptr. That seems to obey the go:nocheckptr directive.

What I did:

$ go test -run=FromMemory -race

What did you expect to see?

No checkptr errors.

What did you see instead?

 $ gotip test -run=FromMemory -race
fatal error: checkptr: pointer arithmetic result points to invalid allocation

goroutine 21 [running]:
runtime.throw(0x12a5898, 0x40)
	/home/chewxy/sdk/gotip/src/runtime/panic.go:1112 +0x72 fp=0xc000080910 sp=0xc0000808e0 pc=0x46c532
runtime.checkptrArithmetic(0xc000080a28, 0x0, 0x0, 0x0)
	/home/chewxy/sdk/gotip/src/runtime/checkptr.go:43 +0xbe fp=0xc000080940 sp=0xc000080910 pc=0x43a67e
gorgonia.org/tensor.FromMemory.func1(0x13b69a0, 0xc0002079e0)
	/home/chewxy/workspace/gorgoniaws/src/gorgonia.org/tensor/consopt.go:156 +0xe6 fp=0xc0000809c0 sp=0xc000080940 pc=0x10606c6
gorgonia.org/tensor.New(0xc000080a48, 0x3, 0x3, 0xc000164880)
	/home/chewxy/workspace/gorgoniaws/src/gorgonia.org/tensor/tensor.go:91 +0x82 fp=0xc000080a08 sp=0xc0000809c0 pc=0xf0e262
gorgonia.org/tensor.Test_FromMemory.func1(0xffdfd3dc8101f1bd, 0x0)
	/home/chewxy/workspace/gorgoniaws/src/gorgonia.org/tensor/consopt_test.go:22 +0xf7 fp=0xc000080a70 sp=0xc000080a08 pc=0x10a4b97
runtime.call16(0xc0001ba960, 0x12babd8, 0xc0001cb3f0, 0x800000010)
	/home/chewxy/sdk/gotip/src/runtime/asm_amd64.s:546 +0x3e fp=0xc000080a90 sp=0xc000080a70 pc=0x4a543e
reflect.Value.call(0x11cd280, 0x12babd8, 0x13, 0x12858cd, 0x4, 0xc000203e48, 0x1, 0x1, 0xc0001ba930, 0xc000155500, ...)
	/home/chewxy/sdk/gotip/src/reflect/value.go:476 +0x99c fp=0xc000080cc8 sp=0xc000080a90 pc=0x547e7c
reflect.Value.Call(0x11cd280, 0x12babd8, 0x13, 0xc000203e48, 0x1, 0x1, 0xc0001ba930, 0x0, 0x0)
	/home/chewxy/sdk/gotip/src/reflect/value.go:337 +0xd9 fp=0xc000080d60 sp=0xc000080cc8 pc=0x5471b9
testing/quick.Check(0x11cd280, 0x12babd8, 0x0, 0x47e3b6, 0x405a90)
	/home/chewxy/sdk/gotip/src/testing/quick/quick.go:290 +0x297 fp=0xc000080e68 sp=0xc000080d60 pc=0xe13b17
gorgonia.org/tensor.Test_FromMemory(0xc000103080)
	/home/chewxy/workspace/gorgoniaws/src/gorgonia.org/tensor/consopt_test.go:30 +0x54 fp=0xc000080ed0 sp=0xc000080e68 pc=0xf457b4
testing.tRunner(0xc000103080, 0x12babe0)
	/home/chewxy/sdk/gotip/src/testing/testing.go:1195 +0x203 fp=0xc000080fd0 sp=0xc000080ed0 pc=0x5a6043
runtime.goexit()
	/home/chewxy/sdk/gotip/src/runtime/asm_amd64.s:1367 +0x1 fp=0xc000080fd8 sp=0xc000080fd0 pc=0x4a6ea1
created by testing.(*T).Run
	/home/chewxy/sdk/gotip/src/testing/testing.go:1240 +0x5d8

goroutine 1 [chan receive]:
testing.(*T).Run(0xc000102f00, 0x128a950, 0xf, 0x12babe0, 0x1)
	/home/chewxy/sdk/gotip/src/testing/testing.go:1241 +0x610
testing.runTests.func1(0xc000102f00)
	/home/chewxy/sdk/gotip/src/testing/testing.go:1513 +0xa7
testing.tRunner(0xc000102f00, 0xc00016fce0)
	/home/chewxy/sdk/gotip/src/testing/testing.go:1195 +0x203
testing.runTests(0xc000203e30, 0x1873e40, 0x19b, 0x19b, 0xbfe93719422be217, 0x8bb4896263, 0x1883b40, 0xc00012c148)
	/home/chewxy/sdk/gotip/src/testing/testing.go:1511 +0x613
testing.(*M).Run(0xc0001ce080, 0x0)
	/home/chewxy/sdk/gotip/src/testing/testing.go:1419 +0x3b4
main.main()
	_testmain.go:1083 +0x237
exit status 2
@ianlancetaylor ianlancetaylor changed the title -race does not obey go:nocheckptr cmd/compile: -race does not obey go:nocheckptr Nov 30, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 30, 2020

Please show exactly what you saw. Thanks.

Please also describe exactly how to reproduce the problem. Thanks.

@chewxy
Copy link
Author

@chewxy chewxy commented Nov 30, 2020

Updated with what exactly I did, and what I saw

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 30, 2020

@chewxy
Copy link
Author

@chewxy chewxy commented Nov 30, 2020

A bit more to add. If I understand the Go semantics correctly, the test, as written shouldn't fail the checkptr either. But I am willing to concede that there are parts which I do not know as well

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Nov 30, 2020

It's not safe to convert a uintptr-typed variable to unsafe.Pointer: https://github.com/gorgonia/tensor/blob/9166554e0f40794485c199a8ecd4d692d24dbe73/consopt.go#L156. cmd/compile is correct to emit an error about that code.

As for why //go:nocheckptr didn't suppress the error, it's because the error is actually within a function literal, and //go: directives only apply to declared functions. Perhaps we should revisit that though.

@chewxy
Copy link
Author

@chewxy chewxy commented Nov 30, 2020

It's not safe to convert a uintptr-typed variable to unsafe.Pointer: https://github.com/gorgonia/tensor/blob/9166554e0f40794485c199a8ecd4d692d24dbe73/consopt.go#L156. cmd/compile is correct to emit an error about that code.

Yeah, this part I am aware of. The only time this is used is for CUDA code, really, which due to some complications, need unsafe.Pointer (until I finish integrating CUDA11... but that's gonna take time)

As for why //go:nocheckptr didn't suppress the error, it's because the error is actually within a function literal, and //go: directives only apply to declared functions. Perhaps we should revisit that though.

Can I put //go:nocheckptr on top of f := func(t Tensor) { ?

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Nov 30, 2020

Can I put //go:nocheckptr on top of f := func(t Tensor) { ?

You can't.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Nov 30, 2020

Yeah, this part I am aware of.

In your last message you wrote "A bit more to add. If I understand the Go semantics correctly, the test, as written shouldn't fail the checkptr either." I was explaining why the checkptr error is correct. Your earlier understanding was incorrect.

Can I put //go:nocheckptr on top of f := func(t Tensor) { ?

No. At the moment, //go:nocheckptr only applies to declared functions.

You could use a helper function like:

//go:nocheckptr
func asUnsafePointer(x uintptr) unsafe.Pointer { return unsafe.Pointer(x) }

But again, I'll emphasize your test case is in fact unsafe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.