From 4af1148079f00b461c9ae79df22aa647aa7ff5ef Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Sat, 9 Jul 2016 17:10:57 -0700 Subject: [PATCH] cmd/vet: improve asmdecl parameter handling The asmdecl check had hand-rolled code that calculated the size and offset of parameters based only on the AST. It included a list of known named types. This CL changes asmdecl to use go/types instead. This allows us to easily handle named types. It also adds support for structs, arrays, and complex parameters. It improves the default names given to unnamed parameters. Previously, all anonymous arguments were called "unnamed", and the first anonymous return argument was called "ret". Anonymous arguments are now called arg, arg1, arg2, etc., depending on the index in the argument list. Return arguments are ret, ret1, ret2. This CL also fixes a bug in the printing of composite data type sizes. Updates #11041 Change-Id: I1085116a26fe6199480b680eff659eb9ab31769b Reviewed-on: https://go-review.googlesource.com/27150 Run-TryBot: Josh Bleecher Snyder TryBot-Result: Gobot Gobot Reviewed-by: Rob Pike --- src/cmd/vet/asmdecl.go | 386 +++++++++++++++++------------------ src/cmd/vet/testdata/asm.go | 9 + src/cmd/vet/testdata/asm1.s | 43 ++++ src/runtime/sys_darwin_arm.s | 2 +- src/runtime/sys_plan9_arm.s | 2 +- 5 files changed, 243 insertions(+), 199 deletions(-) diff --git a/src/cmd/vet/asmdecl.go b/src/cmd/vet/asmdecl.go index bd336cb662..17172ed71d 100644 --- a/src/cmd/vet/asmdecl.go +++ b/src/cmd/vet/asmdecl.go @@ -12,6 +12,7 @@ import ( "go/ast" "go/build" "go/token" + "go/types" "regexp" "strconv" "strings" @@ -25,16 +26,17 @@ type asmKind int const ( asmString asmKind = 100 + iota asmSlice + asmArray asmInterface asmEmptyInterface + asmStruct + asmComplex ) // An asmArch describes assembly parameters for an architecture type asmArch struct { name string - ptrSize int - intSize int - maxAlign int + sizes *types.StdSizes bigEndian bool stack string lr bool @@ -58,16 +60,23 @@ type asmVar struct { inner []*asmVar } +// Common architecture word sizes and alignments. var ( - asmArch386 = asmArch{"386", 4, 4, 4, false, "SP", false} - asmArchArm = asmArch{"arm", 4, 4, 4, false, "R13", true} - asmArchArm64 = asmArch{"arm64", 8, 8, 8, false, "RSP", true} - asmArchAmd64 = asmArch{"amd64", 8, 8, 8, false, "SP", false} - asmArchAmd64p32 = asmArch{"amd64p32", 4, 4, 8, false, "SP", false} - asmArchMips64 = asmArch{"mips64", 8, 8, 8, true, "R29", true} - asmArchMips64LE = asmArch{"mips64", 8, 8, 8, false, "R29", true} - asmArchPpc64 = asmArch{"ppc64", 8, 8, 8, true, "R1", true} - asmArchPpc64LE = asmArch{"ppc64le", 8, 8, 8, false, "R1", true} + size44 = &types.StdSizes{WordSize: 4, MaxAlign: 4} + size48 = &types.StdSizes{WordSize: 4, MaxAlign: 8} + size88 = &types.StdSizes{WordSize: 8, MaxAlign: 8} +) + +var ( + asmArch386 = asmArch{"386", size44, false, "SP", false} + asmArchArm = asmArch{"arm", size44, false, "R13", true} + asmArchArm64 = asmArch{"arm64", size88, false, "RSP", true} + asmArchAmd64 = asmArch{"amd64", size88, false, "SP", false} + asmArchAmd64p32 = asmArch{"amd64p32", size48, false, "SP", false} + asmArchMips64 = asmArch{"mips64", size88, true, "R29", true} + asmArchMips64LE = asmArch{"mips64", size88, false, "R29", true} + asmArchPpc64 = asmArch{"ppc64", size88, true, "R1", true} + asmArchPpc64LE = asmArch{"ppc64le", size88, false, "R1", true} arches = []*asmArch{ &asmArch386, @@ -82,6 +91,10 @@ var ( } ) +func (a *asmArch) intSize() int { return int(a.sizes.WordSize) } +func (a *asmArch) ptrSize() int { return int(a.sizes.WordSize) } +func (a *asmArch) maxAlign() int { return int(a.sizes.MaxAlign) } + var ( re = regexp.MustCompile asmPlusBuild = re(`//\s+\+build\s+([^\n]+)`) @@ -201,10 +214,10 @@ Files: } } localSize, _ = strconv.Atoi(m[3]) - localSize += archDef.intSize + localSize += archDef.intSize() if archDef.lr { // Account for caller's saved LR - localSize += archDef.intSize + localSize += archDef.intSize() } argSize, _ = strconv.Atoi(m[4]) if fn == nil && !strings.Contains(fnName, "<>") { @@ -310,199 +323,179 @@ Files: } } +func asmKindForType(t types.Type, size int) asmKind { + switch t := t.Underlying().(type) { + case *types.Basic: + switch t.Kind() { + case types.String: + return asmString + case types.Complex64, types.Complex128: + return asmComplex + } + return asmKind(size) + case *types.Pointer, *types.Chan, *types.Map, *types.Signature: + return asmKind(size) + case *types.Struct: + return asmStruct + case *types.Interface: + if t.Empty() { + return asmEmptyInterface + } + return asmInterface + case *types.Array: + return asmArray + case *types.Slice: + return asmSlice + } + panic("unreachable") +} + +// A component is an assembly-addressable component of a composite type, +// or a composite type itself. +type component struct { + size int + offset int + kind asmKind + typ string + suffix string // Such as _base for string base, _0_lo for lo half of first element of [1]uint64 on 32 bit machine. + outer string // The suffix for immediately containing composite type. +} + +func newComponent(suffix string, kind asmKind, typ string, offset, size int, outer string) component { + return component{suffix: suffix, kind: kind, typ: typ, offset: offset, size: size, outer: outer} +} + +// componentsOfType generates a list of components of type t. +// For example, given string, the components are the string itself, the base, and the length. +func componentsOfType(arch *asmArch, t types.Type) []component { + return appendComponentsRecursive(arch, t, nil, "", 0) +} + +// appendComponentsRecursive implements componentsOfType. +// Recursion is required to correct handle structs and arrays, +// which can contain arbitrary other types. +func appendComponentsRecursive(arch *asmArch, t types.Type, cc []component, suffix string, off int) []component { + s := t.String() + size := int(arch.sizes.Sizeof(t)) + kind := asmKindForType(t, size) + cc = append(cc, newComponent(suffix, kind, s, off, size, suffix)) + + switch kind { + case 8: + if arch.ptrSize() == 4 { + w1, w2 := "lo", "hi" + if arch.bigEndian { + w1, w2 = w2, w1 + } + cc = append(cc, newComponent(suffix+"_"+w1, 4, "half "+s, off, 4, suffix)) + cc = append(cc, newComponent(suffix+"_"+w2, 4, "half "+s, off+4, 4, suffix)) + } + + case asmEmptyInterface: + cc = append(cc, newComponent(suffix+"_type", asmKind(arch.ptrSize()), "interface type", off, arch.ptrSize(), suffix)) + cc = append(cc, newComponent(suffix+"_data", asmKind(arch.ptrSize()), "interface data", off+arch.ptrSize(), arch.ptrSize(), suffix)) + + case asmInterface: + cc = append(cc, newComponent(suffix+"_itable", asmKind(arch.ptrSize()), "interface itable", off, arch.ptrSize(), suffix)) + cc = append(cc, newComponent(suffix+"_data", asmKind(arch.ptrSize()), "interface data", off+arch.ptrSize(), arch.ptrSize(), suffix)) + + case asmSlice: + cc = append(cc, newComponent(suffix+"_base", asmKind(arch.ptrSize()), "slice base", off, arch.ptrSize(), suffix)) + cc = append(cc, newComponent(suffix+"_len", asmKind(arch.intSize()), "slice len", off+arch.ptrSize(), arch.intSize(), suffix)) + cc = append(cc, newComponent(suffix+"_cap", asmKind(arch.intSize()), "slice cap", off+arch.ptrSize()+arch.intSize(), arch.intSize(), suffix)) + + case asmString: + cc = append(cc, newComponent(suffix+"_base", asmKind(arch.ptrSize()), "string base", off, arch.ptrSize(), suffix)) + cc = append(cc, newComponent(suffix+"_len", asmKind(arch.intSize()), "string len", off+arch.ptrSize(), arch.intSize(), suffix)) + + case asmComplex: + fsize := size / 2 + cc = append(cc, newComponent(suffix+"_real", asmKind(fsize), fmt.Sprintf("real(complex%d)", size*8), off, fsize, suffix)) + cc = append(cc, newComponent(suffix+"_imag", asmKind(fsize), fmt.Sprintf("imag(complex%d)", size*8), off+fsize, fsize, suffix)) + + case asmStruct: + tu := t.Underlying().(*types.Struct) + fields := make([]*types.Var, tu.NumFields()) + for i := 0; i < tu.NumFields(); i++ { + fields[i] = tu.Field(i) + } + offsets := arch.sizes.Offsetsof(fields) + for i, f := range fields { + cc = appendComponentsRecursive(arch, f.Type(), cc, suffix+"_"+f.Name(), off+int(offsets[i])) + } + + case asmArray: + tu := t.Underlying().(*types.Array) + elem := tu.Elem() + // Calculate offset of each element array. + fields := []*types.Var{ + types.NewVar(token.NoPos, nil, "fake0", elem), + types.NewVar(token.NoPos, nil, "fake1", elem), + } + offsets := arch.sizes.Offsetsof(fields) + elemoff := int(offsets[1]) + for i := 0; i < int(tu.Len()); i++ { + cc = appendComponentsRecursive(arch, elem, cc, suffix+"_"+strconv.Itoa(i), i*elemoff) + } + } + + return cc +} + // asmParseDecl parses a function decl for expected assembly variables. func (f *File) asmParseDecl(decl *ast.FuncDecl) map[string]*asmFunc { var ( arch *asmArch fn *asmFunc offset int - failed bool ) - addVar := func(outer string, v asmVar) { - if vo := fn.vars[outer]; vo != nil { - vo.inner = append(vo.inner, &v) - } - fn.vars[v.name] = &v - for i := 0; i < v.size; i++ { - fn.varByOffset[v.off+i] = &v - } - } - - addParams := func(list []*ast.Field) { - for i, fld := range list { - // Determine alignment, size, and kind of type in declaration. - var align, size int - var kind asmKind - names := fld.Names - typ := f.gofmt(fld.Type) - switch t := fld.Type.(type) { - default: - switch typ { - default: - f.Warnf(fld.Type.Pos(), "unknown assembly argument type %s", typ) - failed = true - return - case "int8", "uint8", "byte", "bool": - size = 1 - case "int16", "uint16": - size = 2 - case "int32", "uint32", "float32": - size = 4 - case "int64", "uint64", "float64": - align = arch.maxAlign - size = 8 - case "int", "uint": - size = arch.intSize - case "uintptr", "iword", "Word", "Errno", "unsafe.Pointer": - size = arch.ptrSize - case "string", "ErrorString": - size = arch.ptrSize * 2 - align = arch.ptrSize - kind = asmString - } - case *ast.ChanType, *ast.FuncType, *ast.MapType, *ast.StarExpr: - size = arch.ptrSize - case *ast.InterfaceType: - align = arch.ptrSize - size = 2 * arch.ptrSize - if len(t.Methods.List) > 0 { - kind = asmInterface - } else { - kind = asmEmptyInterface - } - case *ast.ArrayType: - if t.Len == nil { - size = arch.ptrSize + 2*arch.intSize - align = arch.ptrSize - kind = asmSlice - break - } - f.Warnf(fld.Type.Pos(), "unsupported assembly argument type %s", typ) - failed = true - case *ast.StructType: - f.Warnf(fld.Type.Pos(), "unsupported assembly argument type %s", typ) - failed = true - } - if align == 0 { - align = size - } - if kind == 0 { - kind = asmKind(size) - } + // addParams adds asmVars for each of the parameters in list. + // isret indicates whether the list are the arguments or the return values. + addParams := func(list []*ast.Field, isret bool) { + argnum := 0 + for _, fld := range list { + t := f.pkg.types[fld.Type].Type + align := int(arch.sizes.Alignof(t)) + size := int(arch.sizes.Sizeof(t)) offset += -offset & (align - 1) + cc := componentsOfType(arch, t) - // Create variable for each name being declared with this type. + // names is the list of names with this type. + names := fld.Names if len(names) == 0 { - name := "unnamed" - if decl.Type.Results != nil && len(decl.Type.Results.List) > 0 && &list[0] == &decl.Type.Results.List[0] && i == 0 { - // Assume assembly will refer to single unnamed result as r. + // Anonymous args will be called arg, arg1, arg2, ... + // Similarly so for return values: ret, ret1, ret2, ... + name := "arg" + if isret { name = "ret" } - names = []*ast.Ident{{Name: name}} + if argnum > 0 { + name += strconv.Itoa(argnum) + } + names = []*ast.Ident{ast.NewIdent(name)} } + argnum += len(names) + + // Create variable for each name. for _, id := range names { name := id.Name - addVar("", asmVar{ - name: name, - kind: kind, - typ: typ, - off: offset, - size: size, - }) - switch kind { - case 8: - if arch.ptrSize == 4 { - w1, w2 := "lo", "hi" - if arch.bigEndian { - w1, w2 = w2, w1 - } - addVar(name, asmVar{ - name: name + "_" + w1, - kind: 4, - typ: "half " + typ, - off: offset, - size: 4, - }) - addVar(name, asmVar{ - name: name + "_" + w2, - kind: 4, - typ: "half " + typ, - off: offset + 4, - size: 4, - }) + for _, c := range cc { + outer := name + c.outer + v := asmVar{ + name: name + c.suffix, + kind: c.kind, + typ: c.typ, + off: offset + c.offset, + size: c.size, + } + if vo := fn.vars[outer]; vo != nil { + vo.inner = append(vo.inner, &v) + } + fn.vars[v.name] = &v + for i := 0; i < v.size; i++ { + fn.varByOffset[v.off+i] = &v } - - case asmEmptyInterface: - addVar(name, asmVar{ - name: name + "_type", - kind: asmKind(arch.ptrSize), - typ: "interface type", - off: offset, - size: arch.ptrSize, - }) - addVar(name, asmVar{ - name: name + "_data", - kind: asmKind(arch.ptrSize), - typ: "interface data", - off: offset + arch.ptrSize, - size: arch.ptrSize, - }) - - case asmInterface: - addVar(name, asmVar{ - name: name + "_itable", - kind: asmKind(arch.ptrSize), - typ: "interface itable", - off: offset, - size: arch.ptrSize, - }) - addVar(name, asmVar{ - name: name + "_data", - kind: asmKind(arch.ptrSize), - typ: "interface data", - off: offset + arch.ptrSize, - size: arch.ptrSize, - }) - - case asmSlice: - addVar(name, asmVar{ - name: name + "_base", - kind: asmKind(arch.ptrSize), - typ: "slice base", - off: offset, - size: arch.ptrSize, - }) - addVar(name, asmVar{ - name: name + "_len", - kind: asmKind(arch.intSize), - typ: "slice len", - off: offset + arch.ptrSize, - size: arch.intSize, - }) - addVar(name, asmVar{ - name: name + "_cap", - kind: asmKind(arch.intSize), - typ: "slice cap", - off: offset + arch.ptrSize + arch.intSize, - size: arch.intSize, - }) - - case asmString: - addVar(name, asmVar{ - name: name + "_base", - kind: asmKind(arch.ptrSize), - typ: "string base", - off: offset, - size: arch.ptrSize, - }) - addVar(name, asmVar{ - name: name + "_len", - kind: asmKind(arch.intSize), - typ: "string len", - off: offset + arch.ptrSize, - size: arch.intSize, - }) } offset += size } @@ -517,18 +510,15 @@ func (f *File) asmParseDecl(decl *ast.FuncDecl) map[string]*asmFunc { varByOffset: make(map[int]*asmVar), } offset = 0 - addParams(decl.Type.Params.List) + addParams(decl.Type.Params.List, false) if decl.Type.Results != nil && len(decl.Type.Results.List) > 0 { - offset += -offset & (arch.maxAlign - 1) - addParams(decl.Type.Results.List) + offset += -offset & (arch.maxAlign() - 1) + addParams(decl.Type.Results.List, true) } fn.size = offset m[arch.name] = fn } - if failed { - return nil - } return m } @@ -649,11 +639,13 @@ func asmCheckVar(badf func(string, ...interface{}), fn *asmFunc, line, expr stri } vk := v.kind + vs := v.size vt := v.typ switch vk { case asmInterface, asmEmptyInterface, asmString, asmSlice: // allow reference to first word (pointer) vk = v.inner[0].kind + vs = v.inner[0].size vt = v.inner[0].typ } @@ -687,6 +679,6 @@ func asmCheckVar(badf func(string, ...interface{}), fn *asmFunc, line, expr stri fmt.Fprintf(&inner, "%s+%d(FP)", vi.name, vi.off) } } - badf("invalid %s of %s; %s is %d-byte value%s", op, expr, vt, vk, inner.String()) + badf("invalid %s of %s; %s is %d-byte value%s", op, expr, vt, vs, inner.String()) } } diff --git a/src/cmd/vet/testdata/asm.go b/src/cmd/vet/testdata/asm.go index 81947102ec..3f9275c008 100644 --- a/src/cmd/vet/testdata/asm.go +++ b/src/cmd/vet/testdata/asm.go @@ -8,6 +8,12 @@ package testdata +type S struct { + i int32 + b bool + s string +} + func arg1(x int8, y uint8) func arg2(x int16, y uint16) func arg4(x int32, y uint32) @@ -19,6 +25,9 @@ func argslice(x, y []string) func argiface(x interface{}, y interface { m() }) +func argcomplex(x complex64, y complex128) +func argstruct(x S, y struct{}) +func argarray(x [2]S) func returnint() int func returnbyte(x int) byte func returnnamed(x byte) (r1 int, r2 int16, r3 string, r4 byte) diff --git a/src/cmd/vet/testdata/asm1.s b/src/cmd/vet/testdata/asm1.s index 2c6f13b137..bc8cbc2e79 100644 --- a/src/cmd/vet/testdata/asm1.s +++ b/src/cmd/vet/testdata/asm1.s @@ -221,6 +221,49 @@ TEXT ·argiface(SB),0,$0-32 MOVQ y_data+24(FP), AX RET +TEXT ·argcomplex(SB),0,$24 // ERROR "wrong argument size 0; expected \$\.\.\.-24" + MOVSS x+0(FP), X0 // ERROR "invalid MOVSS of x\+0\(FP\); complex64 is 8-byte value containing x_real\+0\(FP\) and x_imag\+4\(FP\)" + MOVSD x+0(FP), X0 // ERROR "invalid MOVSD of x\+0\(FP\); complex64 is 8-byte value containing x_real\+0\(FP\) and x_imag\+4\(FP\)" + MOVSS x_real+0(FP), X0 + MOVSD x_real+0(FP), X0 // ERROR "invalid MOVSD of x_real\+0\(FP\); real\(complex64\) is 4-byte value" + MOVSS x_real+4(FP), X0 // ERROR "invalid offset x_real\+4\(FP\); expected x_real\+0\(FP\)" + MOVSS x_imag+4(FP), X0 + MOVSD x_imag+4(FP), X0 // ERROR "invalid MOVSD of x_imag\+4\(FP\); imag\(complex64\) is 4-byte value" + MOVSS x_imag+8(FP), X0 // ERROR "invalid offset x_imag\+8\(FP\); expected x_imag\+4\(FP\)" + MOVSD y+8(FP), X0 // ERROR "invalid MOVSD of y\+8\(FP\); complex128 is 16-byte value containing y_real\+8\(FP\) and y_imag\+16\(FP\)" + MOVSS y_real+8(FP), X0 // ERROR "invalid MOVSS of y_real\+8\(FP\); real\(complex128\) is 8-byte value" + MOVSD y_real+8(FP), X0 + MOVSS y_real+16(FP), X0 // ERROR "invalid offset y_real\+16\(FP\); expected y_real\+8\(FP\)" + MOVSS y_imag+16(FP), X0 // ERROR "invalid MOVSS of y_imag\+16\(FP\); imag\(complex128\) is 8-byte value" + MOVSD y_imag+16(FP), X0 + MOVSS y_imag+24(FP), X0 // ERROR "invalid offset y_imag\+24\(FP\); expected y_imag\+16\(FP\)" + RET + +TEXT ·argstruct(SB),0,$64 // ERROR "wrong argument size 0; expected \$\.\.\.-24" + MOVQ x+0(FP), AX // ERROR "invalid MOVQ of x\+0\(FP\); testdata.S is 24-byte value" + MOVQ x_i+0(FP), AX // ERROR "invalid MOVQ of x_i\+0\(FP\); int32 is 4-byte value" + MOVQ x_b+0(FP), AX // ERROR "invalid offset x_b\+0\(FP\); expected x_b\+4\(FP\)" + MOVQ x_s+8(FP), AX + MOVQ x_s_base+8(FP), AX + MOVQ x_s+16(FP), AX // ERROR "invalid offset x_s\+16\(FP\); expected x_s\+8\(FP\), x_s_base\+8\(FP\), or x_s_len\+16\(FP\)" + MOVQ x_s_len+16(FP), AX + RET + +TEXT ·argarray(SB),0,$64 // ERROR "wrong argument size 0; expected \$\.\.\.-48" + MOVQ x+0(FP), AX // ERROR "invalid MOVQ of x\+0\(FP\); \[2\]testdata.S is 48-byte value" + MOVQ x_0_i+0(FP), AX // ERROR "invalid MOVQ of x_0_i\+0\(FP\); int32 is 4-byte value" + MOVQ x_0_b+0(FP), AX // ERROR "invalid offset x_0_b\+0\(FP\); expected x_0_b\+4\(FP\)" + MOVQ x_0_s+8(FP), AX + MOVQ x_0_s_base+8(FP), AX + MOVQ x_0_s+16(FP), AX // ERROR "invalid offset x_0_s\+16\(FP\); expected x_0_s\+8\(FP\), x_0_s_base\+8\(FP\), or x_0_s_len\+16\(FP\)" + MOVQ x_0_s_len+16(FP), AX + MOVB foo+25(FP), AX // ERROR "unknown variable foo; offset 25 is x_1_i\+24\(FP\)" + MOVQ x_1_s+32(FP), AX + MOVQ x_1_s_base+32(FP), AX + MOVQ x_1_s+40(FP), AX // ERROR "invalid offset x_1_s\+40\(FP\); expected x_1_s\+32\(FP\), x_1_s_base\+32\(FP\), or x_1_s_len\+40\(FP\)" + MOVQ x_1_s_len+40(FP), AX + RET + TEXT ·returnint(SB),0,$0-8 MOVB AX, ret+0(FP) // ERROR "invalid MOVB of ret\+0\(FP\); int is 8-byte value" MOVW AX, ret+0(FP) // ERROR "invalid MOVW of ret\+0\(FP\); int is 8-byte value" diff --git a/src/runtime/sys_darwin_arm.s b/src/runtime/sys_darwin_arm.s index 52f6a94d46..985ff50245 100644 --- a/src/runtime/sys_darwin_arm.s +++ b/src/runtime/sys_darwin_arm.s @@ -106,7 +106,7 @@ TEXT runtime·raiseproc(SB),NOSPLIT,$24 MOVW $SYS_getpid, R12 SWI $0x80 // arg 1 pid already in R0 from getpid - MOVW unnamed+0(FP), R1 // arg 2 - signal + MOVW arg+0(FP), R1 // arg 2 - signal MOVW $1, R2 // arg 3 - posix MOVW $SYS_kill, R12 SWI $0x80 diff --git a/src/runtime/sys_plan9_arm.s b/src/runtime/sys_plan9_arm.s index 6dee611fbd..bc7a90b4d9 100644 --- a/src/runtime/sys_plan9_arm.s +++ b/src/runtime/sys_plan9_arm.s @@ -131,7 +131,7 @@ TEXT runtime·plan9_tsemacquire(SB),NOSPLIT,$0-12 TEXT runtime·nsec(SB),NOSPLIT,$-4-12 MOVW $SYS_NSEC, R0 SWI 0 - MOVW unnamed+0(FP), R1 + MOVW arg+0(FP), R1 MOVW 0(R1), R0 MOVW R0, ret_lo+4(FP) MOVW 4(R1), R0 -- 2.48.1