From 585c9e8412540b10be5154d019828a1a27f7b4c4 Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Sun, 20 Jan 2019 10:52:11 -0800 Subject: [PATCH] cmd/compile: implement shifts by signed amounts Allow shifts by signed amounts. Panic if the shift amount is negative. TODO: We end up doing two compares per shift, see Ian's comment https://github.com/golang/go/issues/19113#issuecomment-443241799 that we could do it with a single comparison in the normal case. The prove pass mostly handles this code well. For instance, it removes the <0 check for cases like this: if s >= 0 { _ = x << s } _ = x << len(a) This case isn't handled well yet: _ = x << (y & 0xf) I'll do followon CLs for unhandled cases as needed. Update #19113 R=go1.13 Change-Id: I839a5933d94b54ab04deb9dd5149f32c51c90fa1 Reviewed-on: https://go-review.googlesource.com/c/158719 Run-TryBot: Keith Randall TryBot-Result: Gobot Gobot Reviewed-by: Josh Bleecher Snyder --- src/cmd/compile/internal/gc/builtin.go | 1 + .../compile/internal/gc/builtin/runtime.go | 1 + src/cmd/compile/internal/gc/go.go | 1 + src/cmd/compile/internal/gc/ssa.go | 9 +- src/cmd/compile/internal/gc/typecheck.go | 4 +- src/cmd/compile/internal/ssa/rewrite.go | 3 +- src/cmd/internal/obj/x86/obj6.go | 2 +- src/runtime/panic.go | 20 +++- test/fixedbugs/bug073.go | 10 +- test/fixedbugs/issue19113.go | 108 ++++++++++++++++++ 10 files changed, 144 insertions(+), 15 deletions(-) create mode 100644 test/fixedbugs/issue19113.go diff --git a/src/cmd/compile/internal/gc/builtin.go b/src/cmd/compile/internal/gc/builtin.go index 04f4cbfd58..f32fcd675d 100644 --- a/src/cmd/compile/internal/gc/builtin.go +++ b/src/cmd/compile/internal/gc/builtin.go @@ -13,6 +13,7 @@ var runtimeDecls = [...]struct { {"panicindex", funcTag, 5}, {"panicslice", funcTag, 5}, {"panicdivide", funcTag, 5}, + {"panicshift", funcTag, 5}, {"panicmakeslicelen", funcTag, 5}, {"throwinit", funcTag, 5}, {"panicwrap", funcTag, 5}, diff --git a/src/cmd/compile/internal/gc/builtin/runtime.go b/src/cmd/compile/internal/gc/builtin/runtime.go index fc879badb2..210881a6e9 100644 --- a/src/cmd/compile/internal/gc/builtin/runtime.go +++ b/src/cmd/compile/internal/gc/builtin/runtime.go @@ -18,6 +18,7 @@ func newobject(typ *byte) *any func panicindex() func panicslice() func panicdivide() +func panicshift() func panicmakeslicelen() func throwinit() func panicwrap() diff --git a/src/cmd/compile/internal/gc/go.go b/src/cmd/compile/internal/gc/go.go index 376637ba9a..2213d8d9b8 100644 --- a/src/cmd/compile/internal/gc/go.go +++ b/src/cmd/compile/internal/gc/go.go @@ -296,6 +296,7 @@ var ( msanwrite, newproc, panicdivide, + panicshift, panicdottypeE, panicdottypeI, panicindex, diff --git a/src/cmd/compile/internal/gc/ssa.go b/src/cmd/compile/internal/gc/ssa.go index e20137669a..6ddc9fba7a 100644 --- a/src/cmd/compile/internal/gc/ssa.go +++ b/src/cmd/compile/internal/gc/ssa.go @@ -84,6 +84,7 @@ func initssaconfig() { panicnildottype = sysfunc("panicnildottype") panicoverflow = sysfunc("panicoverflow") panicslice = sysfunc("panicslice") + panicshift = sysfunc("panicshift") raceread = sysfunc("raceread") racereadrange = sysfunc("racereadrange") racewrite = sysfunc("racewrite") @@ -2128,7 +2129,13 @@ func (s *state) expr(n *Node) *ssa.Value { case OLSH, ORSH: a := s.expr(n.Left) b := s.expr(n.Right) - return s.newValue2(s.ssaShiftOp(n.Op, n.Type, n.Right.Type), a.Type, a, b) + bt := b.Type + if bt.IsSigned() { + cmp := s.newValue2(s.ssaOp(OGE, bt), types.Types[TBOOL], b, s.zeroVal(bt)) + s.check(cmp, panicshift) + bt = bt.ToUnsigned() + } + return s.newValue2(s.ssaShiftOp(n.Op, n.Type, bt), a.Type, a, b) case OANDAND, OOROR: // To implement OANDAND (and OOROR), we introduce a // new temporary variable to hold the result. The diff --git a/src/cmd/compile/internal/gc/typecheck.go b/src/cmd/compile/internal/gc/typecheck.go index 4fc1c5c73c..63e0d78273 100644 --- a/src/cmd/compile/internal/gc/typecheck.go +++ b/src/cmd/compile/internal/gc/typecheck.go @@ -660,8 +660,8 @@ func typecheck1(n *Node, top int) (res *Node) { r = defaultlit(r, types.Types[TUINT]) n.Right = r t := r.Type - if !t.IsInteger() || t.IsSigned() { - yyerror("invalid operation: %v (shift count type %v, must be unsigned integer)", n, r.Type) + if !t.IsInteger() { + yyerror("invalid operation: %v (shift count type %v, must be integer)", n, r.Type) n.Type = nil return n } diff --git a/src/cmd/compile/internal/ssa/rewrite.go b/src/cmd/compile/internal/ssa/rewrite.go index a154249371..6edb593df9 100644 --- a/src/cmd/compile/internal/ssa/rewrite.go +++ b/src/cmd/compile/internal/ssa/rewrite.go @@ -1115,7 +1115,8 @@ func needRaceCleanup(sym interface{}, v *Value) bool { case OpStaticCall: switch v.Aux.(fmt.Stringer).String() { case "runtime.racefuncenter", "runtime.racefuncexit", "runtime.panicindex", - "runtime.panicslice", "runtime.panicdivide", "runtime.panicwrap": + "runtime.panicslice", "runtime.panicdivide", "runtime.panicwrap", + "runtime.panicshift": // Check for racefuncenter will encounter racefuncexit and vice versa. // Allow calls to panic* default: diff --git a/src/cmd/internal/obj/x86/obj6.go b/src/cmd/internal/obj/x86/obj6.go index babfd38ad2..a6931e8441 100644 --- a/src/cmd/internal/obj/x86/obj6.go +++ b/src/cmd/internal/obj/x86/obj6.go @@ -968,7 +968,7 @@ func isZeroArgRuntimeCall(s *obj.LSym) bool { return false } switch s.Name { - case "runtime.panicindex", "runtime.panicslice", "runtime.panicdivide", "runtime.panicwrap": + case "runtime.panicindex", "runtime.panicslice", "runtime.panicdivide", "runtime.panicwrap", "runtime.panicshift": return true } return false diff --git a/src/runtime/panic.go b/src/runtime/panic.go index bb83be4715..59916dd5e5 100644 --- a/src/runtime/panic.go +++ b/src/runtime/panic.go @@ -23,16 +23,16 @@ func panicCheckMalloc(err error) { var indexError = error(errorString("index out of range")) -// The panicindex, panicslice, and panicdivide functions are called by +// The panic{index,slice,divide,shift} functions are called by // code generated by the compiler for out of bounds index expressions, -// out of bounds slice expressions, and division by zero. The -// panicdivide (again), panicoverflow, panicfloat, and panicmem +// out of bounds slice expressions, division by zero, and shift by negative. +// The panicdivide (again), panicoverflow, panicfloat, and panicmem // functions are called by the signal handler when a signal occurs // indicating the respective problem. // -// Since panicindex and panicslice are never called directly, and +// Since panic{index,slice,shift} are never called directly, and // since the runtime package should never have an out of bounds slice -// or array reference, if we see those functions called from the +// or array reference or negative shift, if we see those functions called from the // runtime package we turn the panic into a throw. That will dump the // entire runtime stack for easier debugging. @@ -68,6 +68,16 @@ func panicoverflow() { panic(overflowError) } +var shiftError = error(errorString("negative shift amount")) + +func panicshift() { + if hasPrefix(funcname(findfunc(getcallerpc())), "runtime.") { + throw(string(shiftError.(errorString))) + } + panicCheckMalloc(shiftError) + panic(shiftError) +} + var floatError = error(errorString("floating point error")) func panicfloat() { diff --git a/test/fixedbugs/bug073.go b/test/fixedbugs/bug073.go index 49b47ae464..f3605b37cf 100644 --- a/test/fixedbugs/bug073.go +++ b/test/fixedbugs/bug073.go @@ -1,4 +1,4 @@ -// errorcheck +// compile // Copyright 2009 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style @@ -7,8 +7,8 @@ package main func main() { - var s int = 0; - var x int = 0; - x = x << s; // ERROR "illegal|inval|shift" - x = x >> s; // ERROR "illegal|inval|shift" + var s int = 0 + var x int = 0 + x = x << s // as of 1.13, these are ok + x = x >> s // as of 1.13, these are ok } diff --git a/test/fixedbugs/issue19113.go b/test/fixedbugs/issue19113.go new file mode 100644 index 0000000000..5e01dde699 --- /dev/null +++ b/test/fixedbugs/issue19113.go @@ -0,0 +1,108 @@ +// run + +// 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. + +package main + +import "reflect" + +var tests = []interface{}{ + func(x int, s int) int { + return x << s + }, + func(x int, s int64) int { + return x << s + }, + func(x int, s int32) int { + return x << s + }, + func(x int, s int16) int { + return x << s + }, + func(x int, s int8) int { + return x << s + }, + func(x int, s int) int { + return x >> s + }, + func(x int, s int64) int { + return x >> s + }, + func(x int, s int32) int { + return x >> s + }, + func(x int, s int16) int { + return x >> s + }, + func(x int, s int8) int { + return x >> s + }, + func(x uint, s int) uint { + return x << s + }, + func(x uint, s int64) uint { + return x << s + }, + func(x uint, s int32) uint { + return x << s + }, + func(x uint, s int16) uint { + return x << s + }, + func(x uint, s int8) uint { + return x << s + }, + func(x uint, s int) uint { + return x >> s + }, + func(x uint, s int64) uint { + return x >> s + }, + func(x uint, s int32) uint { + return x >> s + }, + func(x uint, s int16) uint { + return x >> s + }, + func(x uint, s int8) uint { + return x >> s + }, +} + +func main() { + for _, t := range tests { + runTest(reflect.ValueOf(t)) + } +} + +func runTest(f reflect.Value) { + xt := f.Type().In(0) + st := f.Type().In(1) + + for _, x := range []int{1, 0, -1} { + for _, s := range []int{-99, -64, -63, -32, -31, -16, -15, -8, -7, -1, 0, 1, 7, 8, 15, 16, 31, 32, 63, 64, 99} { + args := []reflect.Value{ + reflect.ValueOf(x).Convert(xt), + reflect.ValueOf(s).Convert(st), + } + if s < 0 { + shouldPanic(func() { + f.Call(args) + }) + } else { + f.Call(args) // should not panic + } + } + } +} + +func shouldPanic(f func()) { + defer func() { + if recover() == nil { + panic("did not panic") + } + }() + f() +}