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

unique: panic for Make(struct{}{}) #69458

Closed
haruyama480 opened this issue Sep 13, 2024 · 8 comments
Closed

unique: panic for Make(struct{}{}) #69458

haruyama480 opened this issue Sep 13, 2024 · 8 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@haruyama480
Copy link
Contributor

haruyama480 commented Sep 13, 2024

Go version

go version go1.23.1 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/xxx/Library/Caches/go-build'
GOENV='/Users/xxx/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/xxx/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/xxx/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/xxx/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.1.darwin-arm64'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='go1.23.1'
GOTOOLDIR='/Users/xxx/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.1.darwin-arm64/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23.1'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/xxx/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/xxx/ghq/github.com/xxx/go-learning/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/7c/vn1g1x4n4nqclvb5dnjkf8q80000gn/T/go-build85467120=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

https://go.dev/play/p/esaCHP90Kzk

output
fatal error: getWeakHandle on invalid pointer

goroutine 1 gp=0xc0000061c0 m=0 mp=0x5003e0 [running]:
runtime.throw({0x4857e3?, 0x500301?})
	/usr/local/go-faketime/src/runtime/panic.go:1067 +0x48 fp=0xc0000485c0 sp=0xc000048590 pc=0x45d948
runtime.getWeakHandle(0x51eec0)
	/usr/local/go-faketime/src/runtime/mheap.go:2135 +0x113 fp=0xc000048608 sp=0xc0000485c0 pc=0x422f93
runtime.getOrAddWeakHandle(0x51eec0)
	/usr/local/go-faketime/src/runtime/mheap.go:2083 +0x1c fp=0xc000048650 sp=0xc000048608 pc=0x422cbc
internal/weak.runtime_registerWeakPointer(0x0?)
	/usr/local/go-faketime/src/runtime/mheap.go:2045 +0x13 fp=0xc000048668 sp=0xc000048650 pc=0x45d233
internal/weak.Make[...](0x51eec0?)
	/usr/local/go-faketime/src/internal/weak/pointer.go:62 +0x5d fp=0xc000048680 sp=0xc000048668 pc=0x46a63d
unique.Make[...].func1()
	/usr/local/go-faketime/src/unique/handle.go:57 +0x88 fp=0xc0000486b8 sp=0xc000048680 pc=0x46a588
unique.Make[...]({})
	/usr/local/go-faketime/src/unique/handle.go:67 +0x135 fp=0xc000048738 sp=0xc0000486b8 pc=0x46a475
main.main()
	/tmp/sandbox2127609540/prog.go:8 +0x1a fp=0xc000048750 sp=0xc000048738 pc=0x46941a
runtime.main()
	/usr/local/go-faketime/src/runtime/proc.go:272 +0x28b fp=0xc0000487e0 sp=0xc000048750 pc=0x4315eb
runtime.goexit({})
	/usr/local/go-faketime/src/runtime/asm_amd64.s:1700 +0x1 fp=0xc0000487e8 sp=0xc0000487e0 pc=0x463ea1

goroutine 2 gp=0xc000006700 m=nil [force gc (idle)]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
	/usr/local/go-faketime/src/runtime/proc.go:424 +0xce fp=0xc000048fa8 sp=0xc000048f88 pc=0x45da2e
runtime.goparkunlock(...)
	/usr/local/go-faketime/src/runtime/proc.go:430
runtime.forcegchelper()
	/usr/local/go-faketime/src/runtime/proc.go:337 +0xa5 fp=0xc000048fe0 sp=0xc000048fa8 pc=0x431925
runtime.goexit({})
	/usr/local/go-faketime/src/runtime/asm_amd64.s:1700 +0x1 fp=0xc000048fe8 sp=0xc000048fe0 pc=0x463ea1
created by runtime.init.7 in goroutine 1
	/usr/local/go-faketime/src/runtime/proc.go:325 +0x1a

goroutine 3 gp=0xc000006c40 m=nil [GC sweep wait]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
	/usr/local/go-faketime/src/runtime/proc.go:424 +0xce fp=0xc000049780 sp=0xc000049760 pc=0x45da2e
runtime.goparkunlock(...)
	/usr/local/go-faketime/src/runtime/proc.go:430
runtime.bgsweep(0xc00006a000)
	/usr/local/go-faketime/src/runtime/mgcsweep.go:277 +0x94 fp=0xc0000497c8 sp=0xc000049780 pc=0x41d814
runtime.gcenable.gowrap1()
	/usr/local/go-faketime/src/runtime/mgc.go:203 +0x25 fp=0xc0000497e0 sp=0xc0000497c8 pc=0x412285
runtime.goexit({})
	/usr/local/go-faketime/src/runtime/asm_amd64.s:1700 +0x1 fp=0xc0000497e8 sp=0xc0000497e0 pc=0x463ea1
created by runtime.gcenable in goroutine 1
	/usr/local/go-faketime/src/runtime/mgc.go:203 +0x66

