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: defer of embedded method resolves pointer too early #52025

Open
ianlancetaylor opened this issue Mar 30, 2022 · 24 comments
Open

cmd/compile: defer of embedded method resolves pointer too early #52025

ianlancetaylor opened this issue Mar 30, 2022 · 24 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

Consider this test case:

package main

type CloseSetter interface {
	Set()
	Close() error
}

type S1 struct {
	f int
}

func (p *S1) Close() error {
	if p.f != 0 {
		panic(p.f)
	}
	return nil
}

type S2 struct {
	*S1
}

func (p *S2) Set() {
	p.S1 = &S1{0}
}

func F1(p CloseSetter) {
	p.Set()
	p.Close()
}

func F2() {
	s2 := &S2{}
	defer s2.Close()
	F1(s2)
}

func main() {
	F2()
}

When run with the gc compiler, this crashes:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x455534]

goroutine 1 [running]:
main.(*S1).Close(0xc000096000?)
	/home/iant/foo10.go:13 +0x14
main.F2()
	/home/iant/foo10.go:36 +0x7c
main.main()
	/home/iant/foo10.go:39 +0x17
exit status 2

When run with the gccgo compiler it exits successfully without printing anything.

I think the difference is that with the gc compiler the defer statement is resolving the embedded pointer at the point of the defer, and thus deferred a call to a nil pointer. If that is correct, then that seems wrong. A defer statement resolves all the arguments, but it shouldn't resolve s2 into s2.S1.

Or maybe gccgo is incorrectly not crashing, but I don't yet see it.

CC @randall77 @griesemer @mdempsky

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 30, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.19 milestone Mar 30, 2022
@cuonglm
Copy link
Member

cuonglm commented Mar 30, 2022

A defer statement resolves all the arguments, but it shouldn't resolve s2 into s2.S1.

Could you please elaborate more?

Not only the arguments, but also the function value is also evaluated when defer statement is executed. So s2 must be resolved to s2.S1, otherwise, the Close method can not be found on S2.

@griesemer
Copy link
Contributor

Per the spec:

Each time a "defer" statement executes, the function value and parameters to the call are evaluated as usual and saved anew but the actual function is not invoked.

@mdempsky
Copy link
Member

Related: #38634

The interpretation I've been leaning towards lately is that x.M(...) always means T.M(y, ...), where T is the declared method's receiver parameter type (i.e., the receiver type as it literally appears in the user source code) and y is the appropriate addressing/dereferencing of x to produce a value of type T.

So in the example above, s2.Close() means (*S1).Close(s2.S1). And so defer s2.Close() means that s2.S1 gets evaluated immediately at the point of execution of the defer statement itself.

Note that cmd/compile gives the same behavior whether you write defer s2.Close() or f := s2.Close; defer f(), whereas gccgo gives them different semantics.

@go101
Copy link

go101 commented Mar 30, 2022

related: #47863 #32035 #32021

@ianlancetaylor
Copy link
Contributor Author

Note that cmd/compile gives the same behavior whether you write defer s2.Close() or f := s2.Close; defer f(), whereas gccgo gives them different semantics.

That is true, but here again there is a difference between gc and gccgo. With gc the nil dereference happens when the defer statement is executed. With gccgo the nil dereference happens on the line f := s2.Close.

@ianlancetaylor
Copy link
Contributor Author

So it sounds like the argument is that the defer statement should evaluate s2.Close to come up with (*s2.S1).Close. And that turns what appears to be the method value s2.Close into the method value s2.S1.Close.

Maybe that is right but I'm not yet convinced. The rules for method promotion say that the method Close is promoted to be a method of S2. So s2 really does have a method Close. Why should the defer statement try to resolve s2.Close into anything else? Sure, we need to do something when we call the method, but at the point of the defer we aren't calling it.

@griesemer
Copy link
Contributor

