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

Jump to bottom

cmd/compile: ICE due to bad ORL constant [1.15] #42753

Closed
randall77 opened this issue Nov 20, 2020 · 8 comments
Closed

cmd/compile: ICE due to bad ORL constant [1.15] #42753

randall77 opened this issue Nov 20, 2020 · 8 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.15.6

Comments

@randall77
Copy link
Contributor 

package main

func f() uint32 {
	s := "\x01"
	x := -int32(s[0])
	return uint32(x) & 0x7fffffff
}

Compile with

go tool compile -d=ssa/check/on test.go

Generates

test.go:6:21: internal compiler error: 'f': bad int32 AuxInt value for v20

Ok at tip. Breaks for 1.15 and 1.14.
@randall77 randall77 added this to the Go1.15.6 milestone Nov 20, 2020
@randall77 randall77 self-assigned this Nov 20, 2020
@randall77
Copy link
Contributor Author

@gopherbot please open backport issue for 1.14.

@gopherbot gopherbot mentioned this issue Nov 20, 2020
Closed
@gopherbot
Copy link

Backport issue(s) opened: #42755 (for 1.14).

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
Copy link

Change https://golang.org/cl/272028 mentions this issue: [release-branch.go1.15] cmd/compile: sign extend consant folding properly

@dmitshur dmitshur changed the title cmd/compile: ICE due to bad ORL constant cmd/compile: ICE due to bad ORL constant [1.15] Nov 20, 2020
@dmitshur dmitshur added the CherryPickCandidate Used during the release process for point releases label Nov 20, 2020
@gopherbot
Copy link

Change https://golang.org/cl/272029 mentions this issue: cmd/compile: add test for 42753

@dmitshur
Copy link
Contributor

dmitshur commented Nov 20, 2020 •
edited

It seems this isn't a recent regression, it happens even with Go 1.5.4:

$ go1.5.4 build p.go
# command-line-arguments
fatal error: still in list
[...]
main.main()
	/usr/local/go/src/cmd/link/main.go:26 +0x189
$ echo $?
1

Our preference in https://golang.org/wiki/MinorReleases is not to backport, unless it's serious problem with no workaround. Given it's been around for this long, maybe it's not worth taking the risk of doing a backport, unless the fix is very safe. @randall77 Do you think there are reasons that justify doing a backport rather than letting this be a part of the upcoming Go 1.16 release (which will go through additional testing)?

@randall77
Copy link
Contributor Author

The example isn't a full program, so it won't reproduce with go build. Not sure what that error you're getting is, but it isn't this one.

It looks like this bug started in 1.12 (probably the introduction of bit test instructions). 1.11 and earlier are ok.

This is similar to #41711 which had a backport approved.

As far as seriousness, I think it is an open question. It is an ICE, but a pretty rare one. We only came across it in generated code to test the compiler. It is conceivable that users will hit it, but we have no evidence for how likely that is.

It should never cause the compiler to generate incorrect code.

I don't think there's any easy workaround. You'd have to un-constant some expressions (by putting a constant in a global variable, say) to prevent the compiler optimizations involved. The error doesn't point to an obvious place to do that.

@dmitshur
Copy link
Contributor

Ah, indeed, go1.5.4 was too old and fails even on simple Go programs for unrelated reasons (not to mention its compiler doesn't support ssa/check/on). Thanks for accurately determining that this regressed in Go 1.12, and providing additional information.

Approving as it is a serious issue without a good workaround. It applies to both 1.15 (this issue) and 1.14 (#42755).

@dmitshur dmitshur added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Nov 20, 2020
gopherbot pushed a commit that referenced this issue Nov 21, 2020
@randall77
cmd/compile: add test for 42753
9ea6364 
This issue was already fixed at tip. Just adding the test that
failed on 1.14/1.15.

Update #42753

Change-Id: I00d13ade476b9c17190d762d7fdcb30cf6c83954
Reviewed-on: https://go-review.googlesource.com/c/go/+/272029
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot
Copy link

Closed by merging a2adbc8 to release-branch.go1.15.

gopherbot pushed a commit that referenced this issue Dec 3, 2020
@randall77 @cagedmantis
[release-branch.go1.15] cmd/compile: sign extend consant folding prop…
a2adbc8 
…erly

MOVLconst must have a properly sign-extended auxint constant.
The bit operations in these rules don't enforce that invariant.

The easiest fix is just to turn on properly typed auxint fields
(which is what fixed this issue at tip).

Fixes #42753

Change-Id: I264245fad45067a6ade65326f7fe681feb5f3739
Reviewed-on: https://go-review.googlesource.com/c/go/+/272028
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@golang golang locked and limited conversation to collaborators Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.15.6
Development

No branches or pull requests
3 participants
@dmitshur @randall77 @gopherbot