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

x/tools/go/analysis: asmdecl false positives on AVX2 instructions #47625

Open
Yawning opened this issue Aug 10, 2021 · 5 comments
Open

x/tools/go/analysis: asmdecl false positives on AVX2 instructions #47625

Yawning opened this issue Aug 10, 2021 · 5 comments
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. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@Yawning
Copy link

Yawning commented Aug 10, 2021

go vet raises invalid VPBROADCASTD of mask+24(FP); uint32 is 4-byte value for VPBROADCASTD mask+24(FP), Y0.

Skimming the asmdecl source, this is because argument size is determined by the final character of the opcode (plus some special cases), but when AVX2 instructions were added, the decision was made to use the actual (ie: Intel's) name of each instruction. I suspect there are other opcodes that also trigger false positives in this way.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Aug 10, 2021
@gopherbot gopherbot added this to the Unreleased milestone Aug 10, 2021
@zpavlinovic zpavlinovic added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Aug 12, 2021
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 13, 2021
@zpavlinovic
Copy link
Contributor

Could we get a reproducer code? What would also help is to have the opening comment follow the default issue formatter.

@Yawning
Copy link
Author

Yawning commented Aug 16, 2021

Could we get a reproducer code? What would also help is to have the opening comment follow the default issue formatter.

I narrowed down and root caused the issue because presumably using a giant wall of assembly isn't all that fun, but ok.

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

go version go1.16.6 linux/amd64
go version go1.17rc2 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

This is irrelevant, it's a logic bug in asmdecl.

What did you do?

Ran go vet on an assembly file that contains a memory-sourced VPBROADCASTD (VEX.256.66.0F38.W0 58 /r VPBROADCASTD ymm1, xmm2/m32), where there's size information available for the m32 due to it being an argument (mask+24(FP), mask is a uint32).

https://github.com/oasisprotocol/curve25519-voi/blob/4da56de5ed176dc827391508a16115b250ca91ad/curve/edwards_vector_amd64.s#L201

What did you expect to see?

No asmdecl vet errors regarding VPBROADCASTD. There's other things that asmdecl complains about in the file that are unrelated (the code generator I'm using so I need to copy-paste · less, is incorrect).

What did you see instead?

./edwards_vector_amd64.s:201:1: [amd64] vecConditionalSelect_AVX2: invalid VPBROADCASTD of mask+24(FP); uint32 is 4-byte value

What's wrong

https://github.com/golang/tools/blob/758a1a1db799319be7c0ac33d911e0e5107e48d3/go/analysis/passes/asmdecl/asmdecl.go#L717 is the generic catch-all for opcodes that end with D, following the plan 9 convention. AVX2 opcodes even in Go's assembler use Intel's naming, so the D means "dword".

@mmcloughlin
Copy link
Contributor

Maybe related: #39220.

@aclements
Copy link
Member

FWIW, I would be fine with adding this as a special case to asmdecl, either to recognize the AVX2 instructions, or to recognize the Yn registers.

We have long-term plans (#17544) to replace asmdecl, which is just a bunch of ad hoc regeps, with integration directly into the assembler so it can use the real parser, but that isn't high on the priority list right now.

@Yawning
Copy link
Author

Yawning commented Oct 12, 2021

FWIW, I would be fine with adding this as a special case to asmdecl, either to recognize the AVX2 instructions, or to recognize the Yn registers.

For the specific VPBROADCASTD case that I am hitting, I believe this needs the former? asmdecl is complaining that I am passing it a GP register that contains the address of a uint32 (xmm2/m32). It is fine with the YMM register I am broadcasting into.

  // ie: Something like this should be "fine", but it costs an extra instruction. 
  VMOVD mask+(24)FP, X0
  VPBROADCASTD X0, Y0

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. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants