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: possible miscompilation comparing interface to concrete value #44051

Open
josharian opened this issue Feb 1, 2021 · 9 comments
Open

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Feb 1, 2021

I noticed this while modifying package context in CL 288193.

To reproduce:

git fetch https://go.googlesource.com/go refs/changes/93/288193/2 && git checkout FETCH_HEAD

Note that the package context tests pass.

Now at context/context.go:306, replace:

pdone, _ := p.done.Load().(chan struct{})
if pdone != done {

with

if p.done.Load() != done {

The tests fail.

Those two formulations should be identical (given that p.done.Load() is a chan struct{}, which it always is).

That suggests to me that we might be miscompiling the comparison p.done.Load() != done.

I've been unable to reproduce in a more minimized form, although I haven't tried very hard.

Tentatively marking as release blocker until diagnosed.

cc @mdempsky @cuonglm @randall77

@josharian josharian added this to the Go1.16 milestone Feb 1, 2021
@randall77
Copy link
Contributor

@randall77 randall77 commented Feb 1, 2021

Here's a simple reproducer:

package main

var c = make(chan int)

func main() {
	println(test1())
	println(test2())
	println(test3())
}

func test1() bool {
	x := (<-chan int)(c)
	y := f().(chan int)
	return x == y
}
func test2() bool {
	x := (<-chan int)(c)
	return x == f()
}

func test3() bool {
	x := c
	y := (<-chan int)(c)
	return x == y
}

//go:noinline
func f() interface{} {
	return c
}

This prints true false true.

I'm actually not sure what the right behavior actually is. The fact that directional channels exist at compile time but don't really exist at runtime is probably part of this.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 1, 2021

It seems like the spec's definition of equality is non-transitive for channels stored in interfaces:

package main

func main() {
	a := make(chan int)
	b := (<-chan int)(a)
	c := interface{}(b)

	println(a == b, b == c, a == c)
}

I think this behavior is surprising but correct per https://golang.org/ref/spec#Comparison_operators. "A value x of non-interface type X and a value t of interface type T are comparable when values of type X are comparable and X implements T. They are equal if t's dynamic type is identical to X and t's dynamic value is equal to x." chan int and <-chan int are different types.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Feb 1, 2021

The spec says

Interface values are comparable. Two interface values are equal if they have identical dynamic types and equal dynamic values or if both have value nil.
A value x of non-interface type X and a value t of interface type T are comparable when values of type X are comparable and X implements T. They are equal if t's dynamic type is identical to X and t's dynamic value is equal to x.

and

Two channel types are identical if they have identical element types and the same direction.

So, when comparing as interface values, the direction matters, and test2 returns false correctly.

When comparing as concrete channel types, the spec says

Channel values are comparable. Two channel values are equal if they were created by the same call to make or if both have value nil.

without mentioning direction. So it seems reasonable for them to be equal.

I agree this is confusing...

@josharian
Copy link
Contributor Author

@josharian josharian commented Feb 1, 2021

It seems like the spec's definition of equality is non-transitive for channels stored in interfaces

That's horrifying.

@randall77
Copy link
Contributor

@randall77 randall77 commented Feb 1, 2021

Maybe there's something to do here, but not for 1.16.

@randall77 randall77 removed this from the Go1.16 milestone Feb 1, 2021
@randall77 randall77 added this to the Unplanned milestone Feb 1, 2021
@randall77
Copy link
Contributor

@randall77 randall77 commented Feb 1, 2021

This also happens for non-channel cases:

package main

func main() {
	x := new(int)
	type P *int
	p := P(x)
	println(x == p)
	println((interface{})(x) == (interface{})(p))
}

prints true false.

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Feb 1, 2021

@cherrymui

without mentioning direction. So it seems reasonable for them to be equal.

So it seems to me that the code in runtime/type.go:typesEqual are wrong, as it checks the channel direction for equality:

return ct.dir == cv.dir && typesEqual(ct.elem, cv.elem, seen)

@randall77
Copy link
Contributor

@randall77 randall77 commented Feb 1, 2021

@cuonglm That quote was about channel values, not channel types. Channel type comparisons do take into account direction. Channel value comparisons do not (not sure how that would work anyway).

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 1, 2021

It seems like the spec's definition of equality is non-transitive for channels stored in interfaces

That's horrifying.

That may be, but equality is defined in terms of assignability (https://golang.org/ref/spec#Comparison_operators):

In any comparison, the first operand must be assignable to the type of the second operand, or vice versa.

And assignability is already not transitive — both because of the “at least one of V or T is not a defined type” restrictions, and because a type assignable to an interface may also be assignable to a type that does not implement that interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants