go/analysis/passes: fix bugs discovered in std

asmdecl:
- MOVW $x+0(FP) is OK if x is big, because $x is an address
  (happens in internal/cpu, golang.org/x/sys/cpu, runtime)
- ignore TEXT lines in comments
  (happens in runtime/internal/atomic)
- wasm's CallImport instruction writes return results
  (happens in syscall)
- allow write out-of-bounds (SP) references in NOFRAME functions
  (happens in runtime)
- recognize "NOP SP" as an SP "write" to disable SP bounds checking
- 'go test' in passes/asmdecl was not testing all architectures; fix that

stdmethods:
- ignore WriteTo if obviously not io.WriterTo (as in go/types and runtime/pprof)

errorsas:
- don't complain about package errors testing invalid calls

structtag:
- don't complain about encoding/json and encoding/xml testing invalid tags

unmarshal:
- don't complain about encoding/gob, encoding/json, encoding/xml testing invalid calls

For golang/go#31916.
Fixes golang/go#25822.

Change-Id: I322c08b5991ffc4995112b8ea945161a4c5193ce
Reviewed-on: https://go-review.googlesource.com/c/tools/+/176097
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>

go/analysis/cmd/vet/vet.go

@@ -31,7 +31,6 @@ import (
 	"golang.org/x/tools/go/analysis/passes/loopclosure"
 	"golang.org/x/tools/go/analysis/passes/lostcancel"
 	"golang.org/x/tools/go/analysis/passes/nilfunc"
-	"golang.org/x/tools/go/analysis/passes/nilness"
 	"golang.org/x/tools/go/analysis/passes/printf"
 	"golang.org/x/tools/go/analysis/passes/shift"
 	"golang.org/x/tools/go/analysis/passes/stdmethods"
@@ -79,6 +78,6 @@ func main() {
 		// pkgfact.Analyzer,
 
 		// uses SSA:
-		nilness.Analyzer,
+		// nilness.Analyzer,
 	)
 }

go/analysis/passes/asmdecl/asmdecl.go

@@ -130,7 +130,7 @@ var (
 	asmPlusBuild = re(`//\s+\+build\s+([^\n]+)`)
 	asmTEXT      = re(`\bTEXT\b(.*)·([^\(]+)\(SB\)(?:\s*,\s*([0-9A-Z|+()]+))?(?:\s*,\s*\$(-?[0-9]+)(?:-([0-9]+))?)?`)
 	asmDATA      = re(`\b(DATA|GLOBL)\b`)
-	asmNamedFP   = re(`([a-zA-Z0-9_\xFF-\x{10FFFF}]+)(?:\+([0-9]+))\(FP\)`)
+	asmNamedFP   = re(`\$?([a-zA-Z0-9_\xFF-\x{10FFFF}]+)(?:\+([0-9]+))\(FP\)`)
 	asmUnnamedFP = re(`[^+\-0-9](([0-9]+)\(FP\))`)
 	asmSP        = re(`[^+\-0-9](([0-9]+)\(([A-Z0-9]+)\))`)
 	asmOpcode    = re(`^\s*(?:[A-Z0-9a-z_]+:)?\s*([A-Z]+)\s*([^,]*)(?:,\s*(.*))?`)
@@ -184,6 +184,7 @@ Files:
 			fnName             string
 			localSize, argSize int
 			wroteSP            bool
+			noframe            bool
 			haveRetArg         bool
 			retLine            []int
 		)
@@ -231,6 +232,11 @@ Files:
 				}
 			}
 
