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

go/internal/gccgoimporter: nil type when decoding a type alias #27856

Closed
alandonovan opened this issue Sep 25, 2018 · 10 comments
Closed

go/internal/gccgoimporter: nil type when decoding a type alias #27856

alandonovan opened this issue Sep 25, 2018 · 10 comments
Labels
FrozenDueToAge NeedsFix
Milestone

Comments

@alandonovan
Copy link
Contributor

@alandonovan alandonovan commented Sep 25, 2018

The export data produced by gccgo for this Go package, when decoded by the gccgoexportdata package in x/tools, contains a nil type for the type alias, E. I'm not sure if the problem is on the encoding or decoding side, but it is breaking the blaze build for gccgo (b/116516363).

// test/lib.go
package lib

type M struct {
        E E
}
type F struct {
        _ *M
}
type E = F

I have attached a standalone Go program containing the export data file produced by Blaze (as a string literal) to exercise the decode side:
gccgoimport.txt

Excerpt:

	r, err := gccgoexportdata.NewReader(strings.NewReader(data))
	if err != nil {
		log.Fatal(err)
	}

	imports := make(map[string]*types.Package)
	pkg, err := gccgoexportdata.Read(r, nil, imports, "lib")
	if err != nil {
		log.Fatal(err)
	}

	E := pkg.Scope().Lookup("M").Type().Underlying().(*types.Struct).Field(0)
	fmt.Println(E, E.Type()) // "field E", <nil>

I'm not sure how to isolate the encode side, outside of Blaze.

@gopherbot gopherbot added this to the Gccgo milestone Sep 25, 2018
@alandonovan
Copy link
Contributor Author

@alandonovan alandonovan commented Sep 25, 2018

cc @ianlancetaylor, @paranoiacblack

@thanm
Copy link
Contributor

@thanm thanm commented Sep 25, 2018

I can take a look at this. Looking at the export data in the object file I see

package lib;
pkgpath test;
type <type 1 "E" = <type 2 "F" <type 3 struct { .test._ <type 4 *<type 5 "M" <type 6 struct { E <type 1>; }>>>; }>>>;
type <type 2>;
type <type 5>;

which as far as I can tell includes the alias 'E' in the right way (assuming that I'm not missing some other problem). I'll poke around on the decode side.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 25, 2018

@thanm Please let me know if you want me to look at this - I'm leaving this alone unless I hear otherwise from you.

@thanm
Copy link
Contributor

@thanm thanm commented Sep 25, 2018

OK, I am fairly certain this is a bug in the decoder/parser (e.g. gccgoimporter/parser.go).

Here is the type of interest:

type <type 1 "E" = <type 2 "F" <type 3 struct { .test._ <type 4 *<type 5 "M" <type 6 struct { E <type 1>; }>>>; }>>>;

parseNamedType() has code that looks for the '=' token (indicates an alias) in parser.go line 390:

	if p.tok == '=' {
		// type alias
		p.next()
		typ := p.parseType(pkg)  // line 393
		if obj := scope.Lookup(name); obj != nil {
			typ = obj.Type() // use previously imported type
			if typ == nil {
				p.errorf("%v (type alias) used in cycle", obj)
			}
		} else {
			obj = types.NewTypeName(token.NoPos, pkg, name, typ)
			scope.Insert(obj)
		}
		p.typeMap[n] = typ
		return typ
	}

The problem here is that the parseType call on line 393 (marked above) can include types that refer to the index of the alias type -- at that point there won't yet be an entry in typeMap for the type. There has to be some sort of entry in the map for "n" before this call is made.

@thanm
Copy link
Contributor

@thanm thanm commented Sep 25, 2018

@griesemer looks like our comments crossed. Yes, if you could please look at it that would be great.

@griesemer griesemer added the NeedsFix label Sep 25, 2018
@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 25, 2018

For the record, to get the export data (.gox file) outside blaze, for a file x.go:

gccgo -c x.go
objcopy -j .go_export x.o x.gox

with x.gox containing the export data.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 25, 2018

Definitively a parser bug (which was not written with aliases in mind...). But the problem is on line parser.go:206 in parser.parseField, when reading the type for field E of struct M. That type E is not yet set up, and thus is nil.

@griesemer griesemer changed the title gccgo: nil type when decoding a type alias go/internal/gccgoimporter: nil type when decoding a type alias Sep 26, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 26, 2018

Change https://golang.org/cl/137857 mentions this issue: go/internal/gccgoimporter: fix updating of "forward declared" types

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 27, 2018

The fix above is for the std library. This needs to be back-ported to x/tools afterwards.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 27, 2018

Change https://golang.org/cl/137995 mentions this issue: go/internal/gccgoimporter: port recent changes from stdlib version

gopherbot pushed a commit to golang/tools that referenced this issue Sep 27, 2018
This CL brings over the following changes from the std lib:

	https://golang.org/cl/137857
	https://golang.org/cl/137935
	https://golang.org/cl/137975

There are no further code changes except that the importer test
cases are split between importer_test.go and importer19_test.go
to support multiple Go versions.

Updates golang/go#27856.

Change-Id: I625def738c22c24c6659af37c3871038fdd8b981
Reviewed-on: https://go-review.googlesource.com/137995
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Sep 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix
Projects
None yet
Development

No branches or pull requests

4 participants