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: bad walkinrange rewrites on constant above 2**63 #27143

Closed
benshi001 opened this issue Aug 22, 2018 · 11 comments

Comments

Projects
None yet
6 participants
@benshi001
Copy link
Member

commented Aug 22, 2018

The below go code is rejected by go1.11.

package main
import "fmt"
func main() {
	var c uint64
	fmt.Scanf("%d", &c)
	if 0xc00921f9f01b866e < c && c < 0xc00921ff2e48e8a7 {
		fmt.Println("AAAAAAAAAAA")
	} else {
		fmt.Println("BBBBBBBBBBB")
	}
}

With error message

# command-line-arguments
./a.go:8:28: constant -4609115386278082961 overflows uint64

But this code is accepted by go1.6 and gccgo.

@benshi001

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2018

unit64's legal range should be 0000 0000 0000 0000 ~~ ffff ffff ffff ffff

Do I misunderstand something here?

@ALTree

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

The constant referenced in the error (-4609115386278082961) is given by the first hex literal minus 0xff...ff:

0xc00921f9f01b866e - (2^64 - 1)         = 
0xc00921f9f01b866e - 0xffffffffffffffff = 
  -4609115386278082961

and any literal in the second comparison triggers the error, for example:

if 0xc00921f9f01b866e < c && c < 2 {

works too (but you need a second comparison: just the first doesn't error).

Some kind of comparison optimization gone wrong? (but the issue is still there even with -N).

@ALTree ALTree added this to the Go1.12 milestone Aug 22, 2018

@go101

This comment has been minimized.

Copy link

commented Aug 22, 2018

Simplified version.

This works:

package main
func main() {
	var c uint64
	const x, y uint64 = 0xc00921f9f01b866e, 0xc00921ff2e48e8a7
	_ = c < y
	_ = c > x
	_ = c < y || c > x
	_ = c < y && c < x
	_ = c > y && c > x
	_ = c > y && c < x
	_ = c < y && c >= x
	_ = c <= y && c >= x
}

This doesn't (at least since 1.8):

package main
func main() {
	var c uint64
	const x, y uint64 = 0xc00921f9f01b866e, 0xc00921ff2e48e8a7
	_ = c < y && c > x // constant -4609115386278082961 overflows uint64
	_ = c <= y && c > x // same as above
}
@ALTree

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

First one works, the second errors:

_ = 0x7ffffffffffffff < c && c < 0x7ffffffffffffff+1
_ = 0x8000000000000000 < c && c < 0x8000000000000000+1

When C >= 2**63 (like in the second line) the front-end rewrites the boolean expression into a single LT:

.   .   LT u(2) l(6) tc(1) bool
.   .   .   CONVNOP u(2) l(6) tc(1) uint64
.   .   .   .   SUB u(2) l(6) tc(1) uint64
.   .   .   .   .   NAME-p.c u(1) a(true) g(1) l(4) x(0+0) class(PAUTO) f(1) tc(1) assigned used(true) uint64
.   .   .   .   .   LITERAL--9223372036854775807 u(1) a(true) l(6) tc(1) uint64
.   .   .   LITERAL-0 u(1) a(true) l(6) tc(1) uint64

The rewrite introduces the bad const.

I suspect this may be an issue with walkinrange (CL 27652).

cc @josharian

@gopherbot

This comment has been minimized.

Copy link

commented Aug 22, 2018

Change https://golang.org/cl/130735 mentions this issue: cmd/compile: prevent overflow in walkinrange

@ALTree

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

walkinrange assumes in several places that a.Int64( ) (where a is a CTINT Node) will succeed, but if a contains a const bigger than 2**63 the result is actually undefined (in practise, it'll return a negative number).

One nuclear option would be to just disable the range check optimization when Int64( ) cannot be used reliably (CL 130735). Another option is to review and fix all Int64 uses in walkinrange.

@ALTree ALTree changed the title cmd/compile: legal go code rejected by go1.11 (by accepted by go1.6) cmd/compile: bad walkinrange rewrites on constant above 2**63 Aug 22, 2018

@josharian

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2018

I'm on the fence about whether this fix for this should be backported. Opinions?

@ALTree

This comment has been minimized.

Copy link
Member

commented Aug 25, 2018

It's the compiler rejecting valid code... even if it's not common to write expressions with integers over 2**63. IMO this should be considered for backporting, at least on 1.11.1. The fix is very simple and it just disables an incorrect optimization, it should be safe to backport.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2018

Certainly seems to qualify for a backport.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2018

Certainly seems to qualify for a backport.

Yeah. My hesitation was only that this appears to have existed for many releases, and the symptom is a spurious compiler error (not bad code generation). Nevertheless, I agree.

@gopherbot please open backport tracking issues.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 26, 2018

Backport issue(s) opened: #27246 (for 1.11), #27247 (for 1.10).

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.