+			// Ignore comments and commented-out code.
+			if i := strings.Index(line, "//"); i >= 0 {
+				line = line[:i]
+			}
+
 			if m := asmTEXT.FindStringSubmatch(line); m != nil {
 				flushRet()
 				if arch == "" {
@@ -254,7 +260,7 @@ Files:
 					// identifiers to represent the directory separator.
 					pkgPath = strings.Replace(pkgPath, "∕", "/", -1)
 					if pkgPath != pass.Pkg.Path() {
-						log.Printf("%s:%d: [%s] cannot check cross-package assembly function: %s is in package %s", fname, lineno, arch, fnName, pkgPath)
+						// log.Printf("%s:%d: [%s] cannot check cross-package assembly function: %s is in package %s", fname, lineno, arch, fnName, pkgPath)
 						fn = nil
 						fnName = ""
 						continue
@@ -275,7 +281,8 @@ Files:
 					localSize += archDef.intSize
 				}
 				argSize, _ = strconv.Atoi(m[5])
-				if fn == nil && !strings.Contains(fnName, "<>") {
+				noframe = strings.Contains(flag, "NOFRAME")
+				if fn == nil && !strings.Contains(fnName, "<>") && !noframe {
 					badf("function %s missing Go declaration", fnName)
 				}
 				wroteSP = false
@@ -305,13 +312,18 @@ Files:
 				continue
 			}
 
-			if strings.Contains(line, ", "+archDef.stack) || strings.Contains(line, ",\t"+archDef.stack) {
+			if strings.Contains(line, ", "+archDef.stack) || strings.Contains(line, ",\t"+archDef.stack) || strings.Contains(line, "NOP "+archDef.stack) || strings.Contains(line, "NOP\t"+archDef.stack) {
 				wroteSP = true
 				continue
 			}
 
+			if arch == "wasm" && strings.Contains(line, "CallImport") {
+				// CallImport is a call out to magic that can write the result.
+				haveRetArg = true
+			}
+
 			for _, m := range asmSP.FindAllStringSubmatch(line, -1) {
-				if m[3] != archDef.stack || wroteSP {
+				if m[3] != archDef.stack || wroteSP || noframe {
 					continue
 				}
 				off := 0
@@ -371,7 +383,7 @@ Files:
 					}
 					continue
 				}
-				asmCheckVar(badf, fn, line, m[0], off, v)
+				asmCheckVar(badf, fn, line, m[0], off, v, archDef)
 			}
 		}
 		flushRet()
@@ -589,7 +601,7 @@ func asmParseDecl(pass *analysis.Pass, decl *ast.FuncDecl) map[string]*asmFunc {
 }
 
 // asmCheckVar checks a single variable reference.
-func asmCheckVar(badf func(string, ...interface{}), fn *asmFunc, line, expr string, off int, v *asmVar) {
+func asmCheckVar(badf func(string, ...interface{}), fn *asmFunc, line, expr string, off int, v *asmVar, archDef *asmArch) {
 	m := asmOpcode.FindStringSubmatch(line)
 	if m == nil {
 		if !strings.HasPrefix(strings.TrimSpace(line), "//") {
@@ -598,6 +610,8 @@ func asmCheckVar(badf func(string, ...interface{}), fn *asmFunc, line, expr stri
 		return
 	}
 
+	addr := strings.HasPrefix(expr, "$")
+
 	// Determine operand sizes from instruction.
 	// Typically the suffix suffices, but there are exceptions.
 	var src, dst, kind asmKind
@@ -617,10 +631,13 @@ func asmCheckVar(badf func(string, ...interface{}), fn *asmFunc, line, expr stri
 	// They just take the address of it.
 	case "386.LEAL":
 		dst = 4
+		addr = true
 	case "amd64.LEAQ":
 		dst = 8
+		addr = true
 	case "amd64p32.LEAL":
 		dst = 4
+		addr = true
 	default:
 		switch fn.arch.name {
 		case "386", "amd64":
@@ -725,6 +742,11 @@ func asmCheckVar(badf func(string, ...interface{}), fn *asmFunc, line, expr stri
 		vs = v.inner[0].size
 		vt = v.inner[0].typ
 	}
+	if addr {
+		vk = asmKind(archDef.ptrSize)
+		vs = archDef.ptrSize
+		vt = "address"
+	}
 
 	if off != v.off {
 		var inner bytes.Buffer

go/analysis/passes/asmdecl/asmdecl_test.go

@@ -5,13 +5,33 @@
 package asmdecl_test
 
 import (
+	"os"
+	"strings"
 	"testing"
 
 	"golang.org/x/tools/go/analysis/analysistest"
 	"golang.org/x/tools/go/analysis/passes/asmdecl"
 )
 
+var goosarches = []string{
+	"linux/amd64",  // asm1.s, asm4.s
+	"linux/386",    // asm2.s
+	"linux/arm",    // asm3.s
+	"linux/mips64", // asm5.s
+	"linux/s390x",  // asm6.s
+	"linux/ppc64",  // asm7.s
+	"linux/mips",   // asm8.s,
+	"js/wasm",      // asm9.s
+}
+
 func Test(t *testing.T) {
 	testdata := analysistest.TestData()
+	for _, goosarch := range goosarches {
+		t.Run(goosarch, func(t *testing.T) {
+			i := strings.Index(goosarch, "/")
+			os.Setenv("GOOS", goosarch[:i])
+			os.Setenv("GOARCH", goosarch[i+1:])
 			analysistest.Run(t, testdata, asmdecl.Analyzer, "a")
+		})
+	}
 }

go/analysis/passes/asmdecl/testdata/src/a/asm1.s

@@ -4,6 +4,11 @@
 
 // +build amd64
 
+// Commented-out code should be ignored.
+//
+//	TEXT ·unknown(SB),0,$0
+//		RET
+
 TEXT ·arg1(SB),0,$0-2
 	MOVB	x+0(FP), AX
 	// MOVB x+0(FP), AX // commented out instructions used to panic
@@ -152,6 +157,7 @@ TEXT ·argptr(SB),7,$0-2 // want `wrong argument size 2; expected \$\.\.\.-40`
 TEXT ·argstring(SB),0,$32 // want `wrong argument size 0; expected \$\.\.\.-32`
 	MOVW	x+0(FP), AX // want `invalid MOVW of x\+0\(FP\); string base is 8-byte value`
 	MOVL	x+0(FP), AX // want `invalid MOVL of x\+0\(FP\); string base is 8-byte value`
+	LEAQ	x+0(FP), AX // ok
 	MOVQ	x+0(FP), AX
 	MOVW	x_base+0(FP), AX // want `invalid MOVW of x_base\+0\(FP\); string base is 8-byte value`
 	MOVL	x_base+0(FP), AX // want `invalid MOVL of x_base\+0\(FP\); string base is 8-byte value`

go/analysis/passes/asmdecl/testdata/src/a/asm3.s

@@ -186,6 +186,6 @@ TEXT ·noframe1(SB),0,$0-4
 TEXT ·noframe2(SB),NOFRAME,$0-4
 	MOVW	0(R13), AX // Okay; caller's saved LR
 	MOVW	x+4(R13), AX // Okay; x argument
-	MOVW	8(R13), AX // want `use of 8\(R13\) points beyond argument frame`
+	MOVW	8(R13), AX // Okay - NOFRAME is assumed special
-	MOVW	12(R13), AX // want `use of 12\(R13\) points beyond argument frame`
+	MOVW	12(R13), AX // Okay - NOFRAME is assumed special
 	RET

go/analysis/passes/asmdecl/testdata/src/a/asm8.s

@@ -47,6 +47,7 @@ TEXT ·arg8(SB),7,$0-2 // want `wrong argument size 2; expected \$\.\.\.-16`
 	MOVH	x+0(FP), R1 // want `invalid MOVH of x\+0\(FP\); int64 is 8-byte value`
 	MOVH	y+8(FP), R1 // want `invalid MOVH of y\+8\(FP\); uint64 is 8-byte value`
 	MOVW	x+0(FP), R1 // want `invalid MOVW of x\+0\(FP\); int64 is 8-byte value containing x_lo\+0\(FP\) and x_hi\+4\(FP\)`
+	MOVW	$x+0(FP), R1 // ok
 	MOVW	x_lo+0(FP), R1
 	MOVW	x_hi+4(FP), R1
 	MOVW	y+8(FP), R1 // want `invalid MOVW of y\+8\(FP\); uint64 is 8-byte value containing y_lo\+8\(FP\) and y_hi\+12\(FP\)`

go/analysis/passes/asmdecl/testdata/src/a/asm9.s

@@ -0,0 +1,13 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build wasm
+
+TEXT ·returnint(SB),NOSPLIT|NOFRAME,$0-8
+	MOVD	24(SP), R1 // ok to access beyond stack frame with NOFRAME
+	CallImport // interpreted as writing result
+	RET
+
+TEXT ·returnbyte(SB),$0-9
+	RET // want `RET without writing`

go/analysis/passes/errorsas/errorsas.go

@@ -29,6 +29,13 @@ var Analyzer = &analysis.Analyzer{
 }
 
 func run(pass *analysis.Pass) (interface{}, error) {
+	switch pass.Pkg.Path() {
+	case "errors", "errors_test":
+		// These packages know how to use their own APIs.
+		// Sometimes they are testing what happens to incorrect programs.
+		return nil, nil
+	}
+
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 
 	nodeFilter := []ast.Node{

go/analysis/passes/stdmethods/stdmethods.go

@@ -116,6 +116,13 @@ func canonicalMethod(pass *analysis.Pass, id *ast.Ident) {
 	args := sign.Params()
 	results := sign.Results()
 
+	// Special case: WriteTo with more than one argument,
+	// not trying at all to implement io.WriterTo,
+	// comes up often enough to skip.
+	if id.Name == "WriteTo" && args.Len() > 1 {
+		return
+	}
+
 	// Do the =s (if any) all match?
 	if !matchParams(pass, expect.args, args, "=") || !matchParams(pass, expect.results, results, "=") {
 		return

go/analysis/passes/stdmethods/testdata/src/a/a.go

@@ -7,6 +7,7 @@ package a
 import (
 	"encoding/xml"
 	"fmt"
+	"io"
 )
 
 type T int
@@ -28,6 +29,10 @@ func (U) UnmarshalXML(*xml.Decoder, xml.StartElement) error { // no error: signa
 	return nil
 }
 
+func (U) WriteTo(w io.Writer) {} // want `method WriteTo\(w io.Writer\) should have signature WriteTo\(io.Writer\) \(int64, error\)`
+
+func (T) WriteTo(w io.Writer, more, args int) {} // ok - clearly not io.WriterTo
+
 type I interface {
 	ReadByte() byte // want `should have signature ReadByte\(\) \(byte, error\)`
 }

go/analysis/passes/structtag/structtag.go

@@ -56,6 +56,13 @@ var checkTagSpaces = map[string]bool{"json": true, "xml": true, "asn1": true}
 
 // checkCanonicalFieldTag checks a single struct field tag.
 func checkCanonicalFieldTag(pass *analysis.Pass, field *types.Var, tag string, seen *map[[2]string]token.Pos) {
+	switch pass.Pkg.Path() {
+	case "encoding/json", "encoding/xml":
+		// These packages know how to use their own APIs.
+		// Sometimes they are testing what happens to incorrect programs.
+		return
+	}
+
 	for _, key := range checkTagDups {
 		checkTagDuplicates(pass, tag, key, field, field, seen)
 	}

go/analysis/passes/unmarshal/unmarshal.go

@@ -29,6 +29,13 @@ var Analyzer = &analysis.Analyzer{
 }
 
 func run(pass *analysis.Pass) (interface{}, error) {
+	switch pass.Pkg.Path() {
+	case "encoding/gob", "encoding/json", "encoding/xml":
+		// These packages know how to use their own APIs.
+		// Sometimes they are testing what happens to incorrect programs.
+		return nil, nil
+	}
+
 	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
 
 	nodeFilter := []ast.Node{