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/vet: asmdecl parameter checking doesn't play well with arm64 LDP #57451

Open
cespare opened this issue Dec 23, 2022 · 1 comment
Open

cmd/vet: asmdecl parameter checking doesn't play well with arm64 LDP #57451

cespare opened this issue Dec 23, 2022 · 1 comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Unfortunate
Milestone

Comments

@cespare
Copy link
Contributor

cespare commented Dec 23, 2022

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

Go 1.19

Does this issue reproduce with the latest release?

Yes, and also with tip.

And also with the latest x/tools repo (the home of the vet code).

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

I'm on linux/amd64, but I'm looking at arm64-specific checks (I'm running vet
with GOARCH=arm64).

What did you do?

Run GOARCH=arm64 go vet . on a package with some code like this:

// func f(b []byte)
TEXT ·f(SB), NOSPLIT|NOFRAME, $0-24
	LDP b_base_len+0(FP), (R2, R3)
	...

Note the LDP instruction is a handy way on arm64 to load two registers at once. It's a pretty natural way to load the base pointer and length of a slice.

What did you expect to see?

No complaints.

What did you see instead?

./x_arm64.s:25:1: [arm64] f: unknown variable b_base_len; offset 0 is b_base+0(FP)

That is, vet expects that any reference to offset 0 is named b_base, but in this case the reference is actually 16 bytes long; it's both base and len.

I skimmed the asmdecl code but it's unfortunately not trivial to fix (there seems to be a pretty deeply-baked assumption that a reference to an FP offset corresponds to a single "var").

/cc @matloob and @timothy-king (owners of cmd/vet and x/tools/go/analysis)

@dr2chase dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 23, 2022
@cherrymui
Copy link
Member

cherrymui commented Dec 27, 2022

Yeah, this is unfortunate. As a workaround, I think LDP b_base+0(FP), (R2, R3) works? I assume same is true if you want to load two separate consecutive arguments, or store two separate consecutive results. The name b_base is not great as it is not accurate. But I'm not sure b_base_len is much better, in particular, I'm not sure what is the best for the two separate arguments case. If you think b_base+0(FP) as the start address of the load, that is at least reasonable.

@seankhliao seankhliao added this to the Unplanned milestone Jan 20, 2023
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Unfortunate
Projects
None yet
Development

No branches or pull requests

5 participants