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: generic code seems to produce duplicate type descriptor #53087

Closed
Porges opened this issue May 26, 2022 · 13 comments
Closed

cmd/compile: generic code seems to produce duplicate type descriptor #53087

Porges opened this issue May 26, 2022 · 13 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@Porges
Copy link

Porges commented May 26, 2022

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

Tested on 1.18 and 1.18.2.

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE="auto"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/vscode/.cache/go-build"
GOENV="/home/vscode/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.2"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/workspaces/azure-service-operator/v2/tools/generator/go.mod"
GOWORK=""
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-build1336644465=/tmp/go-build -gno-record-gcc-switches"

What did you do?

After using maps.Clone, values retrieved from the (copied) map fail to typecast correctly.

I’m still working on a minimal repro, but here is what I’m seeing.

See small repro below.


The original code in question looks like this:

var fromF astmodel.Function = convertFrom
_, ok := fromF.(*functions.PropertyAssignmentFunction)
fmt.Printf("from ok: %v\n", ok)

var toF astmodel.Function = convertTo
_, ok = toF.(*functions.PropertyAssignmentFunction)
fmt.Printf("to ok: %v\n", ok)

m := readonly.EmptyMap[string, astmodel.Function]()
m = m.With("a", convertFrom)
m = m.With("b", convertTo)

it, found := m.Get("a")
if found {
	_, ok = it.(*functions.PropertyAssignmentFunction)
	fmt.Printf("from ok: %v\n", ok)
}

it, found = m.Get("b")
if found {
	_, ok = it.(*functions.PropertyAssignmentFunction)
	fmt.Printf("to ok: %v\n", ok)
}

The expectation is that this prints true four times as the values retrieved from the map are the same as those put into it. However, it does not; the first value retrieved from the map fails to cast correctly.

Output:

from ok: true
to ok: true
from ok: false
to ok: true

The implementation of With looks like this:

func (m Map[K, V]) With(key K, value V) Map[K, V] {
	result := maps.Clone(m.inner)
	result[key] = value
	return CreateMapUnsafe(result)
}

I narrowed down the problem to maps.Clone. If I copy it as a local function then this continues to fail:

func CloneBad[M ~map[K]V, K comparable, V any](m M) M {
	r := make(M, len(m))
	for k, v := range m {
		r[k] = v
	}
	return r
}

However, if the M parameter is replaced with map[K]V directly, then it works as expected:

func CloneGood[K comparable, V any](m map[K]V) map[K]V {
	r := make(map[K]V, len(m))
	for k, v := range m {
		r[k] = v
	}
	return r
}

Note that this also happens if the same value is input as both "a" and "b"; whichever value was in the map before it was maps.Cloned is the one that fails to cast correctly.


Repro

Switch between CloneBad (a copy of maps.Clone) and CloneGood to see the behaviour change.

package main

import (
	"fmt"
)

type I interface {
	Run() string
}

type S struct {
	str string
}

func (s *S) Run() string {
	return s.str
}

var _ I = &S{}

type CloningMap[K comparable, V any] struct {
	inner map[K]V
}

func (cm CloningMap[K, V]) With(key K, value V) CloningMap[K, V] {
	result := CloneBad(cm.inner)
	result[key] = value
	return CloningMap[K, V]{result}
}

func CloneGood[K comparable, V any](m map[K]V) map[K]V {
	r := make(map[K]V, len(m))
	for k, v := range m {
		r[k] = v
	}
	return r
}

func CloneBad[M ~map[K]V, K comparable, V any](m M) M {
	r := make(M, len(m))
	for k, v := range m {
		r[k] = v
	}
	return r
}

func main() {
	s1 := &S{"one"}
	s2 := &S{"two"}

	m := CloningMap[string, I]{inner: make(map[string]I)}
	m = m.With("a", s1)
	m = m.With("b", s2)

	it, found := m.inner["a"]
	if found {
		_, ok := it.(*S)
		fmt.Printf("from ok: %v\n", ok)
	}

	it, found = m.inner["b"]
	if found {
		_, ok := it.(*S)
		fmt.Printf("to ok: %v\n", ok)
	}
}
@ianlancetaylor ianlancetaylor changed the title affected/package: maps.Clone underlying type miscompilation(??) cmd/compile: generic code seems to produce duplicate type descriptor May 26, 2022
@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented May 26, 2022

Thanks for the bug report. Here is a slightly trimmed down test case. For me this prints

panic: got *main.S want *main.S

Perhaps the type descriptor got duplicated.

CC @randall77 @mdempsky

package main

import "fmt"

type I interface {
	M()
}

