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 · 10 comments
Open

cmd/compile: inconsistent results with gccgo #39003

go101 opened this issue May 11, 2020 · 10 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@go101
Copy link

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

Related to #38905.

@ianlancetaylor
Copy link
Contributor

CC @griesemer @mdempsky

@josharian
Copy link
Contributor

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

@mdempsky
Copy link
Member

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

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

@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

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.

@cagedmantis cagedmantis added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 18, 2020
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@mdempsky
Copy link
Member

Stumbled upon this again. I'm inclined to say that gccgo's behavior is correct, or at least more consistent with the Go spec.

The Go spec says:

  1. that selector expressions cannot use the blank identifier (https://go.dev/ref/spec#Selectors);
  2. that assignments to the (standalone) blank operand are discarded without effect (https://go.dev/ref/spec#Assignment_statements); and
  3. that struct values are compared by comparing their non-blank fields (https://go.dev/ref/spec#Comparison_operators).

But there's nothing that says that blank fields otherwise behave differently than other fields. In particular, there's nothing that says they're not initialized in unkeyed struct literals, or not copied in assignments, or not accessible by packages reflect or unsafe.

@go101
Copy link
Author

go101 commented Sep 13, 2023

Should the assignment take effect?

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}
  t1 = T{}
  fmt.Println(t1) // both: {0 false}
}

@mdempsky
Copy link
Member

mdempsky commented Sep 13, 2023

Should the assignment take effect?

There's nothing in the Go spec to suggest otherwise IMO. I think both cmd/compile and gccgo are both inconsistent with the spec in that regard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Development

No branches or pull requests

6 participants