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: invalid move due to type clash, warning unclear #55936

Open
piotr-sneller opened this issue Sep 29, 2022 · 3 comments
Open

cmd/vet: invalid move due to type clash, warning unclear #55936

piotr-sneller opened this issue Sep 29, 2022 · 3 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.
Milestone

Comments

@piotr-sneller
Copy link

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

go1.19.1 windows/amd64

Does this issue reproduce with the latest release?

Yes

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

Windows 10 64-bit

What did you do?

Create a Go file xxx.go with the following content:

package vm

type A [2]uint32

//go:noescape
//go:nosplit
func fun(a A)

And the assembly file xxx_amd64.s:

#include "textflag.h"
#include "funcdata.h"
#include "go_asm.h"

// func fun(a A)
TEXT ·fun(SB), NOSPLIT | NOFRAME, $0-0 
    MOVQ    a+0(FP), AX
    RET

Then run go vet

What did you expect to see?

Nothing; in particular no false positive warning.

What did you see instead?

.\xxx_amd64.s:7:1: [amd64] fun: invalid MOVQ of a+0(FP); vm.A is 8-byte value

The size of the array is exactly 8 bytes and yet go vet warns that 8 bytes != 8 bytes.

@thanm
Copy link
Contributor

thanm commented Sep 29, 2022

The code that triggers the warning begins here:

	if kind != 0 && kind != vk {

Although the sizes are the same, the value types are different (scalar vs array). In general it is a good thing to issue a warning when there is a type clash (for example, if a param is moved into a floating point register with an FP move, but the param in question is not floating point).

@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 29, 2022
@thanm thanm changed the title affected/package: go vet cmd/vet: invalid move due to type clash, warning unclear Sep 29, 2022
@piotr-sneller
Copy link
Author

Thank you Than. I know what I am doing here, so I'd like to suppress the warning somehow. I believe there are two possibilities:

  1. There's an assembler syntax I am unaware of that would do what I want without triggering a vet warning. In this case please share it and resolve the issue as invalid, or:

  2. Switch the register from FP to SP, as I currently do:
    MOVQ v+16(SP), AX // Mute the "invalid MOVQ of v+8(FP); vm.vmref is 8-byte value" go vet false positive.

Unfortunately, there's no elegant mapping between SP and FP, so the offsets are different, in my case by 8: 16(SP) vs. 8(FP). Pushing a programmer to write like that does more harm than vet is trying to avoid, so I'd appreciate some way of reassuring vet that the size is all that matters.

@thanm
Copy link
Contributor

thanm commented Sep 29, 2022

What I have seen in the past for similar problems is that folks will emit the assembly with BYTE, then include a comment explaining why and what it is. E.g.

// The following instruction below emitted as bytes to
// disable vet check, see issue #55936.
/* MOVQ    a+0(FP), AX */ BYTE $48; BYTE $8b; BYTE $44; BYTE $24; BYTE $08

@seankhliao seankhliao added this to the Unplanned milestone Nov 19, 2022
@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.
Projects
None yet
Development

No branches or pull requests

4 participants