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

cmd/go: missing entry in ImportMap for runtime/cgo [runtime.test] when -compiled is set #46462

Closed
dominikh opened this issue May 31, 2021 · 12 comments
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@dominikh
Copy link
Member

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

$ go version
go version devel go1.17-3b770f2ccb Sun May 30 17:47:50 2021 +0000 linux/amd64

Does this issue reproduce with the latest release?

No

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/dominikh/.cache/go-build"
GOENV="/home/dominikh/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/dominikh/prj/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/dominikh/prj"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/dominikh/prj/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/dominikh/prj/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.17-3b770f2ccb Sun May 30 17:47:50 2021 +0000"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/dominikh/prj/go/src/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1117114117=/tmp/go-build -gno-record-gcc-switches"

What did you do?

go list -json -compiled -test -deps runtime

What did you see?

The package with the ImportPath net [runtime.test] has an entry in Imports that reads runtime/cgo [runtime.test]. However, there is no corresponding entry in ImportMap.

It is my understanding that all non-identity imports should have a corresponding entry in ImportMap, and go/packages has matching logic for populating go/packages.Imports, a mapping from import paths to packages.

Note that this does not reproduce without the -compiled flag. It also does not reproduce in Go 1.16.4, but in that version, there is no package runtime/cgo [runtime.test] in the first place.

I am tentatively marking this as a release blocker because it is a regression and affects third party tools, but I may be overstepping here.

/cc @bcmills

@dominikh dominikh added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels May 31, 2021
@dominikh
Copy link
Member Author

dominikh commented May 31, 2021

972e883 seems to be the commit that introduces the runtime/cgo [runtime.test] package to the package graph. At this point, it's already missing from the ImportMap.

Edit: to clarify, the commit causes the problem to become visible, but it's probably not the underlying reason for the incomplete ImportMap.

@jayconrod
Copy link
Contributor

cc @matloob as well.

This might be working as intended, but I'm not completely sure. Would be good for others to confirm. Also I'm not at all sure if go/packages needs to be more aware of this.

It is my understanding that all non-identity imports should have a corresponding entry in ImportMap, and go/packages has matching logic for populating go/packages.Imports, a mapping from import paths to packages.

I think this is only true for mapping import strings from source files to packages passed to the linker (for example, after vendor resolution). It's not true for implicitly imported packages like runtime/cgo.

The runtime/cgo dependency is added to CompiledImports in Package.load. That addImport function makes no attempt to update ImportMap.

I think this only shows up with the -compiled flag because of this branch.

@mdempsky mdempsky added this to the Go1.17 milestone Jun 1, 2021
@mdempsky
Copy link
Member

mdempsky commented Jun 1, 2021

[Adding to the Go 1.17 milestone, at least until we can confirm whether this is working as intended.]

@dmitshur dmitshur added the GoCommand cmd/go label Jun 2, 2021
@gopherbot

This comment has been minimized.

@bcmills

This comment has been minimized.

@bcmills
Copy link
Contributor

bcmills commented Jun 3, 2021

I think this is only true for mapping import strings from source files to packages passed to the linker (for example, after vendor resolution). It's not true for implicitly imported packages like runtime/cgo.

I agree with Jay's assessment. The ImportMap reported by go list should map strings from import statements to the corresponding entities produced by go list. Since the source code does not itself contain the string import "runtime/cgo", the ImportMap does not need an entry for the path "runtime/cgo".

IMO the real bug here is the fact that there is no ImportMap entry for the import path "C". Ideally, instead of adding synthetic imports of the various cgo dependencies, we should add a synthetic C package that itself depends on the cgo dependencies, and add an ImportPath entry mapping "C" to that synthetic package. But that's more refactoring than we can afford to do at this point for Go 1.17.

@bcmills
Copy link
Contributor

bcmills commented Jun 3, 2021

@dominikh, can you give some more detail on why the entry in Imports without a corresponding entry in ImportMap causes a problem for you? Perhaps there is some other way we can address that problem.

@bcmills bcmills added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Jun 3, 2021
@dominikh
Copy link
Member Author

