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

[ianlancetaylor]

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

[cuonglm]

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]

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]

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.

[zigo101]

related: #47863 #32035 #32021

[ianlancetaylor]

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]

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]

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]

Sounds right to me, yes.

[mdempsky]

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]

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]

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

[zigo101]

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]

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.

[zigo101]

@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.

[zigo101]

@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]

@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]

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]

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?

[zigo101]

@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]

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]

Which gccgo version do you use?

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

[ianlancetaylor]

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

[zigo101]

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

[mknyszek]

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!