From 73554e0f78058c37e5421bc48273a72400172221 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 8 May 2019 20:48:33 -0400 Subject: [PATCH] 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 Reviewed-by: Michael Matloob --- go/analysis/cmd/vet/vet.go | 3 +- go/analysis/passes/asmdecl/asmdecl.go | 36 +++++++++++++++---- go/analysis/passes/asmdecl/asmdecl_test.go | 22 +++++++++++- .../passes/asmdecl/testdata/src/a/asm1.s | 6 ++++ .../passes/asmdecl/testdata/src/a/asm3.s | 4 +-- .../passes/asmdecl/testdata/src/a/asm8.s | 1 + .../passes/asmdecl/testdata/src/a/asm9.s | 13 +++++++ go/analysis/passes/errorsas/errorsas.go | 7 ++++ go/analysis/passes/stdmethods/stdmethods.go | 7 ++++ .../passes/stdmethods/testdata/src/a/a.go | 5 +++ go/analysis/passes/structtag/structtag.go | 7 ++++ go/analysis/passes/unmarshal/unmarshal.go | 7 ++++ 12 files changed, 106 insertions(+), 12 deletions(-) create mode 100644 go/analysis/passes/asmdecl/testdata/src/a/asm9.s diff --git a/go/analysis/cmd/vet/vet.go b/go/analysis/cmd/vet/vet.go index 4ad2ca64..ec6dcb92 100644 --- a/go/analysis/cmd/vet/vet.go +++ b/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, ) } diff --git a/go/analysis/passes/asmdecl/asmdecl.go b/go/analysis/passes/asmdecl/asmdecl.go index 6403d778..d41c4e97 100644 --- a/go/analysis/passes/asmdecl/asmdecl.go +++ b/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 diff --git a/go/analysis/passes/asmdecl/asmdecl_test.go b/go/analysis/passes/asmdecl/asmdecl_test.go index e978fbba..f88c188b 100644 --- a/go/analysis/passes/asmdecl/asmdecl_test.go +++ b/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() - analysistest.Run(t, testdata, asmdecl.Analyzer, "a") + 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") + }) + } } diff --git a/go/analysis/passes/asmdecl/testdata/src/a/asm1.s b/go/analysis/passes/asmdecl/testdata/src/a/asm1.s index 4fa31a26..9af86122 100644 --- a/go/analysis/passes/asmdecl/testdata/src/a/asm1.s +++ b/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` diff --git a/go/analysis/passes/asmdecl/testdata/src/a/asm3.s b/go/analysis/passes/asmdecl/testdata/src/a/asm3.s index 71980e2b..a269e0e1 100644 --- a/go/analysis/passes/asmdecl/testdata/src/a/asm3.s +++ b/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 12(R13), AX // want `use of 12\(R13\) points beyond argument frame` + MOVW 8(R13), AX // Okay - NOFRAME is assumed special + MOVW 12(R13), AX // Okay - NOFRAME is assumed special RET diff --git a/go/analysis/passes/asmdecl/testdata/src/a/asm8.s b/go/analysis/passes/asmdecl/testdata/src/a/asm8.s index ba49d7d7..1e736db6 100644 --- a/go/analysis/passes/asmdecl/testdata/src/a/asm8.s +++ b/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\)` diff --git a/go/analysis/passes/asmdecl/testdata/src/a/asm9.s b/go/analysis/passes/asmdecl/testdata/src/a/asm9.s new file mode 100644 index 00000000..8c9b6ec1 --- /dev/null +++ b/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` diff --git a/go/analysis/passes/errorsas/errorsas.go b/go/analysis/passes/errorsas/errorsas.go index 3683c75b..8dcbaaa2 100644 --- a/go/analysis/passes/errorsas/errorsas.go +++ b/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{ diff --git a/go/analysis/passes/stdmethods/stdmethods.go b/go/analysis/passes/stdmethods/stdmethods.go index 72530a0e..bc1db7e4 100644 --- a/go/analysis/passes/stdmethods/stdmethods.go +++ b/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 diff --git a/go/analysis/passes/stdmethods/testdata/src/a/a.go b/go/analysis/passes/stdmethods/testdata/src/a/a.go index de240a3b..f660d321 100644 --- a/go/analysis/passes/stdmethods/testdata/src/a/a.go +++ b/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\)` } diff --git a/go/analysis/passes/structtag/structtag.go b/go/analysis/passes/structtag/structtag.go index 2b67c376..bcdb0429 100644 --- a/go/analysis/passes/structtag/structtag.go +++ b/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) } diff --git a/go/analysis/passes/unmarshal/unmarshal.go b/go/analysis/passes/unmarshal/unmarshal.go index 6cf4358a..d019ecef 100644 --- a/go/analysis/passes/unmarshal/unmarshal.go +++ b/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{