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: static itab construction fails when a type has multiple non-exported methods #24693

Closed
mdempsky opened this issue Apr 5, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@mdempsky
Copy link
Member

commented Apr 5, 2018

In reflect.go:genfun, the code to walk method sets in parallel to compute itab entries doesn't take packages into account for finding the right non-exported method. This causes the code below to print FAIL instead of ok, because it selects U.m instead of T.m for the itab.

$ cat z.go
package z

type T int
func (T) m() { println("ok") }

func F(i interface{ m() }) { i.m() }

$ cat main.go
package main

import "./z"

type U struct{ z.T }
func (U) m() { println("FAIL") }

func main() { z.F(U{}) }

$ go tool compile z.go
$ go tool compile main.go
$ go tool link main.o
$ ./a.out
FAIL

@mdempsky mdempsky added the NeedsFix label Apr 5, 2018

@mdempsky mdempsky added this to the Go1.11 milestone Apr 5, 2018

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2018

Relatedly: when sorting method sets, we sort non-exported names by their package path. However, we always use "" for the local package path, so I think there are cases where a method set will be sorted inconsistently across different compilation units.

We could sort according to the -p flag. We could also make it an error when -p is not set, and sorting a method set would depend on it. This is ugly, but better than miscompiling code.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2018

We have not yet told people that -p is required when invoking the compiler. I'm not even sure if we do it ourselves consistently everywhere. Almost certainly not. If there's a way to avoid requiring -p that would be nice. That said I agree that the method tables are probably sorted wrong, which is bad. Maybe this is the last straw that pushes us over to requiring -p. Not sure.

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2018

Yeah, I'd really like to avoid requiring -p too. I expect we always set it for user builds, but I think it would be obnoxious for compiler development work. I also expect the regress tree would need some work.

Perhaps we could sort non-exported method symbols by package height (i.e., height of the package node in the package dependency DAG) and then package path.

The package under compilation is the only package whose path we might not know at the time. It will also always have a unique height within that compilation unit (i.e., at least 1 more than every other package within the CU, by definition), so this ordering should be insensitive to -p.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 5, 2018

Change https://golang.org/cl/105038 mentions this issue: cmd/compile: add package height to export data

@gopherbot

This comment has been minimized.

Copy link

commented Apr 5, 2018

Change https://golang.org/cl/105039 mentions this issue: cmd/compile: sort method sets using package height

@gopherbot

This comment has been minimized.

Copy link

commented Apr 9, 2018

Change https://golang.org/cl/105936 mentions this issue: cmd/compile: refactor symbol sorting logic

gopherbot pushed a commit that referenced this issue Apr 9, 2018

cmd/compile: refactor symbol sorting logic
This used to be duplicated in methcmp and siglt, because Sig used its
own representation for Syms. Instead, just use Syms, and add a
(*Sym).Less method that both methcmp and siglt can use.

Also, prune some impossible cases purportedly related to blank
methods: the Go spec disallows blank methods in interface method sets,
and addmethod drops blank methods without actually recording them in
the type's method set.

Passes toolstash-check.

Updates #24693.

Change-Id: I24e981659b68504d71518160486989a82505f513
Reviewed-on: https://go-review.googlesource.com/105936
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

gopherbot pushed a commit that referenced this issue Apr 9, 2018

cmd/compile: add package height to export data
A package's height is defined as the length of the longest import path
between itself and a leaf package (i.e., package with no imports).

We can't rely on knowing the path of the package being compiled, so
package height is useful for defining a package ordering.

Updates #24693.

Change-Id: I965162c440b6c5397db91b621ea0be7fa63881f1
Reviewed-on: https://go-review.googlesource.com/105038
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>

@gopherbot gopherbot closed this in 49ed4cb Apr 10, 2018

@golang golang locked and limited conversation to collaborators Apr 10, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.