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

cmd/compile: crash during import #15548

Closed
griesemer opened this issue May 4, 2016 · 6 comments
Closed

cmd/compile: crash during import #15548

griesemer opened this issue May 4, 2016 · 6 comments
Labels
FrozenDueToAge
Milestone
Go1.7

Comments

@griesemer
Copy link
Contributor

Given the files
a.go:

package a

import (
    _ "./b"
    _ "./c"
)

b.go:

package b

import "./c"

func B(c.T) {}

c.go:

package c

type I0 interface {
    I1
}

type T struct {
    I1
}

type I1 interface {
    M(*T) // removing * makes crash go away
}

The compile

go tool compile c.go && go tool compile b.go && go tool compile a.go

dies in import:

a.go:5: inconsistent definition for type c.I1 during import
    interface { M(*c.T) } (in "/Users/gri/bug/b")
    interface { M(*<T>) } (in "/Users/gri/bug/c")
@griesemer griesemer added this to the Go1.7 milestone May 4, 2016
@griesemer griesemer self-assigned this May 4, 2016
@griesemer
Copy link
Contributor Author

cc @robpike

@robpike
Copy link
Contributor

robpike commented May 5, 2016

I0 isn't used anywhere. Can it be removed and still trigger the crash?

@mdempsky
Copy link
Member

mdempsky commented May 5, 2016

In a standard go workspace, the files would be laid out

a.go
b/b.go
b/c/c.go
c/c.go

So a.go's "./c" import would import a different package than b/b.go's "./c" import.

Does bimport need to know that when it's loading a package for "x/y" and it sees a reference to a package "./z", that we need to rewrite it to "x/y/z"?

@griesemer
Copy link
Contributor Author

griesemer commented May 5, 2016 •
edited

I believe the code is correct. The issue is the type equality check in
bimport.go is called before a type is fully set up (too early). It's really
just an assertion and not required unless we have a true bug in the
importer.

That is, the fix is to delay the check. I think if you comment out the
Fatal after the equality test all will work fine.

In a standard go workspace, the files would be laid out

a.go
b/b.go
b/c/c.go
c/c.go

So a.go's "./c" import would import a different package than b/b.go's
"./c" import.

Does bimport need to know that when it's loading a package for "x/y" and
it sees a reference to a package "./z", that we need to rewrite it to
"x/y/z"?

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#15548 (comment)

@gopherbot
Copy link

CL https://golang.org/cl/22803 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/22839 mentions this issue.

gopherbot pushed a commit that referenced this issue May 7, 2016
@griesemer
cmd/compile: add and enable (internal) option to only track named types
394ac81 
The new export format keeps track of all types that are exported.
If a type is seen that was exported before, only a reference to
that type is emitted. The importer maintains a list of all the
seen types and uses that list to resolve type references.

The existing compiler infrastructure's invariants assumes that
only named types are referred to before they are fully set up.
Referring to unnamed incomplete types causes problems. One of
the issues was #15548.

Added a new internal flag 'trackAllTypes' to enable/disable
this type tracking. With this change only named types are
tracked.

Verified that this fix also addresses #15548, even w/o the
prior fix for that issue (in fact that prior fix is turned
off if trackAllTypes is disabled because it's not needed).

The test for #15548 covers also this change.

For #15548.

Change-Id: Id0b3ff983629703d025a442823f99649fd728a56
Reviewed-on: https://go-review.googlesource.com/22839
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@golang golang locked and limited conversation to collaborators May 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.7
Development

No branches or pull requests
4 participants
@mdempsky @robpike @griesemer @gopherbot