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

Jump to bottom

x/tools/go/analysis/passes/asmdecl: false error report for 16 byte move #39220

Open
mmcloughlin opened this issue May 23, 2020 · 6 comments
Open

x/tools/go/analysis/passes/asmdecl: false error report for 16 byte move #39220

mmcloughlin opened this issue May 23, 2020 · 6 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
Unreleased

Comments

@mmcloughlin
Copy link
Contributor

The following assembly

#include "textflag.h"

// func Shuffle(mask []byte, data []byte) [4]uint32
// Requires: SSE2, SSSE3
TEXT ·Shuffle(SB), NOSPLIT, $0-64
	MOVQ   mask_base+0(FP), AX
	MOVQ   data_base+24(FP), CX
	MOVOU  (CX), X0
	PSHUFB (AX), X0
	MOVOU  X0, ret+48(FP)
	RET

produces the following error report from asmdecl:

[amd64] Shuffle: invalid MOVOU of ret+48(FP); [4]uint32 is 16-byte value

Since MOVOU is a 128-bit move, this is a false positive.

The error is produced here: https://github.com/golang/tools/blob/73554e0f78058c37e5421bc48273a72400172221/go/analysis/passes/asmdecl/asmdecl.go#L781

Inspecting this code, it's clear that the move size detection does not handle sizes of 16 bytes and up.
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label May 23, 2020
@gopherbot gopherbot added this to the Unreleased milestone May 23, 2020
@mmcloughlin mmcloughlin mentioned this issue May 23, 2020
Open
@andybons
Copy link
Member

@matloob

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 26, 2020
@randall77
Copy link
Contributor

Related CL (support for complex128): https://go-review.googlesource.com/c/tools/+/204537

@mmcloughlin
Copy link
Contributor Author

Ah thank you @randall77. My attempt to avoid this linter error was to change my test case to 64-bits rather than 128-bit (the PR is mmcloughlin/avo#149).

// Code generated by command: go run asm.go -out issue145.s -stubs stub.go. DO NOT EDIT.

#include "textflag.h"

// func Halves(x uint64) [2]uint32
TEXT ·Halves(SB), NOSPLIT, $0-16
	MOVQ x+0(FP), AX
	MOVQ AX, ret+8(FP)
	RET

This gives the following error

[amd64] Halves: invalid MOVQ of ret+8(FP); [2]uint32 is 8-byte value

As I result I suspect that there are two aspects to this:

  • Detection of moves of 16 bytes and larger
  • Support for non-primitive types like [2]uint32 (complexN also falls in this category)

@zchee
Copy link
Contributor

zchee commented Nov 3, 2020

@randall77 @mmcloughlin sorry mentioned, I would like to know the current status.

@randall77
Copy link
Contributor

I know of no one actively working on this. Patches welcome (1.16 freeze is on us, so it would be for 1.17).

@zchee
Copy link
Contributor

zchee commented Nov 3, 2020

@randall77 Thanks, will do.

This was referenced Aug 21, 2021
Closed
Open
@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
Assignees
No one assigned
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
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@andybons @adonovan @zchee @randall77 @mmcloughlin @gopherbot