go.tools/go/gccgoimporter, go.tools/go/types: keep trac...

go.tools/go/gccgoimporter: keep track of package and import priority

Clients such as compilers need this information in order
to correctly link against imported packages.

This also adds support for the condensed import data format
where the priority information is stored as a suffix of the
condensed import data, as well as support for archive files.

[gri, 2014-03-21 23:19:59]

I like to discuss this change before we go any further (sometimes next
week?).

For one, the go.tools repo is frozen at this point and no API changes can
go in.

Secondly, at least in the current form this appears highly gccgo-specific,
while go/types is really platform-independent. Perhaps we can make a point
that we need this information for all platforms, but that we also need to
implement if for all platforms.

- gri


On Fri, Mar 21, 2014 at 2:06 AM, <pcc@google.com> wrote:

> Reviewers: gri,
>
> Description:
> go.tools/go/gccgoimporter, go.tools/go/types: keep track of package and
> import priority
>
> Clients such as compilers need this information in order
> to correctly link against imported packages.
>
> Please review this at https://codereview.appspot.com/78740043/
>
> Affected files (+132, -31 lines):
>   M go/gccgoimporter/importer_test.go
>   M go/gccgoimporter/parser.go
>   A go/gccgoimporter/testdata/imports.go
>   A go/gccgoimporter/testdata/imports.gox
>   M go/types/package.go
>
>
>

[pcc, 2014-04-11 06:57:24]

I've updated this CL with the changes we discussed.

I hope it isn't too premature to teach the gccgoimporter about the condensed
import format before gccgo can actually export in this format, as I've already
taught llgo to export in this format and it seems to work well. We can consider
the format to be unstable until it lands in gccgo (and I will update llgo to
match).

I also taught godex to print priority data. This is probably the least ugly way
I could think of to do it.

Tested by building an object file with (a modified version of) llgo and
inspecting it with codex.

[gri, 2014-04-14 23:22:46]

Apologies for the delay. Some initial comments; more to come.

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/importer.go
File go/gccgoimporter/importer.go (right):

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/importer....
go/gccgoimporter/importer.go:25: Function string // name of init function
s/Function/InitFunc/ ?

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/importer....
go/gccgoimporter/importer.go:32: // This is based on the maximum depth of the
package's dependency graph,
s/,/;/

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/importer....
go/gccgoimporter/importer.go:33: // hence is guaranteed to be greater than that
of its dependencies.
s/hence/it/