dominikh commented Jun 3, 2021

@bcmills Concretely I ran into this issue because of the logic in https://github.com/golang/tools/blob/7ac129f24ace03f9aa0fa6097083e825536363e9/go/packages/golist.go#L635-L654 which populates a mapping from import paths as they appear in source code to the Package being imported. In Staticcheck I use this mapping when type-checking a package to resolve imports.

Concretely, I am currently getting the following error in Staticcheck when checking std with Go tip:

$ ./staticcheck -checks=none std
/home/dominikh/.cache/go-build/44/44c013685155a782dca631cad53734c5e4bdb7c6aa82272a1f1ceb8e2925e6fe-d:9:10: could not import runtime/cgo (trying to import "runtime/cgo" in the context of "net [runtime.test]" returned nil PackageSpec) (compile)

The file in question is generated by cmd/cgo and starts like this:

//go:cgo_ldflag "-g"
//go:cgo_ldflag "-O2"
// Code generated by cmd/cgo; DO NOT EDIT.

package net

import "unsafe"

import _ "runtime/cgo"

Because of the logic in go/packages, there is no mapping from runtime/cgo to any package.

Now, this might well be a bug in go/packages instead and someone might have to come up with different logic. But I disagree with the notion that "Since the source code does not itself contain the string". No, the code as written by the user does not in fact import runtime/cgo. However, that is not the input to the compiler or linker, either. First, cmd/cgo preprocesses the user's code, and the result of that very much imports runtime/cgo. That code is reflected in CompiledGoFiles. IMO it would make sense for ImportMap to match CompiledGoFiles, not GoFiles.

@bcmills
Copy link
Contributor

bcmills commented Jun 3, 2021

Ahh, I see what you mean. The -compiled flag causes the source code (reported in CompiledGoFiles) to actually contain the literal string for the import, in which case the ImportMap entry is relevant.

@bcmills bcmills changed the title cmd/go: missing entry in ImportMap for runtime/cgo [runtime.test] cmd/go: missing entry in ImportMap for runtime/cgo [runtime.test] when -compiled is set Jun 3, 2021
@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 3, 2021
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 3, 2021
@bcmills bcmills self-assigned this Jun 8, 2021
@bcmills
Copy link
Contributor

bcmills commented Jun 8, 2021

Thanks for reporting this; I have uncovered a rat's nest under this issue.

go list -compiled computes a list of matching packages by calling load.PackagesAndErrors:

pkgs := load.PackagesAndErrors(ctx, pkgOpts, args)

In then modifies those packages extensively in-place, including expanding the Imports field to include CompiledImports:

if *listCompiled {
p.Imports = str.StringList(p.Imports, p.Internal.CompiledImports)
}

That is, in fact, the only place where the CompiledImports field is read:

~/go-review/src$ git gn CompiledImports
cmd/go/internal/list/list.go:615:                       p.Imports = str.StringList(p.Imports, p.Internal.CompiledImports)
cmd/go/internal/load/pkg.go:197:        CompiledImports   []string             // additional Imports necessary when using CompiledGoFiles (all from standard library)
cmd/go/internal/load/pkg.go:1798:                       p.Internal.CompiledImports = append(p.Internal.CompiledImports, path)

The package dependencies actually added to the *load.Package are added to the Internal.Imports field, but I am still trying to figure out how cmd/go tells the compiler which import paths in the source code they represent.

@bcmills
Copy link
Contributor

bcmills commented Jun 8, 2021

I see a loop here that generates importmap entries:

for i, raw := range a.Package.Internal.RawImports {
final := a.Package.Imports[i]
if final != raw {
fmt.Fprintf(&icfg, "importmap %s=%s\n", raw, final)
}
}

But that loop only traverses RawImports, which AFAICT don't include the compiled imports.

@gopherbot
Copy link

Change https://golang.org/cl/326610 mentions this issue: cmd/go: report the imports of CompiledGoFiles in ImportMap

@heschi heschi removed the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Jun 10, 2021
@golang golang locked and limited conversation to collaborators Jun 10, 2022
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants