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: crash on pointer conversion in call to mapaccess2 #51840

Closed
julianiewiejska opened this issue Mar 21, 2022 · 8 comments
Closed

cmd/compile: crash on pointer conversion in call to mapaccess2 #51840

julianiewiejska opened this issue Mar 21, 2022 · 8 comments
Labels
NeedsInvestigation release-blocker
Milestone

Comments

@julianiewiejska
Copy link

@julianiewiejska julianiewiejska commented Mar 21, 2022

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

go version go1.18 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

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

What did you expect to see?

Code compiles and runs without errors.

What did you see instead?

Code produces error:

<autogenerated>:1: cannot use &.autotmp_19 (type *struct { netip.addr netip.uint128; netip.z *intern.Value }) as type *netip.Addr in argument to runtime.mapaccess2

Other remarks

This seems to only happen when maps.Equal is used in the method even if the method is not called. There is no error when it is called directly: https://go.dev/play/p/KlpiWS96MA-

@gopherbot gopherbot added this to the Unreleased milestone Mar 21, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 21, 2022

Here is a standalone test case. CC @randall77 @mdempsky

package main

type Addr struct {
	hi uint64
	lo uint64
	z *byte
}

func EqualMap[M1, M2 ~map[K]V, K, V comparable](m1 M1, m2 M2) bool {
	for k, v1 := range m1 {
		if v2, ok := m2[k]; !ok || v1 != v2 {
			return false
		}
	}
	return true
}

type Set[T comparable] map[T]struct{}

func NewSet[T comparable](items ...T) Set[T] {
	return nil
}

func (s Set[T]) Equals(other Set[T]) bool {
	return EqualMap(s, other)
}

func main() {
	NewSet[Addr](Addr{0, 0, nil})
}
<autogenerated>:1: cannot use &.autotmp_17 (type *struct { hi uint64; lo uint64; z *byte }) as type *Addr in argument to runtime.mapaccess2

@ianlancetaylor ianlancetaylor changed the title x/exp/maps and netip: error when using maps.Equal in a Method on generic map with netip.Addr as keys cmd/compile: crash on pointer conversion in call to mapaccess2 Mar 21, 2022
@ianlancetaylor ianlancetaylor added NeedsInvestigation release-blocker labels Mar 21, 2022
@ianlancetaylor ianlancetaylor removed this from the Unreleased milestone Mar 21, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.19 milestone Mar 21, 2022
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 21, 2022

@gopherbot Please open a backport to 1.18

This is a compiler crash on valid code.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 21, 2022

Backport issue(s) opened: #51849 (for 1.18).

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

@blackgreen100
Copy link

@blackgreen100 blackgreen100 commented Apr 7, 2022

it seems the crash reproduces also on mapassign, and also when the key's underlying type is some composite type with more than one member:

Porges added a commit to Azure/azure-service-operator that referenced this issue Apr 19, 2022
Based upon expanding the `StringSet` type. Note that we cannot substitute uses of `map[T]struct{}` where `T` is an interface because interface types are [not comparable](golang/go#51338), even though the `map[T]struct{}` version works.

`AreEqual` is a standalone function at the moment because making it a method [crashes the compiler](golang/go#51840).
@wdvxdr1123
Copy link
Contributor

@wdvxdr1123 wdvxdr1123 commented May 7, 2022

I noticed this issue has been fixed in 115c3bf.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 7, 2022

@wdvxdr1123 Thanks, for the heads up. That's unexpected, but a believable effect.

If anyone wants to contribute a CL adding the failure cases as a regression test, that would be great. (Just add a file $GOROOT/test/fixedbugs/issue51840.go that demonstrates the previously failing code now compiles correctly. Look for other "// compile" or "// run" test cases in the directory, if you need inspiration.)

Otherwise, I'll prepare one on Monday.

@gopherbot
Copy link

@gopherbot gopherbot commented May 8, 2022

Change https://go.dev/cl/404914 mentions this issue: test: add test case for #51840

@gopherbot
Copy link

@gopherbot gopherbot commented May 10, 2022

Change https://go.dev/cl/405436 mentions this issue: [release-branch.go1.18] cmd/compile: backport fix for #51840

gopherbot pushed a commit that referenced this issue May 25, 2022
This CL is a manual backport of CLs 403837 and 404914 to Go 1.18.

CL 403837 was intended just as a simplification CL, but evidently it
also fixed #51840. However, for backporting to Go 1.18, the existing
logic needs to be preserved to support -G=0 mode (which still relies
on Ntype).

Fixes #51849.

Change-Id: Ib060b0bc67ecf26de8a65d5b4d2f8a65cd547517
Reviewed-on: https://go-review.googlesource.com/c/go/+/405436
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants