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>
diff --git a/go/analysis/cmd/vet/vet.go b/go/analysis/cmd/vet/vet.go
index 4ad2ca6..ec6dcb9 100644
--- a/go/analysis/cmd/vet/vet.go
+++ b/go/analysis/cmd/vet/vet.go
@@ -31,7 +31,6 @@
"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 @@
// 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 6403d77..d41c4e9 100644
--- a/go/analysis/passes/asmdecl/asmdecl.go
+++ b/go/analysis/passes/asmdecl/asmdecl.go
@@ -130,7 +130,7 @@
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 @@
fnName string
localSize, argSize int
wroteSP bool
+ noframe bool
haveRetArg bool
retLine []int
)
@@ -231,6 +232,11 @@
}
}
+ // 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 @@
// 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 @@
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 @@
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 @@
}
continue
}
- asmCheckVar(badf, fn, line, m[0], off, v)
+ asmCheckVar(badf, fn, line, m[0], off, v, archDef)
}
}
flushRet()
@@ -589,7 +601,7 @@
}
// 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 @@
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 @@
// 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 @@
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 e978fbb..f88c188 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 4fa31a2..9af8612 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 ·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 71980e2..a269e0e 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 ·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 ba49d7d..1e736db 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 @@
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 0000000..8c9b6ec
--- /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 3683c75..8dcbaaa 100644
--- a/go/analysis/passes/errorsas/errorsas.go
+++ b/go/analysis/passes/errorsas/errorsas.go
@@ -29,6 +29,13 @@
}
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 72530a0..bc1db7e 100644
--- a/go/analysis/passes/stdmethods/stdmethods.go
+++ b/go/analysis/passes/stdmethods/stdmethods.go
@@ -116,6 +116,13 @@
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 de240a3..f660d32 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 @@
import (
"encoding/xml"
"fmt"
+ "io"
)
type T int
@@ -28,6 +29,10 @@
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 2b67c37..bcdb042 100644
--- a/go/analysis/passes/structtag/structtag.go
+++ b/go/analysis/passes/structtag/structtag.go
@@ -56,6 +56,13 @@
// 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 6cf4358..d019ece 100644
--- a/go/analysis/passes/unmarshal/unmarshal.go
+++ b/go/analysis/passes/unmarshal/unmarshal.go
@@ -29,6 +29,13 @@
}
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{
