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: identical generic types compare unequal #49241

Closed
FiloSottile opened this issue Oct 30, 2021 · 6 comments
Closed

cmd/compile: identical generic types compare unequal #49241

FiloSottile opened this issue Oct 30, 2021 · 6 comments
Labels
generics NeedsInvestigation
Milestone

Comments

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Oct 30, 2021

(Note: this is my first attempt at writing generic code, the chances I am just wrong are high.)

crypto/elliptic has a few functions like P256() Curve and P384() Curve that return a singleton implementation of that curve. The singleton is part of the contract, because applications are expected to check what curve is in use by comparing it against the return value of elliptic.P384().

In CL 360015 I experimented with turning the concrete implementation of those singletons into a pointer to a generic type. (Feedback on the use of generics welcome!)

Unexpectedly, this led to a test failure that stems from this switch not matching the singletons anymore.

go/src/crypto/x509/x509.go

Lines 508 to 521 in 2ebe77a

func oidFromNamedCurve(curve elliptic.Curve) (asn1.ObjectIdentifier, bool) {
switch curve {
case elliptic.P224():
return oidNamedCurveP224, true
case elliptic.P256():
return oidNamedCurveP256, true
case elliptic.P384():
return oidNamedCurveP384, true
case elliptic.P521():
return oidNamedCurveP521, true
}
return nil, false
}

I tried reproducing it with a contained example on the playground, but things worked as expected, so I am not sure what is going on. https://go2goplay.golang.org/p/9jnZvqnPDrh

/cc @griesemer

@FiloSottile FiloSottile added the generics label Oct 30, 2021
@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 30, 2021

Can you whittle down (remove everything that's not needed to re-produce the problem) in the real code and send that CL? Then we can cherry-pick and investigate.

@griesemer griesemer added this to the Go1.18 milestone Oct 30, 2021
@griesemer griesemer added the NeedsInvestigation label Oct 30, 2021
@randall77
Copy link
Contributor

@randall77 randall77 commented Oct 30, 2021

Hm, the switch is failing because the itab of the incoming interface doesn't match the itab returned by the calls.
There are 4 identically-named, but different, itabs in this binary:

00000000011f4e08 s _go.itab.*crypto/elliptic.nistCurve[*crypto/elliptic/internal/nistec.P224Point],crypto/elliptic.Curve
00000000011f4e50 s _go.itab.*crypto/elliptic.nistCurve[*crypto/elliptic/internal/nistec.P224Point],crypto/elliptic.Curve
00000000011f4e98 s _go.itab.*crypto/elliptic.nistCurve[*crypto/elliptic/internal/nistec.P224Point],crypto/elliptic.Curve
00000000011f4ee0 s _go.itab.*crypto/elliptic.nistCurve[*crypto/elliptic/internal/nistec.P224Point],crypto/elliptic.Curve

That's not good. We require that itabs be unique, so that we can type switch using equality of itabs.

@randall77
Copy link
Contributor

@randall77 randall77 commented Oct 30, 2021

The linker is not deduplicating the itabs because their _type fields are different.
The 4 types are

1190f40
1191020
1191100
11911e0

I'm not sure yet why the types aren't being deduplicated. They have the same name, and they look identical except for the ptrToThis fields.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 30, 2021

Change https://golang.org/cl/360134 mentions this issue: cmd/compile: add test for 49241

@randall77
Copy link
Contributor

@randall77 randall77 commented Oct 30, 2021

I've made a small repro that I think is the same underlying cause.
I think we need type deduplication in the linker.
It doesn't look like we have such a thing currently. Kind of surprises me that it hasn't been needed to date. Yet tests without generics that should provoke the same problem ... don't. More investigation needed.

@golang golang locked and limited conversation to collaborators Oct 31, 2021
@golang golang unlocked this conversation Oct 31, 2021
@golang golang locked and limited conversation to collaborators Oct 31, 2021
@golang golang unlocked this conversation Oct 31, 2021
@FiloSottile
Copy link
Contributor Author

@FiloSottile FiloSottile commented Oct 31, 2021

@rogpeppe prepared a self-contained testscript for this issue in #49245.

go run ./pkg1

-- go.mod --
module m

go 1.18
-- pkg1/main.go --
package main

import (
	"fmt"
	"os"
	"reflect"

	"m/pkg2"
	"m/pkg3"
)

func main() {
	i1 := pkg3.GetI()
	i2 := pkg2.Inst
	instEqual := i1 == i2
	if !instEqual {
		v1, v2 := reflect.ValueOf(i1), reflect.ValueOf(i2)
		ptrEqual := v1.Pointer() == v2.Pointer()
		typeEqual := v1.Type() == v2.Type()
		if !ptrEqual {
			fmt.Printf("pointer values are unequal\n")
		}
		if !typeEqual {
			fmt.Printf("types are not equal\n\t%v\n\t%v\n", v1.Type(), v2.Type())
		}
		os.Exit(1)
	}
}

-- pkg2/x.go --
package pkg2

import "m/pkg3"

var Inst = pkg3.GetI()

-- pkg3/x.go --
package pkg3

type I interface{}

type impl[P any] struct{
	_ byte
}

var instance = &impl[int]{}

func GetI() I {
	return instance
}
> go run ./pkg1
[stdout]
types are not equal
	*pkg3.impl[int]
	*pkg3.impl[int]

[stderr]
exit status 1

@FiloSottile FiloSottile changed the title cmd/compile: switch over wrapped singletons not matching cmd/compile: identical generic types compare unequal Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generics NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

4 participants