diff options
author | Alan Donovan <adonovan@google.com> | 2018-10-08 20:58:01 -0400 |
---|---|---|
committer | Alan Donovan <adonovan@google.com> | 2018-10-11 18:00:20 +0000 |
commit | 6979e85b73a6fc202a05126152f035eb7e70b9c0 (patch) | |
tree | 4d6a997b7c30ceccbd320afb5ddefb400c6df3f3 | |
parent | a398e557df606a73ba39c8fd09a1b392b2a7ab59 (diff) | |
download | golang-x-tools-6979e85b73a6fc202a05126152f035eb7e70b9c0.tar.gz |
go/analysis/passes/shift: split out of vet
Change-Id: I9c0c86e7aee4f7bb9239dc4b4836df9bba471b03
Reviewed-on: https://go-review.googlesource.com/c/140757
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
-rw-r--r-- | go/analysis/passes/shift/dead.go (renamed from go/analysis/passes/vet/dead.go) | 65 | ||||
-rw-r--r-- | go/analysis/passes/shift/shift.go | 126 | ||||
-rw-r--r-- | go/analysis/passes/shift/shift_test.go | 13 | ||||
-rw-r--r-- | go/analysis/passes/shift/testdata/src/a/a.go | 155 | ||||
-rw-r--r-- | go/analysis/passes/vet/shift.go | 100 | ||||
-rw-r--r-- | go/analysis/passes/vet/testdata/shift.go | 162 |
6 files changed, 322 insertions, 299 deletions
diff --git a/go/analysis/passes/vet/dead.go b/go/analysis/passes/shift/dead.go index 3eb532308..43415a98d 100644 --- a/go/analysis/passes/vet/dead.go +++ b/go/analysis/passes/shift/dead.go @@ -1,39 +1,50 @@ -// +build ignore - // Copyright 2017 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. -// -// Simplified dead code detector. Used for skipping certain checks -// on unreachable code (for instance, shift checks on arch-specific code). -package main +package shift + +// Simplified dead code detector. +// Used for skipping shift checks on unreachable arch-specific code. import ( "go/ast" "go/constant" + "go/types" ) -// updateDead puts unreachable "if" and "case" nodes into f.dead. -func (f *File) updateDead(node ast.Node) { - if f.dead[node] { +// updateDead puts unreachable "if" and "case" nodes into dead. +func updateDead(info *types.Info, dead map[ast.Node]bool, node ast.Node) { + if dead[node] { // The node is already marked as dead. return } + // setDead marks the node and all the children as dead. + setDead := func(n ast.Node) { + ast.Inspect(n, func(node ast.Node) bool { + if node != nil { + dead[node] = true + } + return true + }) + } + switch stmt := node.(type) { case *ast.IfStmt: // "if" branch is dead if its condition evaluates // to constant false. - v := f.pkg.types[stmt.Cond].Value + v := info.Types[stmt.Cond].Value if v == nil { return } if !constant.BoolVal(v) { - f.setDead(stmt.Body) + setDead(stmt.Body) return } - f.setDead(stmt.Else) + if stmt.Else != nil { + setDead(stmt.Else) + } case *ast.SwitchStmt: // Case clause with empty switch tag is dead if it evaluates // to constant false. @@ -46,12 +57,12 @@ func (f *File) updateDead(node ast.Node) { continue } for _, expr := range cc.List { - v := f.pkg.types[expr].Value + v := info.Types[expr].Value if v == nil || v.Kind() != constant.Bool || constant.BoolVal(v) { continue BodyLoopBool } } - f.setDead(cc) + setDead(cc) } return } @@ -59,7 +70,7 @@ func (f *File) updateDead(node ast.Node) { // Case clause is dead if its constant value doesn't match // the constant value from the switch tag. // TODO: This handles integer comparisons only. - v := f.pkg.types[stmt.Tag].Value + v := info.Types[stmt.Tag].Value if v == nil || v.Kind() != constant.Int { return } @@ -75,7 +86,7 @@ func (f *File) updateDead(node ast.Node) { continue } for _, expr := range cc.List { - v := f.pkg.types[expr].Value + v := info.Types[expr].Value if v == nil { continue BodyLoopInt } @@ -84,27 +95,7 @@ func (f *File) updateDead(node ast.Node) { continue BodyLoopInt } } - f.setDead(cc) + setDead(cc) } } } - -// setDead marks the node and all the children as dead. -func (f *File) setDead(node ast.Node) { - dv := deadVisitor{ - f: f, - } - ast.Walk(dv, node) -} - -type deadVisitor struct { - f *File -} - -func (dv deadVisitor) Visit(node ast.Node) ast.Visitor { - if node == nil { - return nil - } - dv.f.dead[node] = true - return dv -} diff --git a/go/analysis/passes/shift/shift.go b/go/analysis/passes/shift/shift.go new file mode 100644 index 000000000..889f5f383 --- /dev/null +++ b/go/analysis/passes/shift/shift.go @@ -0,0 +1,126 @@ +// Copyright 2014 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. + +package shift + +// TODO(adonovan): integrate with ctrflow (CFG-based) dead code analysis. May +// have impedance mismatch due to its (non-)treatment of constant +// expressions (such as runtime.GOARCH=="386"). + +import ( + "go/ast" + "go/build" + "go/constant" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/analysis/passes/internal/analysisutil" + "golang.org/x/tools/go/ast/inspector" +) + +var Analyzer = &analysis.Analyzer{ + Name: "shift", + Doc: "check for shifts that equal or exceed the width of the integer", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + // Do a complete pass to compute dead nodes. + dead := make(map[ast.Node]bool) + nodeFilter := []ast.Node{ + (*ast.IfStmt)(nil), + (*ast.SwitchStmt)(nil), + } + inspect.Preorder(nodeFilter, func(n ast.Node) { + // TODO(adonovan): move updateDead into this file. + updateDead(pass.TypesInfo, dead, n) + }) + + nodeFilter = []ast.Node{ + (*ast.AssignStmt)(nil), + (*ast.BinaryExpr)(nil), + } + inspect.Preorder(nodeFilter, func(node ast.Node) { + if dead[node] { + // Skip shift checks on unreachable nodes. + return + } + + switch node := node.(type) { + case *ast.BinaryExpr: + if node.Op == token.SHL || node.Op == token.SHR { + checkLongShift(pass, node, node.X, node.Y) + } + case *ast.AssignStmt: + if len(node.Lhs) != 1 || len(node.Rhs) != 1 { + return + } + if node.Tok == token.SHL_ASSIGN || node.Tok == token.SHR_ASSIGN { + checkLongShift(pass, node, node.Lhs[0], node.Rhs[0]) + } + } + }) + return nil, nil +} + +// checkLongShift checks if shift or shift-assign operations shift by more than +// the length of the underlying variable. +func checkLongShift(pass *analysis.Pass, node ast.Node, x, y ast.Expr) { + if pass.TypesInfo.Types[x].Value != nil { + // Ignore shifts of constants. + // These are frequently used for bit-twiddling tricks + // like ^uint(0) >> 63 for 32/64 bit detection and compatibility. + return + } + + v := pass.TypesInfo.Types[y].Value + if v == nil { + return + } + amt, ok := constant.Int64Val(v) + if !ok { + return + } + t := pass.TypesInfo.Types[x].Type + if t == nil { + return + } + b, ok := t.Underlying().(*types.Basic) + if !ok { + return + } + var size int64 + switch b.Kind() { + case types.Uint8, types.Int8: + size = 8 + case types.Uint16, types.Int16: + size = 16 + case types.Uint32, types.Int32: + size = 32 + case types.Uint64, types.Int64: + size = 64 + case types.Int, types.Uint: + size = uintBitSize + case types.Uintptr: + size = uintptrBitSize + default: + return + } + if amt >= size { + ident := analysisutil.Format(pass.Fset, x) + pass.Reportf(node.Pos(), "%s (%d bits) too small for shift of %d", ident, size, amt) + } +} + +var ( + uintBitSize = 8 * archSizes.Sizeof(types.Typ[types.Uint]) + uintptrBitSize = 8 * archSizes.Sizeof(types.Typ[types.Uintptr]) +) + +var archSizes = types.SizesFor("gc", build.Default.GOARCH) diff --git a/go/analysis/passes/shift/shift_test.go b/go/analysis/passes/shift/shift_test.go new file mode 100644 index 000000000..56741d8e7 --- /dev/null +++ b/go/analysis/passes/shift/shift_test.go @@ -0,0 +1,13 @@ +package shift_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/shift" +) + +func TestFromFileSystem(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, shift.Analyzer, "a") +} diff --git a/go/analysis/passes/shift/testdata/src/a/a.go b/go/analysis/passes/shift/testdata/src/a/a.go new file mode 100644 index 000000000..796fcaa6e --- /dev/null +++ b/go/analysis/passes/shift/testdata/src/a/a.go @@ -0,0 +1,155 @@ +// Copyright 2014 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. + +// This file contains tests for the suspicious shift checker. + +package shift + +import ( + "unsafe" +) + +func ShiftTest() { + var i8 int8 + _ = i8 << 7 + _ = (i8 + 1) << 8 // want ".i8 . 1. .8 bits. too small for shift of 8" + _ = i8 << (7 + 1) // want "i8 .8 bits. too small for shift of 8" + _ = i8 >> 8 // want "i8 .8 bits. too small for shift of 8" + i8 <<= 8 // want "i8 .8 bits. too small for shift of 8" + i8 >>= 8 // want "i8 .8 bits. too small for shift of 8" + var i16 int16 + _ = i16 << 15 + _ = i16 << 16 // want "i16 .16 bits. too small for shift of 16" + _ = i16 >> 16 // want "i16 .16 bits. too small for shift of 16" + i16 <<= 16 // want "i16 .16 bits. too small for shift of 16" + i16 >>= 16 // want "i16 .16 bits. too small for shift of 16" + var i32 int32 + _ = i32 << 31 + _ = i32 << 32 // want "i32 .32 bits. too small for shift of 32" + _ = i32 >> 32 // want "i32 .32 bits. too small for shift of 32" + i32 <<= 32 // want "i32 .32 bits. too small for shift of 32" + i32 >>= 32 // want "i32 .32 bits. too small for shift of 32" + var i64 int64 + _ = i64 << 63 + _ = i64 << 64 // want "i64 .64 bits. too small for shift of 64" + _ = i64 >> 64 // want "i64 .64 bits. too small for shift of 64" + i64 <<= 64 // want "i64 .64 bits. too small for shift of 64" + i64 >>= 64 // want "i64 .64 bits. too small for shift of 64" + var u8 uint8 + _ = u8 << 7 + _ = u8 << 8 // want "u8 .8 bits. too small for shift of 8" + _ = u8 >> 8 // want "u8 .8 bits. too small for shift of 8" + u8 <<= 8 // want "u8 .8 bits. too small for shift of 8" + u8 >>= 8 // want "u8 .8 bits. too small for shift of 8" + var u16 uint16 + _ = u16 << 15 + _ = u16 << 16 // want "u16 .16 bits. too small for shift of 16" + _ = u16 >> 16 // want "u16 .16 bits. too small for shift of 16" + u16 <<= 16 // want "u16 .16 bits. too small for shift of 16" + u16 >>= 16 // want "u16 .16 bits. too small for shift of 16" + var u32 uint32 + _ = u32 << 31 + _ = u32 << 32 // want "u32 .32 bits. too small for shift of 32" + _ = u32 >> 32 // want "u32 .32 bits. too small for shift of 32" + u32 <<= 32 // want "u32 .32 bits. too small for shift of 32" + u32 >>= 32 // want "u32 .32 bits. too small for shift of 32" + var u64 uint64 + _ = u64 << 63 + _ = u64 << 64 // want "u64 .64 bits. too small for shift of 64" + _ = u64 >> 64 // want "u64 .64 bits. too small for shift of 64" + u64 <<= 64 // want "u64 .64 bits. too small for shift of 64" + u64 >>= 64 // want "u64 .64 bits. too small for shift of 64" + _ = u64 << u64 // Non-constant shifts should succeed. + + var i int + _ = i << 31 + const in = 8 * unsafe.Sizeof(i) + _ = i << in // want "too small for shift" + _ = i >> in // want "too small for shift" + i <<= in // want "too small for shift" + i >>= in // want "too small for shift" + const ix = 8*unsafe.Sizeof(i) - 1 + _ = i << ix + _ = i >> ix + i <<= ix + i >>= ix + + var u uint + _ = u << 31 + const un = 8 * unsafe.Sizeof(u) + _ = u << un // want "too small for shift" + _ = u >> un // want "too small for shift" + u <<= un // want "too small for shift" + u >>= un // want "too small for shift" + const ux = 8*unsafe.Sizeof(u) - 1 + _ = u << ux + _ = u >> ux + u <<= ux + u >>= ux + + var p uintptr + _ = p << 31 + const pn = 8 * unsafe.Sizeof(p) + _ = p << pn // want "too small for shift" + _ = p >> pn // want "too small for shift" + p <<= pn // want "too small for shift" + p >>= pn // want "too small for shift" + const px = 8*unsafe.Sizeof(p) - 1 + _ = p << px + _ = p >> px + p <<= px + p >>= px + + const oneIf64Bit = ^uint(0) >> 63 // allow large shifts of constants; they are used for 32/64 bit compatibility tricks + + var h uintptr + h = h<<8 | (h >> (8 * (unsafe.Sizeof(h) - 1))) + h <<= 8 * unsafe.Sizeof(h) // want "too small for shift" + h >>= 7 * unsafe.Alignof(h) + h >>= 8 * unsafe.Alignof(h) // want "too small for shift" +} + +func ShiftDeadCode() { + var i int + const iBits = 8 * unsafe.Sizeof(i) + + if iBits <= 32 { + if iBits == 16 { + _ = i >> 8 + } else { + _ = i >> 16 + } + } else { + _ = i >> 32 + } + + if iBits >= 64 { + _ = i << 32 + if iBits == 128 { + _ = i << 64 + } + } else { + _ = i << 16 + } + + if iBits == 64 { + _ = i << 32 + } + + switch iBits { + case 128, 64: + _ = i << 32 + default: + _ = i << 16 + } + + switch { + case iBits < 32: + _ = i << 16 + case iBits > 64: + _ = i << 64 + default: + _ = i << 64 // want "too small for shift" + } +} diff --git a/go/analysis/passes/vet/shift.go b/go/analysis/passes/vet/shift.go deleted file mode 100644 index 5ddd0cb4b..000000000 --- a/go/analysis/passes/vet/shift.go +++ /dev/null @@ -1,100 +0,0 @@ -// +build ignore - -// Copyright 2014 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. - -/* -This file contains the code to check for suspicious shifts. -*/ - -package main - -import ( - "go/ast" - "go/constant" - "go/token" - "go/types" -) - -func init() { - register("shift", - "check for useless shifts", - checkShift, - binaryExpr, assignStmt) -} - -func checkShift(f *File, node ast.Node) { - if f.dead[node] { - // Skip shift checks on unreachable nodes. - return - } - - switch node := node.(type) { - case *ast.BinaryExpr: - if node.Op == token.SHL || node.Op == token.SHR { - checkLongShift(f, node, node.X, node.Y) - } - case *ast.AssignStmt: - if len(node.Lhs) != 1 || len(node.Rhs) != 1 { - return - } - if node.Tok == token.SHL_ASSIGN || node.Tok == token.SHR_ASSIGN { - checkLongShift(f, node, node.Lhs[0], node.Rhs[0]) - } - } -} - -// checkLongShift checks if shift or shift-assign operations shift by more than -// the length of the underlying variable. -func checkLongShift(f *File, node ast.Node, x, y ast.Expr) { - if f.pkg.types[x].Value != nil { - // Ignore shifts of constants. - // These are frequently used for bit-twiddling tricks - // like ^uint(0) >> 63 for 32/64 bit detection and compatibility. - return - } - - v := f.pkg.types[y].Value - if v == nil { - return - } - amt, ok := constant.Int64Val(v) - if !ok { - return - } - t := f.pkg.types[x].Type - if t == nil { - return - } - b, ok := t.Underlying().(*types.Basic) - if !ok { - return - } - var size int64 - switch b.Kind() { - case types.Uint8, types.Int8: - size = 8 - case types.Uint16, types.Int16: - size = 16 - case types.Uint32, types.Int32: - size = 32 - case types.Uint64, types.Int64: - size = 64 - case types.Int, types.Uint: - size = uintBitSize - case types.Uintptr: - size = uintptrBitSize - default: - return - } - if amt >= size { - ident := f.gofmt(x) - f.Badf(node.Pos(), "%s (%d bits) too small for shift of %d", ident, size, amt) - } -} - -var ( - uintBitSize = 8 * archSizes.Sizeof(types.Typ[types.Uint]) - uintptrBitSize = 8 * archSizes.Sizeof(types.Typ[types.Uintptr]) -) diff --git a/go/analysis/passes/vet/testdata/shift.go b/go/analysis/passes/vet/testdata/shift.go deleted file mode 100644 index 73cbaf884..000000000 --- a/go/analysis/passes/vet/testdata/shift.go +++ /dev/null @@ -1,162 +0,0 @@ -// Copyright 2014 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. - -// This file contains tests for the suspicious shift checker. - -package testdata - -import ( - "fmt" - "unsafe" -) - -func ShiftTest() { - var i8 int8 - _ = i8 << 7 - _ = (i8 + 1) << 8 // ERROR ".i8 . 1. .8 bits. too small for shift of 8" - _ = i8 << (7 + 1) // ERROR "i8 .8 bits. too small for shift of 8" - _ = i8 >> 8 // ERROR "i8 .8 bits. too small for shift of 8" - i8 <<= 8 // ERROR "i8 .8 bits. too small for shift of 8" - i8 >>= 8 // ERROR "i8 .8 bits. too small for shift of 8" - var i16 int16 - _ = i16 << 15 - _ = i16 << 16 // ERROR "i16 .16 bits. too small for shift of 16" - _ = i16 >> 16 // ERROR "i16 .16 bits. too small for shift of 16" - i16 <<= 16 // ERROR "i16 .16 bits. too small for shift of 16" - i16 >>= 16 // ERROR "i16 .16 bits. too small for shift of 16" - var i32 int32 - _ = i32 << 31 - _ = i32 << 32 // ERROR "i32 .32 bits. too small for shift of 32" - _ = i32 >> 32 // ERROR "i32 .32 bits. too small for shift of 32" - i32 <<= 32 // ERROR "i32 .32 bits. too small for shift of 32" - i32 >>= 32 // ERROR "i32 .32 bits. too small for shift of 32" - var i64 int64 - _ = i64 << 63 - _ = i64 << 64 // ERROR "i64 .64 bits. too small for shift of 64" - _ = i64 >> 64 // ERROR "i64 .64 bits. too small for shift of 64" - i64 <<= 64 // ERROR "i64 .64 bits. too small for shift of 64" - i64 >>= 64 // ERROR "i64 .64 bits. too small for shift of 64" - var u8 uint8 - _ = u8 << 7 - _ = u8 << 8 // ERROR "u8 .8 bits. too small for shift of 8" - _ = u8 >> 8 // ERROR "u8 .8 bits. too small for shift of 8" - u8 <<= 8 // ERROR "u8 .8 bits. too small for shift of 8" - u8 >>= 8 // ERROR "u8 .8 bits. too small for shift of 8" - var u16 uint16 - _ = u16 << 15 - _ = u16 << 16 // ERROR "u16 .16 bits. too small for shift of 16" - _ = u16 >> 16 // ERROR "u16 .16 bits. too small for shift of 16" - u16 <<= 16 // ERROR "u16 .16 bits. too small for shift of 16" - u16 >>= 16 // ERROR "u16 .16 bits. too small for shift of 16" - var u32 uint32 - _ = u32 << 31 - _ = u32 << 32 // ERROR "u32 .32 bits. too small for shift of 32" - _ = u32 >> 32 // ERROR "u32 .32 bits. too small for shift of 32" - u32 <<= 32 // ERROR "u32 .32 bits. too small for shift of 32" - u32 >>= 32 // ERROR "u32 .32 bits. too small for shift of 32" - var u64 uint64 - _ = u64 << 63 - _ = u64 << 64 // ERROR "u64 .64 bits. too small for shift of 64" - _ = u64 >> 64 // ERROR "u64 .64 bits. too small for shift of 64" - u64 <<= 64 // ERROR "u64 .64 bits. too small for shift of 64" - u64 >>= 64 // ERROR "u64 .64 bits. too small for shift of 64" - _ = u64 << u64 // Non-constant shifts should succeed. - - var i int - _ = i << 31 - const in = 8 * unsafe.Sizeof(i) - _ = i << in // ERROR "too small for shift" - _ = i >> in // ERROR "too small for shift" - i <<= in // ERROR "too small for shift" - i >>= in // ERROR "too small for shift" - const ix = 8*unsafe.Sizeof(i) - 1 - _ = i << ix - _ = i >> ix - i <<= ix - i >>= ix - - var u uint - _ = u << 31 - const un = 8 * unsafe.Sizeof(u) - _ = u << un // ERROR "too small for shift" - _ = u >> un // ERROR "too small for shift" - u <<= un // ERROR "too small for shift" - u >>= un // ERROR "too small for shift" - const ux = 8*unsafe.Sizeof(u) - 1 - _ = u << ux - _ = u >> ux - u <<= ux - u >>= ux - - var p uintptr - _ = p << 31 - const pn = 8 * unsafe.Sizeof(p) - _ = p << pn // ERROR "too small for shift" - _ = p >> pn // ERROR "too small for shift" - p <<= pn // ERROR "too small for shift" - p >>= pn // ERROR "too small for shift" - const px = 8*unsafe.Sizeof(p) - 1 - _ = p << px - _ = p >> px - p <<= px - p >>= px - - const oneIf64Bit = ^uint(0) >> 63 // allow large shifts of constants; they are used for 32/64 bit compatibility tricks - - var h uintptr - h = h<<8 | (h >> (8 * (unsafe.Sizeof(h) - 1))) - h <<= 8 * unsafe.Sizeof(h) // ERROR "too small for shift" - h >>= 7 * unsafe.Alignof(h) - h >>= 8 * unsafe.Alignof(h) // ERROR "too small for shift" -} - -func ShiftDeadCode() { - var i int - const iBits = 8 * unsafe.Sizeof(i) - - if iBits <= 32 { - if iBits == 16 { - _ = i >> 8 - } else { - _ = i >> 16 - } - } else { - _ = i >> 32 - } - - if iBits >= 64 { - _ = i << 32 - if iBits == 128 { - _ = i << 64 - } - } else { - _ = i << 16 - } - - if iBits == 64 { - _ = i << 32 - } - - switch iBits { - case 128, 64: - _ = i << 32 - default: - _ = i << 16 - } - - switch { - case iBits < 32: - _ = i << 16 - case iBits > 64: - _ = i << 64 - default: - _ = i << 64 // ERROR "too small for shift" - } - - // Make sure other vet checks work in dead code. - if iBits == 1024 { - _ = i << 512 // OK - fmt.Printf("foo %s bar", 123) // ERROR "Printf" - } -} |