It sounds like both compilers have bugs here. Assuming @ianlancetaylor is correct (s2 does have a method Close), then:

  • gc should not panic upon execution of the defer (this issue)
  • gccgo should also not panic upon computing f := s2.Close since the Close method does exists (it's a wrapper funtion) and is not yet called.

Does that sound right?

@ianlancetaylor
Copy link
Contributor Author

Sounds right to me, yes.

@mdempsky
Copy link
Member

mdempsky commented Mar 30, 2022

I'm still of the opinion that implicit addressing/dereferencing and embedded field traversal should happen right away, both in the case of method values and deferred method calls. That is, I think defer s2.Close() (this issue) should panic at the execution of the defer statement itself (not at the point of execution of function return, when deferred calls are executed), and f := s2.Close should panic right away too.

In cases where x is addressable and M is a pointer receiver method, then x.M is shorthand for (&x).M and the addressing operation has to happen immediately. It seems inconsistent to me to have that implicit addressing operation happen early, but s2.Close's implicit (*s2).S1 evaluation to happen late.

(Also, in case it sounded like I was suggesting otherwise, I'm sure cmd/compile has issues/inconsistencies in this area too. To date, I've mostly tried to just maintain existing method selector semantics within cmd/compile while unifying call handling, and haven't gotten far enough yet to be pressed to make any hard decisions that risk changing existing behavior.)

@nveeser
Copy link

nveeser commented Mar 30, 2022

Related: #38634

The interpretation I've been leaning towards lately is that x.M(...) always means T.M(y, ...), where T is the declared method's receiver parameter type (i.e., the receiver type as it literally appears in the user source code) and y is the appropriate addressing/dereferencing of x to produce a value of type T.

So in the example above, s2.Close() means (*S1).Close(s2.S1). And so defer s2.Close() means that s2.S1 gets evaluated immediately at the point of execution of the defer statement itself.

Not intuitive but that makes sense if that's how I should start thinking about it.

Seems though f := s2.Close is implemented as a wrapper function: func (s *S2) Close() { s.S1.Close() }

So is the issue here is that defer s2.Close() seems to be "closing" over s2.S1 rather than s2 itself?

@mdempsky
Copy link
Member

@nveeser I wouldn't recommend trying to rationalize the compilers' (or at least cmd/compile's) existing behaviors here.

@go101
Copy link

go101 commented Mar 31, 2022

So s2 really does have a method Close

Seems though f := s2.Close is implemented as a wrapper function: func (s *S2) Close() { s.S1.Close() }

Didn't this issue formally deny this?

Quoting @cuonglm's wording In that issue thread (replace relevant identifers):

... then S2 does not have method Close, .... Close is the promoted method of S2 from embedded field S1, and Close appears in S2's method set.

I do agree some wording in spec need be more clear.

@cuonglm
Copy link
Member

cuonglm commented Mar 31, 2022

So s2 really does have a method Close

Seems though f := s2.Close is implemented as a wrapper function: func (s *S2) Close() { s.S1.Close() }

Didn't this issue formally deny this?

Quoting @cuonglm's wording In that issue thread (replace relevant identifers):

... then S2 does not have method Close, .... Close is the promoted method of S2 from embedded field S1, and Close appears in S2's method set.

I do agree some wording in spec need be more clear.

The wrapper function is gc specific implementation, there's no visible in source code nor in the Go spec that S2 has Close method in case of promoted method.

@go101
Copy link

go101 commented Mar 31, 2022

@ianlancetaylor

When run with the gccgo compiler it exits successfully without printing anything.

Which gccgo version do you use? It looks gccgo (Debian 10.2.1-6) 10.2.1 20210110 outputs similar messages as gc 1.18.

@go101
Copy link

go101 commented Mar 31, 2022

@cuonglm

The wrapper function is gc specific implementation ...

It looks gc doesn't make the wrapper, which is why the program panics by using gc.
It looks the s2.Close has been normalized as (*(*s2).S1).Close at compile time.

@cuonglm
Copy link
Member

cuonglm commented Mar 31, 2022

@cuonglm

The wrapper function is gc specific implementation ...

It looks gc doesn't make the wrapper, which is why the program panics by using gc. It looks the s2.Close has been normalized as (*(*s2).S1).Close at compile time.

Looks at the result of go tool compile -W -l or go tool compile -S -l, you will see S2.Close there. S2.Close simply calls (*S1).Close.

@nveeser
Copy link

nveeser commented Mar 31, 2022

Apologies - I wasn't looking at the compiler, just reflecting back the discussion so far, to clarify what I am reading.

Sounds like the open question is whether the method value expression s2.Close evaluates to s2.S1.Close or whether the spec defines that s2 actually has a method which is "effectively" a wrapper.

@nveeser
Copy link

nveeser commented Mar 31, 2022

I'm still of the opinion that implicit addressing/dereferencing and embedded field traversal should happen right away, both in the case of method values and deferred method calls. That is, I think defer s2.Close() (this issue) should panic at the execution of the defer statement itself (not at the point of execution of function return, when deferred calls are executed), and f := s2.Close should panic right away too.

Novice question - why would s2.Close panic? The issue is that s2.S1 is nil, not s2 and a nil receiver is valid right?

@go101
Copy link

go101 commented Mar 31, 2022

@cuonglm

Looks at the result of go tool compile -W -l or go tool compile -S -l, you will see S2.Close there. S2.Close simply calls (*S1).Close.

But it is not used in defer s2.Close()? Otherwise, the program in the first comment should not panic.

@nveeser

The issue is that s2.S1 is nil, not s2 and a nil receiver is valid right?

A nil receiver is valid. The panic is caused by dereferencing s2.S1.

@cuonglm
Copy link
Member

cuonglm commented Mar 31, 2022

But it is not used in defer s2.Close()? Otherwise, the program in the first comment should not panic.

Yes, I answered for your statement that gc does not make the wrapper, it does make!

Like I said (and discussed with you in the past), that's gc specific implementation, and happen much later after typecheck. In fact, go/types also normalize s2.Close as s2.(*S1).Close.

@ianlancetaylor
Copy link
Contributor Author

Which gccgo version do you use?

I tested with gccgo (GCC) 12.0.0 20210921 (experimental).

@ianlancetaylor
Copy link
Contributor Author

Rolling forward to 1.20. Please comment if you disagree. Thanks.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.19, Go1.20 Jun 24, 2022
@go101
Copy link

go101 commented Jun 25, 2022

This issue might be only a spec issue. But this one is a real bug.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@mknyszek
Copy link
Contributor

mknyszek commented Jun 7, 2023

This was found in triage, ping @griesemer @mdempsky @ianlancetaylor. Do we want any kind of resolution here in Go 1.22? If yes, and you'd like to do it, feel free to assign to yourself. If not, please push into Backlog. Thanks!

@gopherbot gopherbot modified the milestones: Go1.22, Go1.23 Feb 6, 2024
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. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Status: No status
Development

No branches or pull requests

8 participants