Navigation Menu

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

gccgo: double evaluation in interface field expression #26248

Closed
cherrymui opened this issue Jul 6, 2018 · 6 comments
Closed

gccgo: double evaluation in interface field expression #26248

cherrymui opened this issue Jul 6, 2018 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Gccgo

Comments

@cherrymui
Copy link
Member

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.
@gopherbot gopherbot added this to the Gccgo milestone Jul 6, 2018
@cherrymui
Copy link
Member Author

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

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.

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 6, 2018
@cherrymui cherrymui self-assigned this Jul 9, 2018
@gopherbot
Copy link

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

@cherrymui
Copy link
Member Author

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

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

gopherbot pushed a commit that referenced this issue Jul 10, 2018
@cherrymui
test: add test for gccgo bug #26248
5f256dc 
The fix is CL 122756.

Updates #26248.

Change-Id: Ic4250ab5d01da9f65d0bc033e2306343d9c87a99
Reviewed-on: https://go-review.googlesource.com/122757
Run-TryBot: Cherry Zhang <cherryyz@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
hubot pushed a commit to gcc-mirror/gcc that referenced this issue Jul 10, 2018
compiler: fix double evaluation with interface field expression
9189e53 
    
    In Interface_field_reference_expression, the interface expression
    is used in two places, so a temporary variable is used. Previously,
    we used a Set_and_use_temporary_expression, which, when evaluated
    twice, causes double evaluation of the underlying expression. Fix
    by setting the temporary once and use Temporary_reference_expression
    instead.
    
    Fixes golang/go#26248.
    
    Reviewed-on: https://go-review.googlesource.com/122756


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@262533 138bc75d-0d04-0410-961f-82ee72b054a4
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Jul 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
Gccgo
Development

No branches or pull requests
3 participants
@ianlancetaylor @gopherbot @cherrymui