From c7512a5f833b8065e559e4f58f6f8f292da6a179 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Thu, 30 Oct 2014 11:53:56 -0400 Subject: [PATCH] cmd/vet: check return moves, support power64x, various fixes This adds support for checking moves to the return value stack slot (from rsc), adds support for checking power64x assembly, fixes argument offset checking and leaf function support on platforms with a link register (arm and power64). LGTM=rsc R=rsc CC=golang-codereviews https://golang.org/cl/166920043 --- cmd/vet/asmdecl.go | 119 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 111 insertions(+), 8 deletions(-) diff --git a/cmd/vet/asmdecl.go b/cmd/vet/asmdecl.go index cd4befc1..954ffbd9 100644 --- a/cmd/vet/asmdecl.go +++ b/cmd/vet/asmdecl.go @@ -35,6 +35,8 @@ type asmArch struct { intSize int maxAlign int bigEndian bool + stack string + lr bool } // An asmFunc describes the expected variables for a function on a given architecture. @@ -56,27 +58,33 @@ type asmVar struct { } var ( - asmArch386 = asmArch{"386", 4, 4, 4, false} - asmArchArm = asmArch{"arm", 4, 4, 4, false} - asmArchAmd64 = asmArch{"amd64", 8, 8, 8, false} - asmArchAmd64p32 = asmArch{"amd64p32", 4, 4, 8, false} + asmArch386 = asmArch{"386", 4, 4, 4, false, "SP", false} + asmArchArm = asmArch{"arm", 4, 4, 4, false, "R13", true} + asmArchAmd64 = asmArch{"amd64", 8, 8, 8, false, "SP", false} + asmArchAmd64p32 = asmArch{"amd64p32", 4, 4, 8, false, "SP", false} + asmArchPower64 = asmArch{"power64", 8, 8, 8, true, "R1", true} + asmArchPower64LE = asmArch{"power64le", 8, 8, 8, false, "R1", true} arches = []*asmArch{ &asmArch386, &asmArchArm, &asmArchAmd64, &asmArchAmd64p32, + &asmArchPower64, + &asmArchPower64LE, } ) var ( re = regexp.MustCompile asmPlusBuild = re(`//\s+\+build\s+([^\n]+)`) - asmTEXT = re(`\bTEXT\b.*·([^\(]+)\(SB\)(?:\s*,\s*([0-9A-Z|+]+))?(?:\s*,\s*\$([0-9]+)(?:-([0-9]+))?)?`) + 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\)`) 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*(.*))?`) + power64Suff = re(`([BHWD])(ZU|Z|U|BR)?$`) ) func asmCheck(pkg *Package) { @@ -102,7 +110,6 @@ func asmCheck(pkg *Package) { } } - var fn *asmFunc for _, f := range pkg.files { if !strings.HasSuffix(f.name, ".s") { continue @@ -111,19 +118,39 @@ func asmCheck(pkg *Package) { // Determine architecture from file name if possible. var arch string + var archDef *asmArch for _, a := range arches { if strings.HasSuffix(f.name, "_"+a.name+".s") { arch = a.name + archDef = a break } } lines := strings.SplitAfter(string(f.content), "\n") + var ( + fn *asmFunc + fnName string + localSize, argSize int + wroteSP bool + haveRetArg bool + retLine []int + ) + + flushRet := func() { + if fn != nil && fn.vars["ret"] != nil && !haveRetArg && len(retLine) > 0 { + v := fn.vars["ret"] + for _, line := range retLine { + f.Badf(token.NoPos, "%s:%d: [%s] %s: RET without writing to %d-byte ret+%d(FP)", f.name, line, arch, fnName, v.size, v.off) + } + } + retLine = nil + } for lineno, line := range lines { lineno++ badf := func(format string, args ...interface{}) { - f.Badf(token.NoPos, "%s:%d: [%s] %s", f.name, lineno, arch, fmt.Sprintf(format, args...)) + f.Badf(token.NoPos, "%s:%d: [%s] %s: %s", f.name, lineno, arch, fnName, fmt.Sprintf(format, args...)) } if arch == "" { @@ -134,6 +161,7 @@ func asmCheck(pkg *Package) { for _, a := range arches { if a.name == fld { arch = a.name + archDef = a break Fields } } @@ -142,10 +170,12 @@ func asmCheck(pkg *Package) { } if m := asmTEXT.FindStringSubmatch(line); m != nil { + flushRet() if arch == "" { f.Warnf(token.NoPos, "%s: cannot determine architecture for assembly file", f.name) return } + fnName = m[1] fn = knownFunc[m[1]][arch] if fn != nil { size, _ := strconv.Atoi(m[4]) @@ -153,16 +183,69 @@ func asmCheck(pkg *Package) { badf("wrong argument size %d; expected $...-%d", size, fn.size) } } + localSize, _ = strconv.Atoi(m[3]) + localSize += archDef.intSize + if archDef.lr { + // Account for caller's saved LR + localSize += archDef.intSize + } + argSize, _ = strconv.Atoi(m[4]) + wroteSP = false + haveRetArg = false continue } else if strings.Contains(line, "TEXT") && strings.Contains(line, "SB") { // function, but not visible from Go (didn't match asmTEXT), so stop checking + flushRet() fn = nil + fnName = "" + continue + } + + if strings.Contains(line, "RET") { + retLine = append(retLine, lineno) + } + + if fnName == "" { continue } if asmDATA.FindStringSubmatch(line) != nil { fn = nil } + + if archDef == nil { + continue + } + + if strings.Contains(line, ", "+archDef.stack) || strings.Contains(line, ",\t"+archDef.stack) { + wroteSP = true + continue + } + + for _, m := range asmSP.FindAllStringSubmatch(line, -1) { + if m[3] != archDef.stack || wroteSP { + continue + } + off := 0 + if m[1] != "" { + off, _ = strconv.Atoi(m[2]) + } + if off >= localSize { + if fn != nil { + v := fn.varByOffset[off-localSize] + if v != nil { + badf("%s should be %s+%d(FP)", m[1], v.name, off-localSize) + continue + } + } + if off >= localSize+argSize { + badf("use of %s points beyond argument frame", m[1]) + continue + } + badf("use of %s to access argument frame", m[1]) + } + } + if fn == nil { continue } @@ -177,6 +260,9 @@ func asmCheck(pkg *Package) { if m[2] != "" { off, _ = strconv.Atoi(m[2]) } + if name == "ret" || strings.HasPrefix(name, "ret_") { + haveRetArg = true + } v := fn.vars[name] if v == nil { // Allow argframe+0(FP). @@ -194,6 +280,7 @@ func asmCheck(pkg *Package) { asmCheckVar(badf, fn, line, m[0], off, v) } } + flushRet() } } @@ -453,7 +540,8 @@ func asmCheckVar(badf func(string, ...interface{}), fn *asmFunc, line, expr stri case "amd64p32.LEAL": dst = 4 default: - if fn.arch.name == "386" || fn.arch.name == "amd64" { + switch fn.arch.name { + case "386", "amd64": if strings.HasPrefix(op, "F") && (strings.HasSuffix(op, "D") || strings.HasSuffix(op, "DP")) { // FMOVDP, FXCHD, etc src = 8 @@ -489,6 +577,21 @@ func asmCheckVar(badf func(string, ...interface{}), fn *asmFunc, line, expr stri case 'D', 'Q': src = 8 } + case "power64", "power64le": + // Strip standard suffixes to reveal size letter. + m := power64Suff.FindStringSubmatch(op) + if m != nil { + switch m[1][0] { + case 'B': + src = 1 + case 'H': + src = 2 + case 'W': + src = 4 + case 'D': + src = 8 + } + } } } if dst == 0 {