Skip to content

Commit

Permalink
go/ir: don't panic trying to replace phi in unreachable exit node
Browse files Browse the repository at this point in the history
When the exit node is unreachable, for example because of a function
that ends with an infinite loop, and we lift an alloc for a named return
value, we may end up creating a phi node with no edges. Don't do that.

Closes: gh-1533
  • Loading branch information
dominikh committed May 21, 2024
1 parent 6245513 commit a9ef9c7
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 0 deletions.
36 changes: 36 additions & 0 deletions go/ir/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -633,3 +633,39 @@ func TestLabels(t *testing.T) {
pkg.Build()
}
}

func TestUnreachableExit(t *testing.T) {
tests := []string{
`
package pkg
func foo() (err error) {
if true {
println()
}
println()
for {
err = nil
}
}`,
}
for _, test := range tests {
conf := loader.Config{Fset: token.NewFileSet()}
f, err := parser.ParseFile(conf.Fset, "<input>", test, 0)
if err != nil {
t.Errorf("parse error: %s", err)
return
}
conf.CreateFromFiles("pkg", f)
iprog, err := conf.Load()
if err != nil {
t.Error(err)
continue
}
prog := irutil.CreateProgram(iprog, ir.BuilderMode(0))
pkg := prog.Package(iprog.Created[0].Pkg)
pkg.Build()
}
}
16 changes: 16 additions & 0 deletions go/ir/lift.go
Original file line number Diff line number Diff line change
Expand Up @@ -1243,6 +1243,22 @@ func liftAlloc(closure *closure, df domFrontier, rdf postDomFrontier, alloc *All

// Create φ-node.
// It will be prepended to v.Instrs later, if needed.
if len(y.Preds) == 0 {
// The exit block may be unreachable if the function doesn't
// return, e.g. due to an infinite loop. In that case we
// should not replace loads in the exit block with ϕ node that
// have no edges. Such loads exist when the function has named
// return parameters, as the exit block loads them to turn
// them into a Return instruction. By not replacing the loads
// with ϕ nodes, they will later be replaced by zero
// constants. This is arguably more correct, and more
// importantly, it doesn't break code that assumes that phis
// have at least one edge.
//
// For one instance of breakage see
// https://staticcheck.dev/issues/1533
continue
}
phi := &Phi{
Edges: make([]Value, len(y.Preds)),
}
Expand Down

0 comments on commit a9ef9c7

Please sign in to comment.