(it doesn't follow from the prior; it's only possibly because of it)

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/importer....
go/gccgoimporter/importer.go:92: if string(magic[:]) == gccgov1Magic ||
string(magic[:]) == goimporterMagic {
if m := string(magic[:]); m == ... || m == ... {

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/parser.go
File go/gccgoimporter/parser.go (right):

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/parser.go...
go/gccgoimporter/parser.go:733: p.expect(scanner.Ident)
return ?

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/parser.go...
go/gccgoimporter/parser.go:755: p.scanner.Mode &^= scanner.ScanInts |
scanner.ScanFloats
perhaps:

defer func(mode uint) {
   p.scanner.Mode = mode
}(p.scanner.Mode)
p.scanner.Mode &^= scanner.ScanInts | scanner.ScanFloats

and get rid of the inverse assignment below

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/parser.go...
go/gccgoimporter/parser.go:843: func (p *parser) parseInitData() {
I don't see where this is called.

[pcc, 2014-04-15 01:16:00]

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/importer.go
File go/gccgoimporter/importer.go (right):

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/importer....
go/gccgoimporter/importer.go:25: Function string // name of init function
On 2014/04/14 23:22:46, gri wrote:
> s/Function/InitFunc/ ?

Done.

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/importer....
go/gccgoimporter/importer.go:32: // This is based on the maximum depth of the
package's dependency graph,
On 2014/04/14 23:22:46, gri wrote:
> s/,/;/

Done.

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/importer....
go/gccgoimporter/importer.go:33: // hence is guaranteed to be greater than that
of its dependencies.
On 2014/04/14 23:22:46, gri wrote:
> s/hence/it/
> 
> (it doesn't follow from the prior; it's only possibly because of it)

Done. This originally said something like "This is the maximum depth of the
dependency graph", but I don't know if that is actually true -- it monotonically
increases with the depth, which is the important part, but that's probably too
much detail for here.

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/importer....
go/gccgoimporter/importer.go:92: if string(magic[:]) == gccgov1Magic ||
string(magic[:]) == goimporterMagic {
On 2014/04/14 23:22:46, gri wrote:
> if m := string(magic[:]); m == ... || m == ... {

This now uses a switch statement thanks to the new archive support.

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/parser.go
File go/gccgoimporter/parser.go (right):

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/parser.go...
go/gccgoimporter/parser.go:733: p.expect(scanner.Ident)
On 2014/04/14 23:22:46, gri wrote:
> return ?

The call to expect will panic, so I don't think a return is needed. I've added a
comment clarifying this.

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/parser.go...
go/gccgoimporter/parser.go:755: p.scanner.Mode &^= scanner.ScanInts |
scanner.ScanFloats
On 2014/04/14 23:22:46, gri wrote:
> perhaps:
> 
> defer func(mode uint) {
>    p.scanner.Mode = mode
> }(p.scanner.Mode)
> p.scanner.Mode &^= scanner.ScanInts | scanner.ScanFloats
> 
> and get rid of the inverse assignment below

Done.

https://codereview.appspot.com/78740043/diff/40001/go/gccgoimporter/parser.go...
go/gccgoimporter/parser.go:843: func (p *parser) parseInitData() {
On 2014/04/14 23:22:46, gri wrote:
> I don't see where this is called.

From importer.go line 164 (or line 188 now).

[gri, 2014-04-15 17:46:15]

https://codereview.appspot.com/78740043/diff/60001/cmd/godex/gccgo.go
File cmd/godex/gccgo.go (left):

https://codereview.appspot.com/78740043/diff/60001/cmd/godex/gccgo.go#oldcode31
cmd/godex/gccgo.go:31: // importer for gccgo using condensed export format
(experimental)
Why did you remove this? I need this and the code below for the new import
format.

https://codereview.appspot.com/78740043/diff/60001/cmd/godex/gccgo.go
File cmd/godex/gccgo.go (right):

https://codereview.appspot.com/78740043/diff/60001/cmd/godex/gccgo.go#newcode28
cmd/godex/gccgo.go:28: func (p *printer) printGccgoExtra(pkg *types.Package) {
This is not really fitting the current scheme:

godex is written such that new importers register themselves, and printing is
generic. So far, one could leave away any of the compiler-specific files and
godex would still build and run. That's not the case anymore because now this
method is required.

The better way is to register also the printer.

Leave this for now, I will change it once we have something ok'ed.

[pcc, 2014-04-15 17:57:32]

https://codereview.appspot.com/78740043/diff/60001/cmd/godex/gccgo.go
File cmd/godex/gccgo.go (left):

https://codereview.appspot.com/78740043/diff/60001/cmd/godex/gccgo.go#oldcode31
cmd/godex/gccgo.go:31: // importer for gccgo using condensed export format
(experimental)
On 2014/04/15 17:46:15, gri wrote:
> Why did you remove this? I need this and the code below for the new import
> format.

It is redundant now that the gccgoimporter supports the new import format
directly.

https://codereview.appspot.com/78740043/diff/60001/cmd/godex/gccgo.go
File cmd/godex/gccgo.go (right):

https://codereview.appspot.com/78740043/diff/60001/cmd/godex/gccgo.go#newcode28
cmd/godex/gccgo.go:28: func (p *printer) printGccgoExtra(pkg *types.Package) {
On 2014/04/15 17:46:15, gri wrote:
> This is not really fitting the current scheme:
> 
> godex is written such that new importers register themselves, and printing is
> generic. So far, one could leave away any of the compiler-specific files and
> godex would still build and run. That's not the case anymore because now this
> method is required.
> 
> The better way is to register also the printer.
> 
> Leave this for now, I will change it once we have something ok'ed.

Sure. Since there are only a handful of importers at the moment, I figured this
would be ok for now.

[gri, 2014-04-15 18:17:20]

https://codereview.appspot.com/78740043/diff/60001/cmd/godex/gccgo.go
File cmd/godex/gccgo.go (left):

https://codereview.appspot.com/78740043/diff/60001/cmd/godex/gccgo.go#oldcode31
cmd/godex/gccgo.go:31: // importer for gccgo using condensed export format
(experimental)
On 2014/04/15 17:57:33, pcc wrote:
> On 2014/04/15 17:46:15, gri wrote:
> > Why did you remove this? I need this and the code below for the new import
> > format.
> 
> It is redundant now that the gccgoimporter supports the new import format
> directly.

Ah. I missed that.

https://codereview.appspot.com/78740043/diff/60001/go/importer/import.go
File go/importer/import.go (right):

https://codereview.appspot.com/78740043/diff/60001/go/importer/import.go#newc...
go/importer/import.go:25: func ImportData(imports map[string]*types.Package,
data []byte) (*types.Package, error) {
I think instead of the new function below, we should make ImportData make to
look more like a reader; and just have one function again:

// ImportData imports a package from the serialized package data.
// It returns the number of data bytes consumed and the imported
// package, or an error.
func ImportData(imports map[string]*types.Package, data []byte) (int,
*types.Package, error)

A caller can then do

n, pkg, err := ImportData(...)
...
// suffix is data[n:]

[pcc, 2014-04-15 19:17:25]

https://codereview.appspot.com/78740043/diff/60001/go/importer/import.go
File go/importer/import.go (right):

https://codereview.appspot.com/78740043/diff/60001/go/importer/import.go#newc...
go/importer/import.go:25: func ImportData(imports map[string]*types.Package,
data []byte) (*types.Package, error) {
On 2014/04/15 18:17:21, gri wrote:
> I think instead of the new function below, we should make ImportData make to
> look more like a reader; and just have one function again:
> 
> // ImportData imports a package from the serialized package data.
> // It returns the number of data bytes consumed and the imported
> // package, or an error.
> func ImportData(imports map[string]*types.Package, data []byte) (int,
> *types.Package, error)
> 
> A caller can then do
> 
> n, pkg, err := ImportData(...)
> ...
> // suffix is data[n:]

Yes, that would be much more sensible. Done.

[pcc, 2014-05-28 18:30:47]

On 2014/04/15 19:17:25, pcc wrote:
> https://codereview.appspot.com/78740043/diff/60001/go/importer/import.go
> File go/importer/import.go (right):
> 
>
https://codereview.appspot.com/78740043/diff/60001/go/importer/import.go#newc...
> go/importer/import.go:25: func ImportData(imports map[string]*types.Package,
> data []byte) (*types.Package, error) {
> On 2014/04/15 18:17:21, gri wrote:
> > I think instead of the new function below, we should make ImportData make to
> > look more like a reader; and just have one function again:
> > 
> > // ImportData imports a package from the serialized package data.
> > // It returns the number of data bytes consumed and the imported
> > // package, or an error.
> > func ImportData(imports map[string]*types.Package, data []byte) (int,
> > *types.Package, error)
> > 
> > A caller can then do
> > 
> > n, pkg, err := ImportData(...)
> > ...
> > // suffix is data[n:]
> 
> Yes, that would be much more sensible. Done.

Ping.

[gri, 2014-05-28 20:22:14]

LGTM

(for submission after the 1.3 release)

[gri, 2014-06-17 17:56:49]

*** Submitted as
https://code.google.com/p/go/source/detail?r=c6b7985fb5c3&repo=tools ***

go.tools/go/gccgoimporter: keep track of package and import priority

Clients such as compilers need this information in order
to correctly link against imported packages.

This also adds support for the condensed import data format
where the priority information is stored as a suffix of the
condensed import data, as well as support for archive files.

LGTM=gri
R=gri
CC=golang-codereviews, iant
https://codereview.appspot.com/78740043

Committer: Robert Griesemer <gri@golang.org>