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: list -json includes assembly files among CompiledGoFiles #28749

Open
alandonovan opened this Issue Nov 12, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@alandonovan
Copy link
Contributor

alandonovan commented Nov 12, 2018

$ go version
go version devel +a4e958ff2d Mon Nov 12 13:44:06 2018 -0500 linux/amd64
$ go list -json -compiled reflect
{
        "Dir": "/usr/local/google/home/adonovan/goroot/src/reflect",
        "ImportPath": "reflect",
        "Name": "reflect",
...
        "GoFiles": [
                "deepequal.go",
                "makefunc.go",
                "swapper.go",
                "type.go",
                "value.go"
        ],
        "CompiledGoFiles": [
                "deepequal.go",
                "makefunc.go",
                "swapper.go",
                "type.go",
                "value.go",
                "asm_amd64.s"   // !
        ],
        "SFiles": [
                "asm_amd64.s"
        ],
...
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Nov 13, 2018

Change https://golang.org/cl/149237 mentions this issue: go/packages: remove .s files from go list's CompiledGoFiles

gopherbot pushed a commit to golang/tools that referenced this issue Nov 13, 2018

go/packages: remove .s files from go list's CompiledGoFiles
This is a workaround for a go list regression that broke
go/packages but went unnoticed by because of a missing
call to packages.PrintErrors, added here.

Updates golang/go#28749

Change-Id: I1819a6143134a422791106ac037d3458ef864322
Reviewed-on: https://go-review.googlesource.com/c/149237
Reviewed-by: Ian Cottrell <iancottrell@google.com>

@bcmills bcmills added the NeedsFix label Nov 19, 2018

@bcmills bcmills added this to the Go1.13 milestone Nov 19, 2018

@bcmills bcmills added the GoCommand label Jan 10, 2019

@bcmills bcmills removed their assignment Jan 10, 2019

@josharian

This comment has been minimized.

Copy link
Contributor

josharian commented Feb 24, 2019

.cc files too. This is pretty clearly a bug in cmd/go:

  • cmd/go/internal/work/exec.go, around line 590:
	var srcfiles []string // .go and non-.go
	srcfiles = append(srcfiles, gofiles...)
	srcfiles = append(srcfiles, sfiles...)
	srcfiles = append(srcfiles, cfiles...)
	srcfiles = append(srcfiles, cxxfiles...)
	b.cacheSrcFiles(a, srcfiles)
  • cacheSrcFiles eventually does: c.PutBytes(cache.Subkey(a.actionID, "srcfiles"), buf.Bytes()).

  • Later, loadCachedSrcFiles does: list, _, err := c.GetBytes(cache.Subkey(a.actionID, "srcfiles")) and then a.Package.CompiledGoFiles = files.

  • This makes its way into the json that cmd/go emits, which go/packages trusts absolutely.

We should make cmd/go filter out non-Go files.


There's a comment in the go/packages source code, where it manually skips .s files:

		// Workaround for https://golang.org/issue/28749.
		// TODO(adonovan): delete before go1.12 release.

If we don't fix this for 1.12, we'll have to keep the workaround in go/packages for a long time. And expand it to cover .cc, .c, .S, etc.

I'll send a CL to expand the go/packages workaround, but I think it might be good to know how risky it is to fix this yet in cmd/go.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Feb 24, 2019

Change https://golang.org/cl/163597 mentions this issue: go/packages: expand CompiledGoFiles workaround

gopherbot pushed a commit to golang/tools that referenced this issue Feb 27, 2019

go/packages: expand CompiledGoFiles workaround
Updates golang/go#28749

Change-Id: I76eb264f61b511fec7e05cef1bdd35e1c7fd603b
Reviewed-on: https://go-review.googlesource.com/c/163597
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.