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

Comments

Projects
None yet
8 participants
@benjaminp
Copy link

benjaminp commented Mar 20, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

bradfitz commented Mar 20, 2019

@bradfitz bradfitz added this to the Go1.13 milestone Mar 20, 2019

@cherrymui

This comment has been minimized.

Copy link
Contributor

cherrymui commented Mar 20, 2019

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

This comment has been minimized.

Copy link
Contributor

cuonglm commented Mar 21, 2019

git bisect points to 0e9f8a2 as the first bad commit

@josharian

This comment has been minimized.

Copy link
Contributor

josharian commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Contributor

josharian commented Mar 21, 2019

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

@cherrymui

This comment has been minimized.

Copy link
Contributor

cherrymui commented Mar 21, 2019

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

cherrymui commented Mar 22, 2019

@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

This comment has been minimized.

Copy link
Contributor

cherrymui commented Mar 22, 2019

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

This comment has been minimized.

Copy link
Contributor

cuonglm commented Mar 22, 2019

@cherrymui isStaticCompositeLiteral works 👍 I'm sending a CL

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 22, 2019

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

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Apr 2, 2019

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Apr 2, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.