gccgo: double evaluation in interface field expression

[cherrymui]

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

go version go1.10.3 gccgo (GCC) 9.0.0 20180622 (experimental) linux/amd64

What did you do?

package main
  
type I interface{
        M()
}

type T struct{}
func (T) M() {}

//go:noinline
func G() I {
        println("XXX")
        return T{}
}

//go:noinline
func G2(interface{}) {}

//go:noinline
func F() {
        x := G().M
        G2(x)
}

func main() {
        F()
}

What did you expect to see?

$ go run -compiler=gc i.go
XXX

What did you see instead?

$ go run -compiler=gccgo i.go
XXX
XXX

It generates two calls to G in F, one in nil check, one in constructing the func value G().M.

[cherrymui]

The generated AST is

F() : (func.8.9.8.9)
{
  tmp.41498720 (main.I) // i.go:21
  var .main.x (func.8.9.8.9) = (tmp.41498720 = G() ).M // i.go:21
  G2(interfacevalue(type_descriptor: (func.8.9.8.9), object: &(x)))  // i.go:22
}

The Set_and_use_temporary_expression (tmp.41498720 = G() ) is evaluated twice, one in nil check, one in constructing the func value. This is generated in Interface_field_reference_expression::do_get_backend.

I wonder whether Set_and_use_temporary_expression::do_get_backend can be called multiple times. If it can, we should arrange it that only the first call sets the temporary, and later calls just read it. We'd need to be sure that the first call to Set_and_use_temporary_expression::do_get_backend actually comes first in the logic order of the program being compiled.

If Set_and_use_temporary_expression::do_get_backend is supposed to be called only once, then there is some problem in handling interface field expression, or somewhere along the way. If this is the case, maybe in lower or flatten we should create a temporary for G() as a separate statement, then use its value in the interface field expression?

[ianlancetaylor]

A Set_and_use_temporary_expression is only expected to be evaluated once. If it's evaluated multiple times then a different mechanism should be used.

Set_and_use_temporary_expression precedes the current approach of Statement_inserter which can be used to insert new temporary variables, so it may be unnecessary these days.

[gopherbot]

Change https://golang.org/cl/122756 mentions this issue: compiler: fix double evaluation with interface field expression

[cherrymui]

I sent a CL to fix this. I'll look into other uses of Set_and_use_temporary_expression, see if they also have this issue and if we can avoid them. Thanks, @ianlancetaylor.

[gopherbot]

Change https://golang.org/cl/122757 mentions this issue: test: add test for gccgo bug #26248

[gopherbot]

Change https://golang.org/cl/125097 mentions this issue: compiler: clean up Set_and_use_temporary_expression