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

x/tools/go/packages: assembly among CompiledGoFiles unless NeedFiles flag specified #56632

Closed
adonovan opened this issue Nov 7, 2022 · 2 comments
Closed

x/tools/go/packages: assembly among CompiledGoFiles unless NeedFiles flag specified #56632

adonovan opened this issue Nov 7, 2022 · 2 comments
Assignees
@matloob
Labels
FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@adonovan
Copy link
Member

adonovan commented Nov 7, 2022 •
edited

Failing to specify the NeedFiles mode flag seems to trigger the erroneous inclusion of .s files among CompiledGoFiles. Adding the flag fixes the problem. I suspect this is related to #28749, but even so:
(a) go/packages could implement a workaround if the underlying go list bug can't be quickly fixed;
(b) the NeedFiles flag shouldn't have any effect on the CompiledGoFiles field; and
(c) why is the CompiledGoFiles field populated at all when that mode bit wasn't specified?

$ cat main.go 
package main

import (
	"fmt"
	"log"
	"strings"

	"golang.org/x/tools/go/packages"
)

func main() {
	cfg := &packages.Config{
		Mode: packages.NeedImports |
			// packages.NeedFiles |                           // try with and without this line
			packages.NeedCompiledGoFiles,
	}
	pkgs, err := packages.Load(cfg, "runtime")
	if err != nil {
		log.Fatalf("load: %v", err)
	}
	fmt.Println(strings.Join(pkgs[0].CompiledGoFiles, "\n"))
}

$ go run ./main.go > without
# now uncomment the NeedFiles line
$ go run ./main.go > with
$ diff with without
123a124,133
> /usr/local/go/src/runtime/asm.s
> /usr/local/go/src/runtime/asm_arm64.s
> /usr/local/go/src/runtime/atomic_arm64.s
> /usr/local/go/src/runtime/duff_arm64.s
> /usr/local/go/src/runtime/memclr_arm64.s
> /usr/local/go/src/runtime/memmove_arm64.s
> /usr/local/go/src/runtime/preempt_arm64.s
> /usr/local/go/src/runtime/rt0_darwin_arm64.s
> /usr/local/go/src/runtime/sys_darwin_arm64.s
> /usr/local/go/src/runtime/tls_arm64.s
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Nov 7, 2022
@gopherbot gopherbot added this to the Unreleased milestone Nov 7, 2022
@mdempsky
Copy link
Member

mdempsky commented Nov 9, 2022

This has an additional consequence that NeedSyntax without specifying NeedFiles tries to parse the .s files as Go source, and then produces invalid ASTs that fail type checking.

@mdempsky mdempsky mentioned this issue Nov 9, 2022
Open
@gopherbot
Copy link

Change https://go.dev/cl/452059 mentions this issue: go/packages: change workaround for issue 28749 to not require NeedFiles

@golang golang locked and limited conversation to collaborators Nov 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@matloob matloob

Labels
FrozenDueToAge Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@mdempsky @adonovan @gopherbot @matloob