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: incorrectly permits merging of unexported embedded pointer field name #21120

Closed
ianlancetaylor opened this issue Jul 21, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@ianlancetaylor
Copy link
Contributor

commented Jul 21, 2017

Test case has three input files.

a.go:

package a

import "reflect"

type S struct {
	x int
}

func F() string {
	v := reflect.TypeOf(S{0})
	return v.PkgPath()
}

b.go:

package b

import "reflect"

type X int

func F1() string {
	type x X

	s := struct {
		*x
	}{nil}
	v := reflect.TypeOf(s)
	return v.Field(0).PkgPath
}

func F2() string {
	type y X

	s := struct {
		*y
	}{nil}
	v := reflect.TypeOf(s)
	return v.Field(0).PkgPath
}

main.go:

package main

import (
	"fmt"
	"os"

	"./a"
	"./b"
)

func main() {
	ok := true
	if s, want := a.F(), "a"; s != want {
		fmt.Printf("a.F() == %q, expected %q\n", s, want)
		ok = false
	}
	if s, want := b.F1(), ""; s != want {
		fmt.Printf("b.F1() == %q, expected %q\n", s, want)
		ok = false
	}
	if s, want := b.F2(), ""; s != want {
		fmt.Printf("b.F2() == %q, expected %q\n", s, want)
		ok = false
	}
	if !ok {
		os.Exit(1)
	}
}

The two functions in b.go are identical except for the name they use for the type defined within the function. Therefore, they should return the same value. All Go releases since Go 1 have returned the empty string for the PkgPath of a struct field of embedded pointer type, even if the embedded pointer type is not exported. That seems wrong, but it is what we have done, and it is what the code in main.go expects.

However, in this case, F1 does not return an empty string. The reason is that F1 uses a name x that is internally represented as a string with the exported flag (the least significant bit of the first byte in the reflect.name) clear. That is fine, but in a.go the existence of the field x in S produces a name x that is internally represented as a string with the exported flag set. Although those two reflect.name values are different, the compiler generates the same symbol name for both (type..namedata.x.). The linker merges the two names, causing F1 to unexpectedly see an unexported name, and to therefore return an unexpected non-empty PkgPath.

This bug is new in 1.9. I suspect that it was introduced by https://golang.org/cl/35732.

@ianlancetaylor ianlancetaylor added this to the Go1.9 milestone Jul 21, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented Jul 21, 2017

Hey @ianlancetaylor I've made a repro under my bugs repo at https://github.com/odeke-em/bugs/tree/master/golang/21120.
However, one part of your test case also fails in Go1.8.3 ie

go1.8.3

$ go version && go run main.go 
go version go1.8.3 darwin/amd64
runtime.Version() = "go1.8.3"
a.F() == "_/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/21120/a", expected "a"
exit status 1

tip at 83fb9c8

$ go version && go run main.go 
go version devel +83fb9c8 Tue Jul 18 21:31:15 2017 +0000 darwin/amd64
runtime.Version() = "devel +83fb9c8 Tue Jul 18 21:31:15 2017 +0000"
a.F() == "_/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/21120/a", expected "a"
b.F1() == "_/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/21120/b", expected ""
exit status 1

where the only difference/regression in output is b.F1() ==

where main.go is a modification of your main, to just print the Go version first

package main

import (
	"fmt"
	"os"
	"runtime"

	"./a"
	"./b"
)

func main() {
	fmt.Printf("runtime.Version() = %q\n", runtime.Version())
	ok := true
	if s, want := a.F(), "a"; s != want {
		fmt.Printf("a.F() == %q, expected %q\n", s, want)
		ok = false
	}
	if s, want := b.F1(), ""; s != want {
		fmt.Printf("b.F1() == %q, expected %q\n", s, want)
		ok = false
	}
	if s, want := b.F2(), ""; s != want {
		fmt.Printf("b.F2() == %q, expected %q\n", s, want)
		ok = false
	}
	if !ok {
		os.Exit(1)
	}
}

/cc @rsc @griesemer @mdempsky

@gopherbot

This comment has been minimized.

Copy link

commented Jul 21, 2017

CL https://golang.org/cl/50710 mentions this issue.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2017

@odeke-em Thanks, the expected "a" only expects "a" if you write the import exactly as "./a". So it's OK that your version acts differently.

In any case the CL I just sent has a cleaner version of the test, that simply verifies that the two essentially identical functions return the same result, which I believe they should.

@gopherbot gopherbot closed this in ee392ac Jul 24, 2017

gopherbot pushed a commit that referenced this issue Jul 24, 2017

[release-branch.go1.9] cmd/compile: consider exported flag in namedata
It is possible to have an unexported name with a nil package,
for an embedded field whose type is a pointer to an unexported type.
We must encode that fact in the type..namedata symbol name,
to avoid incorrectly merging an unexported name with an exported name.

Fixes #21120

Change-Id: I2e3879d77fa15c05ad92e0bf8e55f74082db5111
Reviewed-on: https://go-review.googlesource.com/50710
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Reviewed-on: https://go-review.googlesource.com/50970
Reviewed-by: Chris Broadfoot <cbro@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Jul 27, 2017

Change https://golang.org/cl/50970 mentions this issue: [release-branch.go1.9] cmd/compile: consider exported flag in namedata

@golang golang locked and limited conversation to collaborators Jul 27, 2018

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