Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: miscompile of shift on GOOS=arm #64715

Closed
thanm opened this issue Dec 14, 2023 · 10 comments
Closed

cmd/compile: miscompile of shift on GOOS=arm #64715

thanm opened this issue Dec 14, 2023 · 10 comments
Labels
arch-arm Issues solely affecting the 32-bit arm architecture. compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@thanm
Copy link
Contributor

thanm commented Dec 14, 2023

Go version

go version devel go1.22-f2d243db8f Wed Dec 13 16:20:09 2023 +0000 linux/amd64

What operating system and processor architecture are you using (go env)?

linux/arm

What did you do?

NB: filing this issue on behalf of Jan Mercl, who discovered and reported the problem here

Run this program on a GOOS=linux GOARCH=arm machine:

package main

func boolInt32(b bool) int32 {
	if b {
		return 1
	}

	return 0
}

func f(left uint16, right int32) (r uint16) {
	return left >> right
}

var n = uint16(65535)

func main() {
	println(f(n, boolInt32(int64(n^n) > 1)))
}

What did you expect to see?

Expected:

$ go run prog.go
65535
$

What did you see instead?

Got instead:

$ go run prog.go
0
$

Also worth noting that building with -gcflags="-l -N" makes the problem go away.

@thanm thanm added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Dec 14, 2023
@thanm thanm added this to the Go1.22 milestone Dec 14, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 14, 2023
@bcmills bcmills added the arch-arm Issues solely affecting the 32-bit arm architecture. label Dec 14, 2023
@dr2chase
Copy link
Contributor

Also present in go-1.18,19,20.

@randall77
Copy link
Contributor

Weird. It seems to get through the SSA backend ok, but then there is a mis-assembly.
SSA ends with the useless, but correct:

00009 (12) SRL $0, R0, R0

But if I disassemble the binary, there is

  issue64715.go:12      0x78108                 e1a00020                LSR $32, R0, R0                 

Indeed, if I add some unused code to runtime/asm_arm.s like this:

	SRL	$0, R0, R0
	SRL	$1, R0, R0

It disassembles to this:

  asm_arm.s:239         0x757f8                 e1a00020                LSR $32, R0, R0         
  asm_arm.s:240         0x757fc                 e1a000a0                LSR $1, R0, R0          

@randall77
Copy link
Contributor

Good lord, that's horrible: https://stackoverflow.com/questions/66487890/why-different-ranges-of-permitted-shift-values-for-arm-lsl-vs-lsr

@cuonglm
Copy link
Member

cuonglm commented Dec 14, 2023

Good lord, that's horrible: https://stackoverflow.com/questions/66487890/why-different-ranges-of-permitted-shift-values-for-arm-lsl-vs-lsr

So we probably need to add a rule to rewrite right shift by 0 to left shitt by 0.

@randall77
Copy link
Contributor

@cuonglm That sounds right.
I think we need to handle LSR, ASR that way.
That page also mentions ROR. I'm not sure what we should do with ROR of 0.

Weirdly, our disassembler gets it right. Note the distinction between arg_imm5 and arg_imm5_32.

	case arg_imm5:
		return Imm((x >> 7) & (1<<5 - 1))

	case arg_imm5_32:
		x = (x >> 7) & (1<<5 - 1)
		if x == 0 {
			x = 32
		}
		return Imm(x)

arg_imm5_32 is used for ASR and LSR.

@ryan-berger
Copy link

So we probably need to add a rule to rewrite right shift by 0 to left shift by 0.

Shouldn't an ASHR/SRL of 0 just optimize away rather than changing one into another?

But taking a look at the ARM manual, it looks like they've thought ahead about this edge case:

Assemblers can permit the use of some or all of ASR #0, LSL #0, LSR #0, and ROR #0 to specify that no shift is to be performed. This is not standard UAL, and the encoding selected for Thumb instructions might vary between UAL assemblers if it is used. To ensure disassembled code assembles to the original instructions, disassemblers must omit the shift specifier when the instruction specifies no shift.

It looks like this might be something that should get handled at the assembler level to keep things consistent. This case also only applies to constant shifts, see the register version of the instruction vs the immediate version, so no extra work needs to be done on a register based shift of zero

@randall77
Copy link
Contributor

Shouldn't an ASHR/SRL of 0 just optimize away rather than changing one into another?

Certainly fixing the compiler to not issue 0 shifts is worth doing. But since we need to fix the assembler for this issue anyway, that compiler change is not needed to fix this bug. It would just be a performance improvement that could be done independently. (And this bug fix would be nice to get in for 1.22. Performance improvements would wait for 1.23.)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/549955 mentions this issue: cmd/asm: fix encoding for arm right shift by constant 0

@randall77
Copy link
Contributor

This just got more complicated because there are not only shift instructions, but shifted register arguments (in fact, the former is implemented using MOV + the latter). We already give errors in some cases for register arguments shifted by bad amounts, like R0>>0.
Maybe we should just error with the 0 right shifts, and fix the compiler to never emit them.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/550335 mentions this issue: cmd/asm: for arm, rewrite argument shifted right by 0 to left by 0.

gopherbot pushed a commit that referenced this issue Dec 15, 2023
Right shift by 0 has bad semantics. Make sure if we try to right shift by 0,
do a left shift by 0 instead.

CL 549955 handled full instructions with this strange no-op encoding.
This CL handles the shift done to instruction register inputs.
(The former is implemented using the latter, but not until deep
inside the assembler.)

Update #64715

Change-Id: Ibfabb4b13e2595551e58b977162fe005aaaa0ad1
Reviewed-on: https://go-review.googlesource.com/c/go/+/550335
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Keith Randall <khr@google.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Right shifts, for some odd reasons, can encode shifts of constant
1-32 instead of 0-31. Left shifts, however, can encode shifts 0-31.
When the shift amount is 0, arm recommends encoding right shifts
using left shifts.

Fixes golang#64715

Change-Id: Id3825349aa7195028037893dfe01fa0e405eaa51
Reviewed-on: https://go-review.googlesource.com/c/go/+/549955
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Keith Randall <khr@google.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Right shift by 0 has bad semantics. Make sure if we try to right shift by 0,
do a left shift by 0 instead.

CL 549955 handled full instructions with this strange no-op encoding.
This CL handles the shift done to instruction register inputs.
(The former is implemented using the latter, but not until deep
inside the assembler.)

Update golang#64715

Change-Id: Ibfabb4b13e2595551e58b977162fe005aaaa0ad1
Reviewed-on: https://go-review.googlesource.com/c/go/+/550335
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Keith Randall <khr@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-arm Issues solely affecting the 32-bit arm architecture. compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
Status: Done
Development

No branches or pull requests

7 participants