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: Export of FOR (and FORUNTIL) assumes ExprOrNil for OFOR.Right, but it is StmtOrNil #25222

Closed
dr2chase opened this issue May 2, 2018 · 6 comments
Closed

cmd/compile: Export of FOR (and FORUNTIL) assumes ExprOrNil for OFOR.Right, but it is StmtOrNil #25222

dr2chase opened this issue May 2, 2018 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.11

Comments

@dr2chase
Copy link
Contributor

dr2chase commented May 2, 2018

What version of Go are you using (go version)?

1.11dev

What did you do?

I enabled inlining for OFOR nodes, which caused the export of an OFOR node, which failed.

<autogenerated>:1: internal compiler error: cannot export = (20) node
==> please file an issue and assign to gri@

goroutine 1 [running]:
runtime/debug.Stack(0x0, 0x0, 0x0)
	/usr/local/google/home/drchase/work/go-ssa/src/runtime/debug/stack.go:24 +0xa7
cmd/compile/internal/gc.Fatalf(0xbe0f4c, 0x47, 0xc0068091f8, 0x2, 0x2)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/subr.go:182 +0x1f7
cmd/compile/internal/gc.(*exportWriter).expr(0xc0059fa900, 0xc00219e900)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:1319 +0x2a0
cmd/compile/internal/gc.(*exportWriter).exprsOrNil(0xc0059fa900, 0xc00219e880, 0xc00219e900)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:1341 +0x8d
cmd/compile/internal/gc.(*exportWriter).stmt(0xc0059fa900, 0xc00219e800)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:1053 +0x544
cmd/compile/internal/gc.(*exportWriter).stmtList(0xc0059fa900, 0xc006809318)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:974 +0xa6
cmd/compile/internal/gc.(*iexporter).doInline(0xc005f1c000, 0xc00046f1d0)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:505 +0xb4
cmd/compile/internal/gc.(*exportWriter).funcExt(0xc0059fa870, 0xc00046f1d0)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:953 +0x144
cmd/compile/internal/gc.(*iexporter).doDecl(0xc005f1c000, 0xc00046f1d0)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:434 +0x136
cmd/compile/internal/gc.iexport(0xc0055a2000)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/iexport.go:276 +0x38d
cmd/compile/internal/gc.dumpexport(0xc004606000)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/export.go:79 +0x8f
cmd/compile/internal/gc.dumpCompilerObj(0xc004606000)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/obj.go:119 +0x39
cmd/compile/internal/gc.dumpobj1(0x7fffbe08a3cf, 0x23, 0x3)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/obj.go:70 +0x15b
cmd/compile/internal/gc.dumpobj()
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/obj.go:51 +0x60
cmd/compile/internal/gc.Main(0xbe3d58)
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/internal/gc/main.go:684 +0x277c
main.main()
	/usr/local/google/home/drchase/work/go-ssa/src/cmd/compile/main.go:49 +0x96

Root cause is

	case OFOR:
...
		w.exprsOrNil(n.Left, n.Right) 
...

because n.Right is the increment of the for loop:

	OFOR      // for Ninit; Left; Right { Nbody }

i.e., n.Right is a statement, not an expression, and may be nil.
@dr2chase
Copy link
Contributor Author

dr2chase commented May 2, 2018

@gri, @mdempsky could you give this a look, if it is easy and low risk it would help with inlining for loops in 1.11, requested in #14768.

@andybons andybons changed the title Export of FOR (and FORUNTIL) assumes ExprOrNil for OFOR.Right, but it is StmtOrNil cmd/compile: Export of FOR (and FORUNTIL) assumes ExprOrNil for OFOR.Right, but it is StmtOrNil May 2, 2018
@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 2, 2018
@andybons andybons added this to the Go1.11 milestone May 2, 2018
@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 13, 2018
@griesemer
Copy link
Contributor

This shouldn't be too hard; the current code simply makes a wrong assumption for OFOR.

@dr2chase Do you have a complete reproducible example for me so I can test this?

@dr2chase
Copy link
Contributor Author

Requires a slightly tweaked compiler so it will attempt to inline OFOR, will put together that CL. I'll just reconnect all the inlining knobs, and add one for OFOR, we'll need this anyway to work on heuristics.

@gopherbot
Copy link

Change https://golang.org/cl/118996 mentions this issue: cmd/compile: add some inliner knobs for parameter search

@dr2chase
Copy link
Contributor Author

dr2chase commented Jun 14, 2018 •
edited

There you go, a new CL for inlining knobs, plus a reproducer (except this one complains about a different node, 26):

cat > main.go <<EOF
package main

func Foo(x int) int {
	a := 0
	for i := 0; i < x; i++ {
		a += a*a + i
	}
	return a
}

func main() {
	for i := 0; i < 7; i++ {
		println("Foo(", i, ")=", Foo(i))
	}
}
EOF
go build -gcflags -d=env=INLLOG=1:INLFOREXTRA=1 main.go

With this set of flags, it fails with the original error (node 20) somewhere compiling runtime

go build -gcflags all=-d=env=INLLOG=1:INLFOREXTRA=1 main.go

@gopherbot
Copy link

Change https://golang.org/cl/119595 mentions this issue: cmd/compile: fix exporting of 'for' loops

@golang golang locked and limited conversation to collaborators Jun 19, 2019
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.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
4 participants
@andybons @dr2chase @griesemer @gopherbot