cmd/compile: sort method sets using package height

Also, when statically building itabs, compare *types.Sym instead of
name alone so that method sets with duplicate non-exported methods are
handled correctly.

Fixes #24693.

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

src/cmd/compile/internal/gc/reflect.go

@@ -1390,6 +1390,8 @@ func genfun(t, it *types.Type) []*obj.LSym {
 	sigs := imethods(it)
 	methods := methods(t)
 	out := make([]*obj.LSym, 0, len(sigs))
+	// TODO(mdempsky): Short circuit before calling methods(t)?
+	// See discussion on CL 105039.
 	if len(sigs) == 0 {
 		return nil
 	}
@@ -1397,7 +1399,7 @@ func genfun(t, it *types.Type) []*obj.LSym {
 	// both sigs and methods are sorted by name,
 	// so we can find the intersect in a single pass
 	for _, m := range methods {
-		if m.name.Name == sigs[0].name.Name {
+		if m.name == sigs[0].name {
 			out = append(out, m.isym.Linksym())
 			sigs = sigs[1:]
 			if len(sigs) == 0 {
@@ -1406,6 +1408,10 @@ func genfun(t, it *types.Type) []*obj.LSym {
 		}
 	}
 
+	if len(sigs) != 0 {
+		Fatalf("incomplete itab")
+	}
+
 	return out
 }
 

src/cmd/compile/internal/types/sym.go

@@ -80,7 +80,14 @@ func (sym *Sym) Linksym() *obj.LSym {
 // Less reports whether symbol a is ordered before symbol b.
 //
 // Symbols are ordered exported before non-exported, then by name, and
-// finally (for non-exported symbols) by package path.
+// finally (for non-exported symbols) by package height and path.
+//
+// Ordering by package height is necessary to establish a consistent
+// ordering for non-exported names with the same spelling but from
+// different packages. We don't necessarily know the path for the
+// package being compiled, but by definition it will have a height
+// greater than any other packages seen within the compilation unit.
+// For more background, see issue #24693.
 func (a *Sym) Less(b *Sym) bool {
 	if a == b {
 		return false
@@ -93,11 +100,15 @@ func (a *Sym) Less(b *Sym) bool {
 		return ea
 	}
 
-	// Order by name and then (for non-exported names) by package.
+	// Order by name and then (for non-exported names) by package
+	// height and path.
 	if a.Name != b.Name {
 		return a.Name < b.Name
 	}
 	if !ea {
+		if a.Pkg.Height != b.Pkg.Height {
+			return a.Pkg.Height < b.Pkg.Height
+		}
 		return a.Pkg.Path < b.Pkg.Path
 	}
 	return false

test/fixedbugs/issue24693.dir/a.go

@@ -0,0 +1,11 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package a
+
+type T struct{}
+
+func (T) m() { println("FAIL") }
+
+type I interface{ m() }

test/fixedbugs/issue24693.dir/b.go

@@ -0,0 +1,38 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package b
+
+import "./a"
+
+type T struct{ a.T }
+
+func (T) m() { println("ok") }
+
+// The compiler used to not pay attention to package for non-exported
+// methods when statically constructing itabs. The consequence of this
+// was that the call to b.F1(b.T{}) in c.go would create an itab using
+// a.T.m instead of b.T.m.
+func F1(i interface{ m() }) { i.m() }
+
+// The interface method calling convention depends on interface method
+// sets being sorted in the same order across compilation units.  In
+// the test case below, at the call to b.F2(b.T{}) in c.go, the
+// interface method set is sorted as { a.m(); b.m() }.
+//
+// However, while compiling package b, its package path is set to "",
+// so the code produced for F2 uses { b.m(); a.m() } as the method set
+// order. So again, it ends up calling the wrong method.
+//
+// Also, this function is marked noinline because it's critical to the
+// test that the interface method call happen in this compilation
+// unit, and the itab construction happens in c.go.
+//
+//go:noinline
+func F2(i interface {
+	m()
+	a.I // embeds m() from package a
+}) {
+	i.m()
+}

test/fixedbugs/issue24693.dir/c.go

@@ -0,0 +1,12 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import "./b"
+
+func main() {
+	b.F1(b.T{})
+	b.F2(b.T{})
+}

test/fixedbugs/issue24693.go

@@ -0,0 +1,7 @@
+// rundir
+
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package ignored

test/fixedbugs/issue24693.out

@@ -0,0 +1,2 @@
+ok
+ok