From 96139f25993f3d8122e27a6fec877a4d4f69f83b Mon Sep 17 00:00:00 2001 From: Pat Gavlin Date: Fri, 26 Mar 2021 14:46:37 +0000 Subject: [release-branch.go1.16] cmd/compile: fix long RMW bit operations on AMD64 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under certain circumstances, the existing rules for bit operations can produce code that writes beyond its intended bounds. For example, consider the following code: func repro(b []byte, addr, bit int32) { _ = b[3] v := uint32(b[0]) | uint32(b[1])<<8 | uint32(b[2])<<16 | uint32(b[3])<<24 | 1<<(bit&31) b[0] = byte(v) b[1] = byte(v >> 8) b[2] = byte(v >> 16) b[3] = byte(v >> 24) } Roughly speaking: 1. The expression `1 << (bit & 31)` is rewritten into `(SHLL 1 bit)` 2. The expression `uint32(b[0]) | uint32(b[1])<<8 | uint32(b[2])<<16 | uint32(b[3])<<24` is rewritten into `(MOVLload &b[0])` 3. The statements `b[0] = byte(v) ... b[3] = byte(v >> 24)` are rewritten into `(MOVLstore &b[0], v)` 4. `(ORL (SHLL 1, bit) (MOVLload &b[0]))` is rewritten into `(BTSL (MOVLload &b[0]) bit)`. This is a valid transformation because the destination is a register: in this case, the bit offset is masked by the number of bits in the destination register. This is identical to the masking performed by `SHL`. 5. `(MOVLstore &b[0] (BTSL (MOVLload &b[0]) bit))` is rewritten into `(BTSLmodify &b[0] bit)`. This is an invalid transformation because the destination is memory: in this case, the bit offset is not masked, and the chosen instruction may write outside its intended 32-bit location. These changes fix the invalid rewrite performed in step (5) by explicitly maksing the bit offset operand to `BT(S|R|C)(L|Q)modify`. In the example above, the adjusted rules produce `(BTSLmodify &b[0] (ANDLconst [31] bit))` in step (5). These changes also add several new rules to rewrite bit sets, toggles, and clears that are rooted at `(OR|XOR|AND)(L|Q)modify` operators into appropriate `BT(S|R|C)(L|Q)modify` operators. These rules catch cases where `MOV(L|Q)store ((OR|XOR|AND)(L|Q) ...)` is rewritten to `(OR|XOR|AND)(L|Q)modify` before the `(OR|XOR|AND)(L|Q) ...` can be rewritten to `BT(S|R|C)(L|Q) ...`. Overall, compilecmp reports small improvements in code size on darwin/amd64 when the changes to the compiler itself are exlcuded: file before after Δ % runtime.s 536464 536412 -52 -0.010% bytes.s 32629 32593 -36 -0.110% strings.s 44565 44529 -36 -0.081% os/signal.s 7967 7959 -8 -0.100% cmd/vendor/golang.org/x/sys/unix.s 81686 81678 -8 -0.010% math/big.s 188235 188253 +18 +0.010% cmd/link/internal/loader.s 89295 89056 -239 -0.268% cmd/link/internal/ld.s 633551 633232 -319 -0.050% cmd/link/internal/arm.s 18934 18928 -6 -0.032% cmd/link/internal/arm64.s 31814 31801 -13 -0.041% cmd/link/internal/riscv64.s 7347 7345 -2 -0.027% cmd/compile/internal/ssa.s 4029173 4033066 +3893 +0.097% total 21298280 21301472 +3192 +0.015% Fixes #45253 Change-Id: I2e560548b515865129e1724e150e30540e9d29ce GitHub-Last-Rev: ab94ede1d097f920a9d1d3da403c8e4a3d8f6d44 GitHub-Pull-Request: golang/go#45242 Reviewed-on: https://go-review.googlesource.com/c/go/+/305069 Trust: Emmanuel Odeke Run-TryBot: Emmanuel Odeke TryBot-Result: Go Bot Reviewed-by: Keith Randall --- test/codegen/bits.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'test') diff --git a/test/codegen/bits.go b/test/codegen/bits.go index 4508eba487..6f7502292a 100644 --- a/test/codegen/bits.go +++ b/test/codegen/bits.go @@ -262,8 +262,8 @@ func bitcompl32(a, b uint32) (n uint32) { return n } -// check direct operation on memory with constant source -func bitOpOnMem(a []uint32) { +// check direct operation on memory with constant and shifted constant sources +func bitOpOnMem(a []uint32, b, c, d uint32) { // amd64:`ANDL\s[$]200,\s\([A-Z]+\)` a[0] &= 200 // amd64:`ORL\s[$]220,\s4\([A-Z]+\)` @@ -276,6 +276,12 @@ func bitOpOnMem(a []uint32) { a[4] |= 0x4000 // amd64:`BTCL\s[$]13,\s20\([A-Z]+\)`,-`XORL` a[5] ^= 0x2000 + // amd64:`BTRL\s[A-Z]+,\s24\([A-Z]+\)` + a[6] &^= 1 << (b & 31) + // amd64:`BTSL\s[A-Z]+,\s28\([A-Z]+\)` + a[7] |= 1 << (c & 31) + // amd64:`BTCL\s[A-Z]+,\s32\([A-Z]+\)` + a[8] ^= 1 << (d & 31) } func bitcheckMostNegative(b uint8) bool { -- cgit v1.2.3-54-g00ecf