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: inconsistent results with gccgo #39003

Open
go101 opened this issue May 11, 2020 · 7 comments
Open

cmd/compile: inconsistent results with gccgo #39003

go101 opened this issue May 11, 2020 · 7 comments
Labels
Milestone

Comments

@go101
Copy link

@go101 go101 commented May 11, 2020

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

$ go version
go version go1.14.2 linux/amd64

$ gccgo --version
gccgo (Debian 8.3.0-6) 8.3.0

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

import "fmt"

type T struct {
  _ int
  _ bool
}

func main() {
  var t1 = T{123, true}
  fmt.Println(t1) // gc result: {0 false}, gccgo result: {123 true}
}

What did you expect to see?

consistent results from gc and gccgo.

What did you see instead?

inconsistent results from gc and gccgo.

@gopherbot gopherbot added this to the Gccgo milestone May 11, 2020
@ianlancetaylor ianlancetaylor changed the title cmd/compiler: inconsistent results with gccgo cmd/compile: inconsistent results with gccgo May 11, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 11, 2020

Related to #38905.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 11, 2020

@josharian
Copy link
Contributor

@josharian josharian commented May 11, 2020

Related to #31546. See also other related issues linked to there.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 11, 2020

I'd be inclined to say blank fields shouldn't be visible to reflection. They're not exported, they can't be accessed normally, and they can't be a source of field/method promotion.

However, that could change field numberings, which is arguably a backwards incompatible change, if there are programs that hard code field numberings and assume that blanks will be included (as they are today).

@josharian
Copy link
Contributor

@josharian josharian commented May 11, 2020

Why would that change field numbering?

I believe one could implement that by having reflect always return the zero value when queried about a blank field, which would leave field numbering intact.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 11, 2020

@josharian By not visible to reflection, I meant omitting them completely from the reflection metadata. I.e., change NumField to return the number of non-blank fields. (As precedence for something like this, in Go 1.7 we stopped allowing access of non-exported methods via reflection: #15673.)

As for synthesizing zero values when accessing blank fields, how would we handle UnsafeAddr for blank fields?

@josharian
Copy link
Contributor

@josharian josharian commented May 11, 2020

As for synthesizing zero values when accessing blank fields, how would we handle UnsafeAddr for blank fields?

I see (at least) three paths.

(1) Leave behavior as-is. We've then covered up one more little corner of mess and left one corner messy.
(2) Allocate a new zero value and return a pointer to it. Expensive, but who cares? People really shouldn't do this.
(3) Panic. This case is pretty unclear. The docs say: "UnsafeAddr returns a pointer to v's data. It is for advanced clients that also import the "unsafe" package. It panics if v is not addressable." The spec says of the & operator: "The operand must be addressable, that is, either a variable, pointer indirection, or slice indexing operation; or a field selector of an addressable struct operand." But you can't write a field selector involving _, so in a language-lawyerly kind of way, you might say it isn't addressable, and thus may panic.

I find (2) the most appealing of these options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.