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: struct interface{} value lost passing by value #30956

Closed
benjaminp opened this issue Mar 20, 2019 · 14 comments
Closed

cmd/compile: struct interface{} value lost passing by value #30956

benjaminp opened this issue Mar 20, 2019 · 14 comments
Milestone

Comments

@benjaminp
Copy link
Contributor

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

$ go version
1.12.1

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

Linux amd64

What did you do?

Consider this program (https://play.golang.org/p/lblyBSvvbis):

package main

import "fmt"

type X struct {
	V interface{}

	a uint64
	b uint64
	c uint64
}

func pr(x X) {
	fmt.Println(x.V)
}

func main() {
	pr(X{
		V: struct {
			A int
		}{42},
	})
}

What did you expect to see?

{42} printed.

What did you see instead?

With Go 1.12.1, this program prints <nil>. With Go 1.11.6, it prints {42} as expected.

@elagergren-spideroak
Copy link

elagergren-spideroak commented Mar 20, 2019

Note that it only occurs when not assigned to a variable.

y := struct{ A int }{42}
pr(X{V: y}) // prints {42}

It also seems to occur only when certain fields are added to X as well.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz added this to the Go1.13 milestone Mar 20, 2019
@bradfitz bradfitz added CherryPickCandidate Used during the release process for point releases NeedsFix The path to resolution is known, but the work has not been done. labels Mar 20, 2019
@cherrymui
Copy link
Member

It seems that now we generate static data for X. However, the symbol ..stmp_0 doesn't seem to contain any data.

$ go tool compile -S=2 x.go
...
""..inittask SNOPTRDATA size=32
        0x0000 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00  ................
        0x0010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
        rel 24+8 t=1 fmt..inittask+0
""..stmp_0 SRODATA size=40
go.loc.type..hash."".X SDWARFLOC dupok size=103
        0x0000 ff ff ff ff ff ff ff ff 00 00 00 00 00 00 00 00  ................
...

(no content for the stmp_0 symbol)

$ go tool nm -sort=address x.o
...
     d02 R %22%22..stmp_0
     d02 ? go.loc.type..hash.%22%22.X

(the address is overlapping with the next symbol.)

@bradfitz bradfitz changed the title struct interface{} value lost passing by value cmd/compile: struct interface{} value lost passing by value Mar 21, 2019
@cuonglm
Copy link
Member

cuonglm commented Mar 21, 2019

git bisect points to 0e9f8a2 as the first bad commit

@josharian
Copy link
Contributor

Thanks for bisecting! Based on where that landed, I suspect that that commit uncovered a latent bug. Looking at sinit.go on my phone, it looks like we need to add special handling for pointer-to-zero-width-val when constructing static interface data.

@josharian
Copy link
Contributor

Looks like somewhere around line 522 in sinit.go we need to use runtime.zerobase for this case.

@cherrymui
Copy link
Member

It looks to me that walk decided to use fixedlit to generate the struct literal X, because it isStaticCompositeLiteral. However, fixedlit doesn't know how to handle CONVIFACE.

Maybe we should teach fixedlit or anylit handle CONVIFACE.

Alternatively, maybe Order.addrTemp should handle that. It already handles CONVIFACE of LITERAL, but not composite literal. And there is a TODO: // TODO: expand this to all static composite literal nodes?.

@cuonglm
Copy link
Member

cuonglm commented Mar 22, 2019

@josharian @cherrymui I did some investigation, and found the culprit is this line 0e9f8a2#diff-f429d083e18b2e96d5c8ebeae38f3c54R1052

Before that change, everything except interface will be added temporary address. So when fixedlit runs in

for _, r := range n.List.Slice() {
, it does process:

$ go-tip run main.go
# command-line-arguments
1: A:42
2: .autotmp_0 - <N>
3: 42 | LITERAL
4: 0x0 - 0x3 - .autotmp_0 = <N>
5: true false false false false
6: true
7: 1
1: V:struct { A int } literal
2: .autotmp_1 - <N>
3: struct { A int } literal | CONVIFACE
4: 0x0 - 0x3 - .autotmp_1 = <N>
5: false false false false false
6: true
7: 0
{42}

With that change, the struct does not have temp address, the process become:

$ go-tip run main.go
# command-line-arguments
1: V:struct { A int } literal
2: statictmp_0 - <N>
3: struct { A int } literal | CONVIFACE
4: 0x0 - 0x1 - 
5: false true false true false
6: false
7: 0
<nil>

Interestingly, if I added an addressable field, then problem gone, see: https://play.golang.org/p/DnCxNoFUB0P


I simply fix by this change:

diff --git a/src/cmd/compile/internal/gc/order.go b/src/cmd/compile/internal/gc/order.go
index 7b86537a21..78a8d12db3 100644
--- a/src/cmd/compile/internal/gc/order.go
+++ b/src/cmd/compile/internal/gc/order.go
@@ -1054,7 +1054,7 @@ func (o *Order) expr(n, lhs *Node) *Node {
                if n.Left.Type.IsInterface() {
                        break
                }
-               if _, needsaddr := convFuncName(n.Left.Type, n.Type); needsaddr || consttype(n.Left) > 0 {
+               if _, needsaddr := convFuncName(n.Left.Type, n.Type); needsaddr || consttype(n.Left) > 0 || n.Left.Type.IsStruct() {
                        // Need a temp if we need to pass the address to the conversion function.
                        // We also process constants here, making a named static global whose
                        // address we can put directly in an interface (see OCONVIFACE case in walk).

The problem gone, go-tip tool dist test passed. But I'm afraid it won't cover all the cases.

If both of you feel ok with that fix, I will send a CL.

Any idea?

cc @randall77

@cherrymui
Copy link
Member

@Gnouc Thanks for digging into this!

I think this is probably on the right direction. But special-casing structs seems not the best way. At least, I think we should also handle arrays. And not all structs need addrTemp.

Also, ideally, I think this code is supposed to handle static data and generate static data. With your change, it doesn't generate static data for X. This may be fine, as Go 1.11 doesn't either.

Maybe conditioning on isStaticCompositeLiteral is better. I'm not really sure at the moment.

@cherrymui
Copy link
Member

Interestingly, if I added an addressable field, then problem gone, see: https://play.golang.org/p/DnCxNoFUB0P

I think this is because with the map field, X is not static any more.

@cuonglm
Copy link
Member

cuonglm commented Mar 22, 2019

@cherrymui isStaticCompositeLiteral works 👍 I'm sending a CL

@gopherbot
Copy link

Change https://golang.org/cl/168858 mentions this issue: cmd/compile: fix literal struct interface {} lost passing by value

@randall77
Copy link
Contributor

@gopherbot, please consider this for backport to 1.12, it's a regression.

@gopherbot
Copy link

Backport issue(s) opened: #31209 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@randall77 randall77 removed CherryPickCandidate Used during the release process for point releases NeedsFix The path to resolution is known, but the work has not been done. labels Apr 2, 2019
@golang golang locked and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants