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

spec: clarify the semantics of &p.Field when p is nil #4238

Closed
mirtchovski opened this issue Oct 12, 2012 · 17 comments
Closed

spec: clarify the semantics of &p.Field when p is nil #4238

mirtchovski opened this issue Oct 12, 2012 · 17 comments
Milestone

Comments

@mirtchovski
Copy link
Contributor

mirtchovski commented Oct 12, 2012

Before filing a bug, please check whether it has been fixed since the
latest release. Search the issue tracker and check that you're running the
latest version of Go:

Run "go version" and compare against
http://golang.org/doc/devel/release.html  If a newer version of Go exists,
install it and retry what you did to reproduce the problem.

Thanks.

What steps will reproduce the problem?
If possible, include a link to a program on play.golang.org.
1. http://play.golang.org/p/Bh8FhRu7NQ
2. click 'run'
3. corollary: http://play.golang.org/p/ytggyD6PGV

What is the expected output?
According to the spec, section "selectors", point 4: "If x is of pointer
or interface type and has the value nil, assigning to, evaluating, or calling x.f causes
a run-time panic."

What do you see instead?
The first call to a nil pointer allows the method on "s" to be called. The
second however panics. The difference between the two structs is one byte.

Which compiler are you using (5g, 6g, 8g, gccgo)?
6g and the playground.

Which operating system are you using?
Linux, OSX, Playground

Which version are you using?  (run 'go version')
go version devel +cfa9208b98fc Fri Oct 12 23:19:39 2012 +0800

Please provide any additional information below.
Initially the issue was experienced by real users getting confusing errors deep inside
mutex.Lock while trying to log to a nil logger.
@remyoudompheng
Copy link
Contributor

remyoudompheng commented Oct 12, 2012

Comment 1:

I suggest that the underlying question is: given a nil pointer p to a struct type.
- Must the expression &p.Field cause a runtime-panic?
- If it is legal, is &p.Field a nil pointer to the type of p.Field?
- What about &*p (which is legal and equal to p currently) ?
My understanding of the current spec is that the answers to these questions are
unspecified.

@robpike
Copy link
Contributor

robpike commented Oct 14, 2012

Comment 2:

- Must the expression &p.Field cause a runtime-panic?
not necessarily, because there are instances in which that might not be a problem, such
as unsafe.Sizeof(&p.Field)
  - If it is legal, is &p.Field a nil pointer to the type of p.Field?
no, because if the field offset is 4, &p.Field is 4 if p is nil.
  - What about &*p (which is legal and equal to p currently) ?
not sure
Perhaps the resolution is to clarify the meaning around indirection of nil pointers.

Labels changed: added priority-soon, removed priority-triage.

Status changed to Accepted.

@rsc
Copy link
Contributor

rsc commented Oct 21, 2012

Comment 4:

- x := &*p
This never fails. The & operator computes the address of an expression. Computing the
address of an expression does not require evaluating it, so *p is never evaluated in
this case. &*p is always just 'p'.
- x := &p.Field
This usually does not fail, for the same reason. However, there is a wrinkle. If you had
var p = (*T)(nil) for
type T struct {
    F0 byte
    F1 byte
    F2 byte
    ...
    F1000000 byte
}
then it is fine to make &p.F2 be pointer value 2, since dereferencing it later will
crash. However, as the field offsets get bigger, eventually you cross into actual memory
values, and you cannot let them through. So right now once you get that far (in the gc
compiler I believe the limit is F4096) x := &p.F4096 will panic rather than return a
pointer you could use without causing a trap. It would be unfortunate to need to talk
about this in the spec.
If we want to avoid the panic, the only other option is to reuse another pointer, like 0
or 1, but that would produce the perhaps odd result that &p.F1 == &p.F4096 or maybe
&p.F0 == &p.F4096 or &p.F4096 == nil. On the other hand it might already be considered
odd that &p.F0 == nil.
The same is true for array types, of course.

@griesemer
Copy link
Contributor

griesemer commented Nov 4, 2012

Comment 5:

I might be sufficient to explicitly state that the & operators does not require
evaluation of it's operand (in the http://tip.golang.org/ref/spec#Address_operators
section). This solves &*p and &p.Field for "small" field offsets.
The &p.F4096 panic is an implementation issue: For instance, one could conceive an
implementation where pointers are always a pair (base pointer, offset) and the this
panic could be avoided (at the cost of a more expensive implementation). I'd be ok with
a short sentence mentioning an implementation restriction.

@rsc
Copy link
Contributor

rsc commented Dec 10, 2012

Comment 6:

See also good discussion in issue #4464.

@rsc
Copy link
Contributor

rsc commented Dec 10, 2012

Comment 7:

Issue #4464 has been merged into this issue.

@rsc
Copy link
Contributor

rsc commented Dec 10, 2012

Comment 8:

Labels changed: added size-xl.

@rsc
Copy link
Contributor

rsc commented Jan 31, 2013

Comment 9:

Labels changed: added priority-later, removed priority-soon.

@rsc
Copy link
Contributor

rsc commented Mar 12, 2013

Comment 10:

Labels changed: added go1.1maybe, removed go1.1.

@robpike
Copy link
Contributor

robpike commented May 18, 2013

Comment 11:

Labels changed: added go1.2maybe, removed go1.1maybe.

@rsc
Copy link
Contributor

rsc commented Jun 21, 2013

Comment 12:

Labels changed: added go1.2, removed go1.2maybe.

@remyoudompheng
Copy link
Contributor

remyoudompheng commented Jun 21, 2013

Comment 13:

I also want to mention that I expect that if a pointer is not nil, and I did not use
unsafe, then derefrencing it does not panic.
The spec doesn't say it excplicitly at the moment.

@rsc
Copy link
Contributor

rsc commented Jun 21, 2013

Comment 14:

I am not sure the spec should be required to enumerate the things that
don't panic. Addition doesn't panic either, but we don't mention that. It
should suffice to enumerate the list of things that do panic.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 15:

Labels changed: added feature.

@rsc
Copy link
Contributor

rsc commented Aug 2, 2013

Comment 16:

golang.org/s/go12nil. We've decided to do this.
Robert, do you have bandwidth for the spec changes?

Owner changed to @griesemer.

@griesemer
Copy link
Contributor

griesemer commented Aug 2, 2013

Comment 17:

Probably not today for this one - I suspect it requires some careful combing through the
existing text. Monday.

@robpike
Copy link
Contributor

robpike commented Aug 16, 2013

Comment 18:

Spec change in https://golang.org/cl/12964043

Status changed to Fixed.

This issue was closed.
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

6 participants