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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky 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 The path to resolution is known, but the work has not been done. label Apr 5, 2018
@mdempsky mdempsky added this to the Go1.11 milestone Apr 5, 2018
@mdempsky
Copy link
Member Author

mdempsky 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
Copy link
Contributor

rsc 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
Copy link
Member Author

mdempsky 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
Copy link

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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
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
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>
@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.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants