golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.99k stars 17.67k forks source link

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

Open piotr-sneller opened 2 years ago

piotr-sneller commented 2 years ago

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?

_.\xxxamd64.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 commented 2 years ago

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).

piotr-sneller commented 2 years ago

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 commented 2 years ago

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