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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: incorrect use of CMN on arm64 #50854

Closed
rsc opened this issue Jan 27, 2022 · 7 comments
Closed

cmd/compile: incorrect use of CMN on arm64 #50854

rsc opened this issue Jan 27, 2022 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker
Milestone
Go1.18

Comments

@rsc
Copy link
Contributor

rsc commented Jan 27, 2022 

% go version
go version devel go1.18-78d3202a14 Sun Jan 16 12:20:41 2022 -0500 darwin/arm64
% cat x.go
package main

//go:noinline
func f(p int64, x, y int64, b bool) bool { return -x <= p && b }

func main() { println(f(1, -1<<63, 1<<63-1, true)) }

% go run x.go
true
% go tool compile -S x.go
"".f STEXT size=32 args=0x20 locals=0x0 funcid=0x0 align=0x0 leaf
	0x0000 00000 (x.go:4)	TEXT	"".f(SB), LEAF|NOFRAME|ABIInternal, $0-32
	0x0000 00000 (x.go:4)	FUNCDATA	ZR, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	0x0000 00000 (x.go:4)	FUNCDATA	$1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	0x0000 00000 (x.go:4)	FUNCDATA	$5, "".f.arginfo1(SB)
	0x0000 00000 (x.go:4)	FUNCDATA	$6, "".f.argliveinfo(SB)
	0x0000 00000 (x.go:4)	PCDATA	$3, $1
	0x0000 00000 (x.go:4)	NEG	R1, R1
	0x0004 00004 (x.go:4)	CMP	R1, R0
	0x0008 00008 (x.go:4)	CSET	GE, R1
	0x000c 00012 (x.go:4)	AND	R3, R1, R0
	0x0010 00016 (x.go:4)	RET	(R30)
	0x0000 e1 03 01 cb 1f 00 01 eb e1 b7 9f 9a 20 00 03 8a  ............ ...
	0x0010 c0 03 5f d6 00 00 00 00 00 00 00 00 00 00 00 00  .._.............

So far so good. Note that -x <= p is implemented as NEG, CMP.

Now let's change the body of f: s/b/p <= y/.

% cat x.go
package main

//go:noinline
func f(p int64, x, y int64, b bool) bool { return -x <= p && p <= y }

func main() { println(f(1, -1<<63, 1<<63-1, true)) }
% go run x.go
false
% go tool compile -S x.go
"".f STEXT size=32 args=0x20 locals=0x0 funcid=0x0 align=0x0 leaf
	0x0000 00000 (x.go:4)	TEXT	"".f(SB), LEAF|NOFRAME|ABIInternal, $0-32
	0x0000 00000 (x.go:4)	FUNCDATA	ZR, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	0x0000 00000 (x.go:4)	FUNCDATA	$1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	0x0000 00000 (x.go:4)	FUNCDATA	$5, "".f.arginfo1(SB)
	0x0000 00000 (x.go:4)	FUNCDATA	$6, "".f.argliveinfo(SB)
	0x0000 00000 (x.go:4)	PCDATA	$3, $1
	0x0000 00000 (x.go:4)	CMN	R1, R0
	0x0004 00004 (x.go:4)	BLT	20
	0x0008 00008 (x.go:4)	CMP	R0, R2
	0x000c 00012 (x.go:4)	CSET	GE, R1
	0x0010 00016 (x.go:4)	JMP	24
	0x0014 00020 (x.go:4)	MOVD	ZR, R1
	0x0018 00024 (x.go:4)	MOVD	R1, R0
	0x001c 00028 (x.go:4)	RET	(R30)
	0x0000 1f 00 01 ab 8b 00 00 54 5f 00 00 eb e1 b7 9f 9a  .......T_.......
	0x0010 02 00 00 14 01 00 80 d2 e0 03 01 aa c0 03 5f d6  .............._.

Note that the NEG, CMP has changed to CMN. The CMN is wrong in the case where the comparison argument is -1<<63, since its negation is itself.

This optimization should not be applied when compiling -x <= p.
@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker labels Jan 27, 2022
@rsc
Copy link
Contributor Author

rsc commented Jan 27, 2022

Given that this issue arose during code related to protocol buffers, we should probably also backport the fix (assuming it is trivial to just turn off this optimization).

@aclements aclements added this to the Go1.18 milestone Jan 27, 2022
@dr2chase
Copy link
Contributor

I see a whole bunch of CMN optimizations that give me the willies.

@gopherbot
Copy link

Change https://golang.org/cl/381318 mentions this issue: cmd/compile: remove incorrect arm,arm64 CMP->CMN transformations

@dr2chase
Copy link
Contributor

@gopherbot, please backport to Go 1.16 and Go 1.17. This is an arm and arm64 compiler bug that generates incorrect code with the usual possibility of data corruption. The "fix" involves disabling an (incorrect) optimization in the "rules" (pattern-transmformations), so it is believed low risk.

@gopherbot gopherbot mentioned this issue Jan 27, 2022
Closed
@gopherbot
Copy link

Backport issue(s) opened: #50866 (for 1.16), #50867 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot gopherbot mentioned this issue Jan 27, 2022
Closed
@gopherbot
Copy link

Change https://golang.org/cl/381474 mentions this issue: [release-branch.go1.17] cmd/compile: remove incorrect arm,arm64 CMP->CMN transformations

@gopherbot
Copy link

Change https://golang.org/cl/381475 mentions this issue: [release-branch.go1.16] cmd/compile: remove incorrect arm,arm64 CMP->CMN transformations

@golang golang locked and limited conversation to collaborators Jan 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
4 participants
@rsc @dr2chase @aclements @gopherbot