gccgo: non-canonical reflect.Type

[cosmos72]

Please answer these questions before submitting your issue. Thanks!

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

Created a reflect.Type with reflect.SliceOf() and compared against reflect.TypeOf([]int{}).
See https://play.golang.org/p/ua1-JdssJSF

What did you expect to see?

good: []int == []int

instances of reflect.TypeOf([]int{}) should be canonical, i.e. == shoud return true, no matter how they are created: with reflection, or any other means - see https://golang.org/pkg/reflect/#Type

What did you see instead?

BAD: []int != []int

On gccgo, the reflect.Type created with reflect.SliceOf() compares as different than reflect.TypeOf([]int{})

The same issue happens at least with reflect.FuncOf(), see https://play.golang.org/p/_t5enrDarXt
so an educated guess is that instances of reflect.Type are not always canonicalized.

Does this issue reproduce with the latest release (go1.10.2)?

Only the gccgo compiler shows this issue. Tested on:
go version go1.10 gccgo (Debian 8.1.0-1) 8.1.0 linux/amd64

System details

go version go1.10 gccgo (Debian 8.1.0-1) 8.1.0 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/max/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/max/go"
GORACE=""
GOROOT="/usr"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/gcc/x86_64-linux-gnu/8"
GCCGO="/usr/bin/x86_64-linux-gnu-gccgo-8"
CC="x86_64-linux-gnu-gcc-8"
CXX="x86_64-linux-gnu-g++-8"
CGO_ENABLED="1"
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-build795045746=/tmp/go-build -gno-record-gcc-switches -funwind-tables"
uname -sr: Linux 4.15.13-phoenix-1
Distributor ID:	Debian
Description:	Debian GNU/Linux stable-updates (sid)
Release:	stable-updates
Codename:	sid
/lib/x86_64-linux-gnu/libc.so.6: GNU C Library (Debian GLIBC 2.27-1) stable release version 2.27.
gdb --version: GNU gdb (Debian 7.12-6+b1) 7.12.0.20161007-git

[ianlancetaylor]

Definitely a bug, but hard to fix.

[cosmos72]

Would you care to elaborate why it's hard to fix?

I naively thought that a canonicalizing map from reflect.Type to reflect.Type, analogous to what golang.org/x/tools/go/types/typeutil.Map does for go/types.Type, would suffice

[Update] multiple calls to reflect.SliceOf(reflect.TypeOf(int(0))) already return the same reflect.Type. The problem seems limited to reflect.TypeOf() that does not return canonicalized types

[ianlancetaylor]

Yes, we already have a canonicaizing map in gccgo's version of the reflect package. What we don't have is a mechanism for efficiently entering the program defined types into that map.

The gc toolchain builds a list of program defined types in the linker. But doing this efficiently without massive duplication requires linker support that gccgo doesn't have. What we need is some mechanism that only introduces a performance cost for the very rare programs that call reflect.SliceOf and friends.

[cosmos72]

Having examined gcc-20180425/libgo/go/reflect/type.go, I have an idea that only touches reflect.SliceOf and friends as you want:

it's basically about returning canonicalize(ti.(Type)) instead of ti.(Type) as last statement in SliceOf() - I will describe more in detail in the next days, because there are some non-trivial interactions between the several maps: lookupCache, funcLookupCache, structLookupCache and canonicalType

[cosmos72]

Here are the details:

in my opinion there are at least three alternative fixes

0. "build a list of program defined types in the linker" as you wrote. Hard, requires linker support
1. a minimal fix: call canonicalize() before returning a type in the functions reflect.ArrayOf(), reflect.ChanOf(), reflect.FuncOf(), reflect.MapOf(), reflect.PtrTo(), reflect.SliceOf(), reflect.StructOf()
2. a more complex fix where the three caches lookupCache, funcLookupCache, structLookupCache in reflect/type.go cooperate with canonicalType, in the sense that they only store canonicalized types. This seems tricky to implement because of thread-safety. In extreme summary, it requires that all reflect.Type to be inserted into these caches are first passed through toType() or canonicalize() to be canonicalized, and only later inserted in the caches.

Does it make sense to you?

[ianlancetaylor]

I don't understand why calling canonicalize will fix the problem. canonicalize maintains a map of type structures created at run time, but the type structures created by the compiler won't be in that map. Unless I'm forgetting something.

[cosmos72]

You're right in one point: at program startup they will not be in that map.

But all other functions that return a reflect.Type (reflect.TypeOf(), reflect.Value.Type() and many others) already invoke canonicalize() indirectly through toType(), so the canonicalType map will be populated on-demand as a side effect of retrieving reflect.Types.
By having all function that return a reflect.Type go through canonicalize(), user code will be guaranteed to only see canonical reflect.Types.

If you want, I have a patch - and a test suite showing that the patch is effective at least in the tested cases

[ianlancetaylor]

Ah, of course, got it. Yes, clearly all the reflect functions should return via canonicalize one way or another. It's a bug that they don't. Thanks for looking into this. Do you want to send in your patch?

[cosmos72]

Good , thanks :)

If you need just the patch and test suite, here they are:
gccgo-reflecttype-fix.diff.txt
reflectType_test.go.txt

For the formal contribution procedure to gccgo I will need a bit more time

[ianlancetaylor]

Thanks. I'm not going to look at the patches to avoid any copyright issues. You should be able to contribute to gofrontend using a Github pull request if you like.

[cosmos72]

Sent the patch using the official channel :)

https://go-review.googlesource.com/c/gofrontend/+/112575

[cosmos72]

Sent new patch following Ian's recommendation: https://go-review.googlesource.com/115577

[gopherbot]

Change https://golang.org/cl/115635 mentions this issue: reflect: check that types match without calling TypeOf

[gopherbot]

Change https://golang.org/cl/115577 mentions this issue: reflect: canonicalize types returned by StructOf() and friends

[ianlancetaylor]

This test still passes with gc but fails with gccgo:

package main

import (
	"fmt"
	"reflect"
)

func main() {
	type Interface interface{}
	t1 := reflect.StructOf([]reflect.StructField{
		{
			Name: "I",
			Type: reflect.TypeOf((*Interface)(nil)).Elem(),
		},
	})
	t2 := reflect.TypeOf(struct { I Interface }{nil})
	fmt.Println(t1 == t2)
}

[gopherbot]

Change https://golang.org/cl/116416 mentions this issue: reflect: add StructOf test case that gccgo used to fail

[gopherbot]

Change https://golang.org/cl/116515 mentions this issue: reflect: fix StructOf hash and string

[cosmos72]

Exactly.
At the moment I have no idea how to fix that.

[ianlancetaylor]

@cosmos72 I sent a fix in https://golang.org/cl/116515. Thanks for uncovering the problem.