goroutine 4 gp=0xc000006e00 m=nil [GC scavenge wait]:
runtime.gopark(0xc00006a000?, 0x49eab8?, 0x1?, 0x0?, 0xc000006e00?)
	/usr/local/go-faketime/src/runtime/proc.go:424 +0xce fp=0xc000049f78 sp=0xc000049f58 pc=0x45da2e
runtime.goparkunlock(...)
	/usr/local/go-faketime/src/runtime/proc.go:430
runtime.(*scavengerState).park(0x4ff640)
	/usr/local/go-faketime/src/runtime/mgcscavenge.go:425 +0x49 fp=0xc000049fa8 sp=0xc000049f78 pc=0x41b289
runtime.bgscavenge(0xc00006a000)
	/usr/local/go-faketime/src/runtime/mgcscavenge.go:653 +0x3c fp=0xc000049fc8 sp=0xc000049fa8 pc=0x41b7bc
runtime.gcenable.gowrap2()
	/usr/local/go-faketime/src/runtime/mgc.go:204 +0x25 fp=0xc000049fe0 sp=0xc000049fc8 pc=0x412225
runtime.goexit({})
	/usr/local/go-faketime/src/runtime/asm_amd64.s:1700 +0x1 fp=0xc000049fe8 sp=0xc000049fe0 pc=0x463ea1
created by runtime.gcenable in goroutine 1
	/usr/local/go-faketime/src/runtime/mgc.go:204 +0xa5

goroutine 17 gp=0xc000084380 m=nil [chan receive]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
	/usr/local/go-faketime/src/runtime/proc.go:424 +0xce fp=0xc000044718 sp=0xc0000446f8 pc=0x45da2e
runtime.chanrecv(0xc000098070, 0x0, 0x1)
	/usr/local/go-faketime/src/runtime/chan.go:639 +0x41c fp=0xc000044790 sp=0xc000044718 pc=0x40587c
runtime.chanrecv1(0x0?, 0x0?)
	/usr/local/go-faketime/src/runtime/chan.go:489 +0x12 fp=0xc0000447b8 sp=0xc000044790 pc=0x405452
runtime.unique_runtime_registerUniqueMapCleanup.func1(...)
	/usr/local/go-faketime/src/runtime/mgc.go:1732
runtime.unique_runtime_registerUniqueMapCleanup.gowrap1()
	/usr/local/go-faketime/src/runtime/mgc.go:1735 +0x2f fp=0xc0000447e0 sp=0xc0000447b8 pc=0x414fef
runtime.goexit({})
	/usr/local/go-faketime/src/runtime/asm_amd64.s:1700 +0x1 fp=0xc0000447e8 sp=0xc0000447e0 pc=0x463ea1
created by unique.runtime_registerUniqueMapCleanup in goroutine 1
	/usr/local/go-faketime/src/runtime/mgc.go:1730 +0x96

Program exited.

What did you see happen?

fatal error: getWeakHandle on invalid pointer

What did you expect to see?

no panic

@haruyama480
Copy link
Contributor Author

struct{} has no value, so there may be no way to implement unique.Make(sturct{}{}).
However, a runtime panic may be somewhat surprising.

@ianlancetaylor
Copy link
Contributor

CC @mknyszek

@timothy-king timothy-king added this to the Backlog milestone Sep 13, 2024
@timothy-king timothy-king added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 13, 2024
@cuonglm
Copy link
Member

cuonglm commented Sep 14, 2024

The address of *T where T is zero-size type will be the address of runtime.zerobase, which does not belong to heap address, thus the runtime panic.

It seems to me that we need a special treatment for zerobase.

@haruyama480
Copy link
Contributor Author

haruyama480 commented Sep 14, 2024

It seems to me that we need a special treatment for zerobase.
yes.

I think a traditional approach is treat zerobase as a null pointer in Handle[T], though I don't know Go can treat a null pointer.
To apply this handling for all types, we can treat zero value too, to omit insertion of default value.

@mknyszek
Copy link
Contributor

I think we do need to handle zero-sized types specially, but I don't think the unique package needs to be aware of runtime.zerobase.

In theory we can have all zero-sized types use the same pointer under the hood, because you can't compare two Handle[T] where T differs anyway, and all zero-sized values of the same type compare equal with each other (really, there's only one value for each type).

package unique

var zero uintptr

func Make[T comparable](value T) Handle[T] {
    if unsafe.Sizeof(*new(T)) == 0 {
        return Handle[T]{(*T)(unsafe.Pointer(&zero))}
    }
    // Rest of Make.
    ...
}

func (h Handle[T]) Value() T {
    if unsafe.Sizeof(*new(T)) == 0 {
        return *new(T)
    }
    return *h.value
}

I don't really see the use-case for this, so it doesn't seem urgent, but I can send a patch for this.

@mknyszek
Copy link
Contributor

Actually @haruyama480 is right, we don't need var zero uintptr and &zero, we can just say nil. That works too.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/613397 mentions this issue: unique: handle zero-size types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

7 participants