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: internal compiler error: Flag* ops should never make it to codegen #39505

Closed
ALTree opened this issue Jun 10, 2020 · 11 comments
Closed

Comments

@ALTree
Copy link
Member

@ALTree ALTree commented Jun 10, 2020

$ gotip version
go version devel +e92be18fd8 Wed Jun 10 14:56:01 2020 +0000 linux/amd64

The following program:

package p

func f() {
	if len([]int{})-1 < len([]int{}) {
	}

	var st struct {
		i int
	}
	g := func() string {
		return ""
	}
	h := func(string) string {
		return g() + g()
	}
	s, i := "", 0

	st.i = len(s)
	i = len(h(s[i+0:i+1])) + len(s[len(s)+1:i+1])
	s = s[(len(s[i+1:len(s)+1])+1):len(h(""))+1] + (s[i+1 : len([]int{})+i])
	i = 1 + len([]int{len([]string{s[i+len([]int{}) : len(s)+i]})})

	var ch chan int
	ch <- len(h("")) - len(s)
}

Crashes the tip compiler when built for arm or arm64:

$ GOARCH=arm gotip tool compile crash.go 

crash.go:4:20: internal compiler error: 'f': Flag* ops should never make it to codegen v294 = FlagLT_ULT <flags>

goroutine 1 [running]:
runtime/debug.Stack(0xd4bb80, 0xc0000b8008, 0x0)
	/home/alberto/go/src/runtime/debug/stack.go:24 +0x9f
cmd/compile/internal/gc.Fatalf(0xc000018180, 0x32, 0xc00000c0c0, 0x2, 0x2)
	/home/alberto/go/src/cmd/compile/internal/gc/subr.go:199 +0x1b0
cmd/compile/internal/gc.(*ssafn).Fatalf(0xc0003ca510, 0x414000000002, 0xc61cd5, 0x2c, 0xc000044040, 0x1, 0x1)
	/home/alberto/go/src/cmd/compile/internal/gc/ssa.go:6844 +0x1a5
cmd/compile/internal/ssa.(*Value).Fatalf(...)
	/home/alberto/go/src/cmd/compile/internal/ssa/value.go:384
cmd/compile/internal/arm.ssaGenValue(0xc0003eb400, 0xc0004080a0)
	/home/alberto/go/src/cmd/compile/internal/arm/ssa.go:865 +0x2b3d
cmd/compile/internal/gc.genssa(0xc00010cc60, 0xc0003eb380)
	/home/alberto/go/src/cmd/compile/internal/gc/ssa.go:6071 +0x702
cmd/compile/internal/gc.compileSSA(0xc00010c2c0, 0x0)
	/home/alberto/go/src/cmd/compile/internal/gc/pgen.go:327 +0x3a5
cmd/compile/internal/gc.compile(0xc00010c2c0)
	/home/alberto/go/src/cmd/compile/internal/gc/pgen.go:275 +0x345
cmd/compile/internal/gc.funccompile(0xc00010c2c0)
	/home/alberto/go/src/cmd/compile/internal/gc/pgen.go:220 +0xc5
cmd/compile/internal/gc.Main(0xc6e018)
	/home/alberto/go/src/cmd/compile/internal/gc/main.go:747 +0x344f
main.main()
	/home/alberto/go/src/cmd/compile/main.go:52 +0xb1

It compiles fine on go1.14.4, so this is a tip regression in two First Class Ports. I'm tentatively labelling this as a release-blocker, but leaving to you to decide if this is fine.

cc @randall77 @cherrymui

@marigonzes
Copy link

@marigonzes marigonzes commented Jun 10, 2020

This error seems to have been caused by e031318

@ALTree
Copy link
Member Author

@ALTree ALTree commented Jun 10, 2020

Thanks @marigonzes for bisecting.

cc @shawn-xdji and @zhangfannie too, then.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Jun 10, 2020

Thanks. It seems the new noov ops also need to handle constant flags. (I also leave a comment on the CL.)

@shawn-xdji
Copy link
Contributor

@shawn-xdji shawn-xdji commented Jun 11, 2020

@marigonzes Thanks for locating the culprit commit, I'm working on a fix.

@shawn-xdji
Copy link
Contributor

@shawn-xdji shawn-xdji commented Jun 11, 2020

Hi @ALTree I'm trying to figure out more testing scenarios, could you please shed a light what kind of code snippet would trigger the rewriting? I tried some expressions like "if const_function1() <OP> const_function2()" but no lucky. Thanks.

@ALTree
Copy link
Member Author

@ALTree ALTree commented Jun 11, 2020

@shawn-xdji Sorry, I'm afraid I can't help. In general it can be quite hard to come up with a Go program that will trigger a specific SSA rule (unless the rule is a very peculiar one). Maybe Cherry or Keith have a better insight.

@shawn-xdji
Copy link
Contributor

@shawn-xdji shawn-xdji commented Jun 14, 2020

The simplest one I could figure out is

package main
func f1() {
        const c = 1
        var a = 0
        var v *int = &a
        if *v-c < len([]int{}) {
        } else {
                panic("bad")
        }
}

By adjusting 'c', '*v' and the operator I think all concerned rules could be tested, not sure if it could be simplified further. I will install them to test/fixedbugs along with the fix.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 15, 2020

Change https://golang.org/cl/237860 mentions this issue: cmd/compile: handle constant flags for 'noov' Ops

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 15, 2020

Change https://golang.org/cl/238077 mentions this issue: cmd/compile: redo flag constant ops

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 16, 2020

Change https://golang.org/cl/237999 mentions this issue: cmd/compile: redo flag constant ops for arm64

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 18, 2020

Change https://golang.org/cl/238677 mentions this issue: cmd/compile: Install testcases for flag constant Ops

gopherbot pushed a commit that referenced this issue Jun 18, 2020
Encode the flag results in an auxint field instead of having
one opcode per flag state. This helps us handle the new *noov
branches in a unified manner.

This is only for arm, arm64 is in a subsequent CL.

We could extend to other architectures as well, athough it would
only be cleanup, no behavioral change.

Update #39505

Change-Id: Ia46cea596faad540d1496c5915ab1274571543f0
Reviewed-on: https://go-review.googlesource.com/c/go/+/238077
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@gopherbot gopherbot closed this in a07e281 Jun 18, 2020
gopherbot pushed a commit that referenced this issue Aug 28, 2020
Flag constant Ops on arm and arm64 are under refactoring, this change adds
a couple of testcases that verify the behavior of 'noov' branches.

Updates #39505
Updates #38740
Updates #39303
Change-Id: I493344b52276900cd296c32da494d72932dfc9be
Reviewed-on: https://go-review.googlesource.com/c/go/+/238677
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.