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.

Sign up for GitHub

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

Jump to bottom

gccgo: incorrect export data for duplicate interface #30659

Closed
thanm opened this issue Mar 7, 2019 · 5 comments
Closed

gccgo: incorrect export data for duplicate interface #30659

thanm opened this issue Mar 7, 2019 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Gccgo

Comments

@thanm
Copy link
Contributor

thanm commented Mar 7, 2019

What version of Go are you using (go version)?

$ go version
go version devel +7d13c1b887 Fri Mar 1 13:46:39 2019 -0500 linux/amd64
$ gccgo --version
gccgo (GCC) 9.0.1 20190304 (experimental)

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

linux/amd64

go env Output
$ go env
GOARCH="amd64"
...
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"

What did you do?

Create three packages, "usetm", "tm" and "tm/another/tm".

First package $GOPATH/src/tm/tm.go:

package tm

import (
	"tm/another/tm"
)

type LocalM interface {
	tm.LocalM
}

func NewM(x int) LocalM {
	return tm.NewM(x)
}

Second package $GOPATH/src/tm/another/tm/tm.go:

package tm

type LocalM interface {
	Foo(x int) int
}

type CM struct {
	q int
}

func (cm *CM) Foo(x int) int {
	return x
}

func NewM(x int) LocalM {
	return &CM{q: x}
}


Third package $GOPATH/src/usetm/usetm.go:

package usetm

import (
	"tm"
)

func Blah() {
	tm.NewM(3)
}

Then cd to $GOPATH/usetm and issue a build with gccgo:

$ go build -compiler gccgo .

What did you expect to see?

Successful build

What did you see instead?

Build fails with:

$ go build -compiler gccgo .
# usetm
./usetm.go:4:4: error: inherited interface loop
    4 |  "tm"
      |    ^
./usetm.go:4:4: error: invalid recursive interface
$

Looking at the export data for GOPATH/src/tm:

Excerpt from $GOPATH/pkg/gccgo_linux_amd64/libtm.a

v3;
package tm
pkgpath tm
import tm tm/another/tm "tm/another/tm"
init ... 
init_graph 0 1 0 ... 
types 4 2 25 33 39
type 1 "LocalM" <type 2>
type 2 interface { ? <type 3>; }
type 3 "tm/another/tm.LocalM" <type 2>
func NewM (x <type -11>) <type 1>
checksum E8DA5660A96FEDE5919157B0A4433F5FDF8C99A4

Note the messed up types (cycle involving 2 and 3). So the problem is really with the export data, not with the error itself (compiler it correct to report a cycle, since that's what the bogus export data says).
@thanm thanm added this to the Gccgo milestone Mar 7, 2019
@thanm thanm self-assigned this Mar 7, 2019
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 8, 2019
@thanm
Copy link
Contributor Author

thanm commented Mar 10, 2019

This problem seems to be happening fairly late in the game -- from
spending time in the debugger I don't see anything suspicious until it
comes time to emit types into the export data.

There are four types of interest:

T1 named type "LocalM" defined in /src/tm, points to T1
T2 interface type with embedded interface T3
T3 named type "LocalM" defined in /src/tm/another/tm, points to T4
T4 interface type with method Foo

During Export::prepare_types, the traversal visits all four types,
however when it visits T4 and Export::set_type_index() is called, the
lookup into "type_refs" decides that T4 is identical to T2 (according
to the criteria set up for Type_alias_identical in export.cc), so it
returns the type index for T2 instead of adding a new entry. This
effectively makes the second LocalM an alias of the first LocalM,
which is what introduces the cycle in the type graph.

When T2 is exported, the code in Interface_type::do_export
walks the list of methods and encounters the embedded interface, it
goes ahead and emits the reference to T3, hence the problem.

My feeling at this point is that for the purposes of the exporter, it
would be better to consider these types non-identical. I will see about
adding a flag to Types_are_identical to do a more nuanced comparison
(in particular, to compare package name/path in this case).

@ianlancetaylor
Copy link
Contributor

Interesting. One way to look at the problem is to observe that Interface_type::do_export works on parse_methods_, which includes references to embedded types, while Interface_type::is_identical works on all_methods_, in which all methods have been pulled in from the embedded types. If Interface_type::do_export used all_methods_, then presumably there would be no problem. But I doubt we want to do that. So I think you're right that we need Types::are_identical flag to compare interface types by comparing parse_methods_ rather than all_methods_, or, in other words, to compare based on embedded interface types rather than only on the final method set.

@gopherbot
Copy link

Change https://golang.org/cl/166638 mentions this issue: compiler: compare package when indexing interface types for export

@gopherbot
Copy link

gopherbot commented Mar 11, 2019 •
edited by thanm

Change https://golang.org/cl/166657 mentions this issue: test: add new test for gccgo compilation problem [Note: this change abandoned in favor of https://golang.org/cl/166917]

@gopherbot
Copy link

Change https://golang.org/cl/166917 mentions this issue: test: add new test for gccgo compilation problem

gopherbot pushed a commit that referenced this issue Mar 12, 2019
@thanm @bradfitz
test: add new test for gccgo compilation problem
30cc8a4 
New test for issue 30659 (compilation error due to bad
export data).

Updates #30659.

Change-Id: I2541ee3c379e5b22033fea66bb4ebaf720cc5e1f
Reviewed-on: https://go-review.googlesource.com/c/go/+/166917
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
kraj pushed a commit to kraj/gcc that referenced this issue Mar 13, 2019
compiler: compare parse methods when indexing interface types for ex…
52e9d4d 
…port

    
    This change fixes a bug in which two interface types were being
    incorrectly commoned (considered identical) in the initial stages of
    writing out types to export data. The indexer does a walk to collect
    candidates for export, inserting types into a table to eliminate
    duplicates; as part of this process a local interface type T1 was
    being commoned with a different interface type T2. This caused a cycle
    in the exported type graph due to the way embedded interfaces are
    handled.
    
    The fix was to add a new flag to the Type::is_identical utility
    routine to request that interface type comparison be done by examining
    the original parse methods, as opposed to the expanded method set,
    then use the new flag when creating the hash map for the exporter.
    
    Fixes golang/go#30659.
    
    Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/166638


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@269634 138bc75d-0d04-0410-961f-82ee72b054a4
@golang golang locked and limited conversation to collaborators Mar 12, 2020
@rsc rsc unassigned thanm Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Gccgo
Development

No branches or pull requests
3 participants
@ianlancetaylor @gopherbot @thanm