From acdddf6756bbc8a7a0d43fb7d20507438425d7d0 Mon Sep 17 00:00:00 2001 From: Tim King Date: Fri, 25 Feb 2022 12:23:22 -0800 Subject: [PATCH] go/ssa: allows right operand of a shift to be signed. Removes the forced conversion of the right operand of a shift to an unsigned type. This allows for clients to correctly model the runtime panic when a signed shift count is negative. Fixes golang/go#51363 Change-Id: If59000eeb503fd45cdc6d4143dcc249242e7a957 Reviewed-on: https://go-review.googlesource.com/c/tools/+/387995 Trust: Tim King Run-TryBot: Tim King gopls-CI: kokoro Reviewed-by: Zvonimir Pavlinovic Reviewed-by: Dominik Honnef Trust: Dominik Honnef TryBot-Result: Gopher Robot --- go/ssa/emit.go | 13 ++++++++--- go/ssa/interp/interp_test.go | 1 + go/ssa/interp/ops.go | 32 ++++++++++++++++++++++++-- go/ssa/interp/testdata/convert.go | 38 +++++++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 5 deletions(-) create mode 100644 go/ssa/interp/testdata/convert.go diff --git a/go/ssa/emit.go b/go/ssa/emit.go index 7c8cfdc66..ff53705fa 100644 --- a/go/ssa/emit.go +++ b/go/ssa/emit.go @@ -74,9 +74,16 @@ func emitArith(f *Function, op token.Token, x, y Value, t types.Type, pos token. case token.SHL, token.SHR: x = emitConv(f, x, t) // y may be signed or an 'untyped' constant. - // TODO(adonovan): whence signed values? - if b, ok := y.Type().Underlying().(*types.Basic); ok && b.Info()&types.IsUnsigned == 0 { - y = emitConv(f, y, types.Typ[types.Uint64]) + + // There is a runtime panic if y is signed and <0. Instead of inserting a check for y<0 + // and converting to an unsigned value (like the compiler) leave y as is. + + if b, ok := y.Type().Underlying().(*types.Basic); ok && b.Info()&types.IsUntyped != 0 { + // Untyped conversion: + // Spec https://go.dev/ref/spec#Operators: + // The right operand in a shift expression must have integer type or be an untyped constant + // representable by a value of type uint. + y = emitConv(f, y, types.Typ[types.Uint]) } case token.ADD, token.SUB, token.MUL, token.QUO, token.REM, token.AND, token.OR, token.XOR, token.AND_NOT: diff --git a/go/ssa/interp/interp_test.go b/go/ssa/interp/interp_test.go index 28ebf5f80..1b43742c8 100644 --- a/go/ssa/interp/interp_test.go +++ b/go/ssa/interp/interp_test.go @@ -109,6 +109,7 @@ var gorootTestTests = []string{ var testdataTests = []string{ "boundmeth.go", "complit.go", + "convert.go", "coverage.go", "defer.go", "fieldprom.go", diff --git a/go/ssa/interp/ops.go b/go/ssa/interp/ops.go index 6af7847c0..3bc6a4e32 100644 --- a/go/ssa/interp/ops.go +++ b/go/ssa/interp/ops.go @@ -137,6 +137,26 @@ func asUint64(x value) uint64 { panic(fmt.Sprintf("cannot convert %T to uint64", x)) } +// asUnsigned returns the value of x, which must be an integer type, as its equivalent unsigned type, +// and returns true if x is non-negative. +func asUnsigned(x value) (value, bool) { + switch x := x.(type) { + case int: + return uint(x), x >= 0 + case int8: + return uint8(x), x >= 0 + case int16: + return uint16(x), x >= 0 + case int32: + return uint32(x), x >= 0 + case int64: + return uint64(x), x >= 0 + case uint, uint8, uint32, uint64, uintptr: + return x, true + } + panic(fmt.Sprintf("cannot convert %T to unsigned", x)) +} + // zero returns a new "zero" value of the specified type. func zero(t types.Type) value { switch t := t.(type) { @@ -576,7 +596,11 @@ func binop(op token.Token, t types.Type, x, y value) value { } case token.SHL: - y := asUint64(y) + u, ok := asUnsigned(y) + if !ok { + panic("negative shift amount") + } + y := asUint64(u) switch x.(type) { case int: return x.(int) << y @@ -603,7 +627,11 @@ func binop(op token.Token, t types.Type, x, y value) value { } case token.SHR: - y := asUint64(y) + u, ok := asUnsigned(y) + if !ok { + panic("negative shift amount") + } + y := asUint64(u) switch x.(type) { case int: return x.(int) >> y diff --git a/go/ssa/interp/testdata/convert.go b/go/ssa/interp/testdata/convert.go new file mode 100644 index 000000000..0dcf13bdd --- /dev/null +++ b/go/ssa/interp/testdata/convert.go @@ -0,0 +1,38 @@ +// Copyright 2022 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. + +// Test conversion operations. + +package main + +func left(x int) { _ = 1 << x } +func right(x int) { _ = 1 >> x } + +func main() { + wantPanic( + func() { + left(-1) + }, + "runtime error: negative shift amount", + ) + wantPanic( + func() { + right(-1) + }, + "runtime error: negative shift amount", + ) +} + +func wantPanic(fn func(), s string) { + defer func() { + err := recover() + if err == nil { + panic("expected panic") + } + if got := err.(error).Error(); got != s { + panic("expected panic " + s + " got " + got) + } + }() + fn() +}