type S struct {
	str string
}

func (s *S) M() {}

var _ I = &S{}

type CloningMap[K comparable, V any] struct {
	inner map[K]V
}

func (cm CloningMap[K, V]) With(key K, value V) CloningMap[K, V] {
	result := CloneBad(cm.inner)
	result[key] = value
	return CloningMap[K, V]{result}
}

func CloneBad[M ~map[K]V, K comparable, V any](m M) M {
	r := make(M, len(m))
	for k, v := range m {
		r[k] = v
	}
	return r
}

func main() {
	s1 := &S{"one"}
	s2 := &S{"two"}

	m := CloningMap[string, I]{inner: make(map[string]I)}
	m = m.With("a", s1)
	m = m.With("b", s2)

	it, found := m.inner["a"]
	if !found {
		panic("a not found")
	}
	if _, ok := it.(*S); !ok {
		panic(fmt.Sprintf("got %T want *main.S", it))
	}
}

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 26, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.19 milestone May 26, 2022
@cherrymui
Copy link
Member

The type descriptor is not duplicated, but the itab is. it.(*S) is compiled to comparing the itab of it to the itab of S converted to I (go.itab.*main.S,main.I). For some reason it points to a runtime-generated itab, instead of the static one.

@randall77
Copy link
Contributor

There is one I,*main.S itab and one interface{M()},*main.S itab. Somehow we have one named and one unnamed version of that same interface.
I'll keep digging.

@randall77 randall77 self-assigned this May 26, 2022
@cherrymui
Copy link
Member

CloneBad is instantiated as CloneBad[go.shape.map[string]interface { M() }_0,go.shape.string_1,go.shape.interface { M() }_2]. The map assignment in the loop body of CloneBad is compiled to, effectively, r[k] = interface { M() }(v), which then causes the runtime to create an itab for interface { M() }(v), i.e. concrete type *S, interface type interface { M() }. This itab is different from the static itab for converting an *S to I, so the comparison fails.

The conversion interface { M() }(v) is probably problematic. Maybe it shouldn't be there, as it knows the type of v is the same as the value type of r. Or maybe it should use a type descriptor from the dictionary.

@randall77
Copy link
Contributor

I've had a general unease about instantiating a function like

F[M ~map[K]V, K comparable, V any]

with K==string, V==int as

F[.shape.map[string]int{}, .shape.string, shape.int]

instead of

F[.shape.map[.shape.string].shape.int{}, .shape.string, shape.int]

Or something like that. Having K be a shape type but M[K] be a concrete type is unsettling, they should match.

That said, I'm not super sure that's the cause of this bug. Just a hypothesis.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/409356 mentions this issue: cmd/compile: fix wrong underlying type for shapify-ed types

@dmitshur
Copy link
Contributor

dmitshur commented Jun 1, 2022

Checking on on this as it's currently blocking beta 1. Do you expect it'll be resolved by next week?

@randall77
Copy link
Contributor

Honestly I don't know.
I'm going to remove beta blocker. This is a bug in 1.18 also, so I don't think it will introduce any new failures for people trying 1.19 beta.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/410197 mentions this issue: cmd/compile: don't remove interface names when shapifying

@Porges
Copy link
Author

Porges commented Jun 6, 2022

I’ve managed to hit this without any T ~ M code (no reduced case yet) so it seems that it might be more widely hittable than I thought.

@dmitshur dmitshur added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 12, 2022
@matthchr
Copy link

Wondering if there's been any movement on this?

@gopherbot gopherbot modified the milestones: Go1.19, Go1.20 Aug 2, 2022
@matthchr
Copy link

matthchr commented Nov 8, 2022

It looks like both @Porges and @ianlancetaylor's repros are passing on the Go dev branch of the golang playground: https://go.dev/play/p/gxm5cDGYxKv?v=gotip

Is it possible that this has been fixed in master? I don't see any issue/PR mentioning this, so unsure if it was really fixed or if something has changed so the repro doesn't work.

Possibly fixed by this? 38edd9b

@randall77
Copy link
Contributor

This has been fixed at tip. I don't think we'll be backporting, as it would require changes to the non-unified generics implementation.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 10, 2022
gopherbot pushed a commit that referenced this issue Mar 8, 2023
This issue has been fixed with unified IR, so just add a test.

Update #53087

Change-Id: I965d9f27529fa6b7c89e2921c65e5a100daeb9fe
Reviewed-on: https://go-review.googlesource.com/c/go/+/410197
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Auto-Submit: Keith Randall <khr@google.com>
@golang golang locked and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: Done
Development

No branches or pull requests

7 participants