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

reflect: MakeMap() and not native map type wrong behavior #37716

Closed
maxatome opened this issue Mar 6, 2020 · 7 comments
Closed

reflect: MakeMap() and not native map type wrong behavior #37716

maxatome opened this issue Mar 6, 2020 · 7 comments

Comments

@maxatome
Copy link

maxatome commented Mar 6, 2020

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

$ go version
go version go1.14 linux/amd64

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=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/msoule/.cache/go-build"
GOENV="/home/msoule/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/msoule/Projet/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/msoule/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/msoule/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-build869392983=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package main

import (
	"fmt"
	"reflect"
)

type Final map[int64]string

func main() {
	var out Final
	outVal := reflect.ValueOf(&out).Elem()

	valType := outVal.Type()
	valKeyType := valType.Key()
	valElemType := valType.Elem()
	mapType := reflect.MapOf(valKeyType, valElemType)

	valMap := reflect.MakeMap(mapType)

	currentKey := reflect.Indirect(reflect.New(reflect.TypeOf(int64(0))))
	currentKey.Set(reflect.ValueOf(int64(0)))
	currentVal := reflect.Indirect(reflect.New(reflect.TypeOf("")))
	currentVal.Set(reflect.ValueOf("pipo"))

	valMap.SetMapIndex(currentKey, currentVal)

	outVal.Set(valMap)
	fmt.Printf("> %s\n", valMap.MapIndex(reflect.ValueOf(int64(0))).Kind())
	fmt.Printf("> %s\n", outVal.MapIndex(reflect.ValueOf(int64(0))).Kind())
}

What did you expect to see?

> string
> string

What did you see instead?

> string
> invalid

Hints

  • It works on the same host with go1.13.7;
  • same wrong behavior on FreeBSD/amd64;
  • but works on https://play.golang.org/;
  • if we replace var out Final by var out map[int64]string it works;
  • var out = map[int64]string{} works, but var out = Final{} still fails;
  • type Final map[string]string (string key instead of int64 key), and replacing all int64(0) occurrences by "0", works.
@maxatome
Copy link
Author

maxatome commented Mar 6, 2020

I forgot adding a final:

fmt.Printf("> %s\n", out[0])

prints:

pipo

:)

@randall77
Copy link
Contributor

Strange. I can reproduce with 1.14 on darwin/amd64.
I cannot reproduce on tip, though. I guess a bisect might illuminate something.
I'll keep looking.

@randall77 randall77 self-assigned this Mar 6, 2020
@randall77
Copy link
Contributor

Looks like a hash function mismatch.
It was accidentally fixed at tip by https://go-review.googlesource.com/c/go/+/219338 . But not really fixed, just fixed for int32 and int64 keys.
The named and unnamed maps have different hash functions. When assigning one to the other, we just copy the map, the type (which contains the hash function) doesn't come along for the ride.
Still thinking about how to fix.

@randall77
Copy link
Contributor

Here's a somewhat simpler reproducer, including at tip.

package main

import "reflect"

// complicated enough to require a compile-generated hash function
type K struct {
     a, b int32 // these get merged by the compiler into a single field, something typehash doesn't do
     c float64  // make the key not plain memory
}

func main() {
     k := K{a:1,b:2,c:3}

     // Make a reflect map.
     m := reflect.MakeMap(reflect.MapOf(reflect.TypeOf(K{}), reflect.TypeOf(true)))
     m.SetMapIndex(reflect.ValueOf(k), reflect.ValueOf(true))

     // The binary must not contain the type map[K]bool anywhere, or reflect.MapOf
     // will use that type instead of making a new one. So use an equivalent named type.
     type M map[K]bool
     var x M
     reflect.ValueOf(&x).Elem().Set(m)
     println(x[k])
}

This should print true, but prints false for 1.14 and tip.

@randall77 randall77 added this to the Go1.14.1 milestone Mar 6, 2020
@gopherbot
Copy link

Change https://golang.org/cl/222357 mentions this issue: runtime: make typehash match compiler generated hashes exactly

@randall77 randall77 modified the milestones: Go1.14.1, Go1.15 Mar 6, 2020
@randall77
Copy link
Contributor

@gopherbot please open a backport issue for 1.14.

@gopherbot
Copy link

Backport issue(s) opened: #37721 (for 1.14).

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants