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: GOEXPERIMENT=loopvar bug #61906

Closed
zigo101 opened this issue Aug 9, 2023 · 13 comments
Closed

cmd/compile: GOEXPERIMENT=loopvar bug #61906

zigo101 opened this issue Aug 9, 2023 · 13 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone

Comments

@zigo101
Copy link

zigo101 commented Aug 9, 2023

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

$ go version
go version go1.21.0 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

func main() {
	for i, ptr := 0, new(int); i < 3; i, ptr = i + 1, &i {
		println(ptr == &i)
	}
}
GOEXPERIMENT=loopvar go run main.go

What did you expect to see?

3 false.

What did you see instead?

1 false, 2 true.

@zigo101 zigo101 changed the title affected/package: cmd/compile: GOEXPERIMENT=loopvar bug Aug 9, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 9, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Aug 9, 2023

Semantically treating i as a new variable on each loop iteration isn't the same as guaranteeing that it'll have a different address on each loop iteration. i is stack-allocated, and that stack slot is reused. (It can be thought of as "freeing and reallocating" on every loop.) If this were guaranteed, then the following would be forced to always heap-allocate x:

for ... {
    x := myBigStruct{}
    // do stuff with x.
}

Note that that example is completely independent of the loop semantics.

(Actually, even heap-allocating isn't enough to guarantee a different address. A full GC could technically happen in between loop iterations, freeing the old heap object and reusing the very same address in the heap. Very unlikely, but theoretically possible.)

As a result, I don't think this is a bug. Closing for now.

CC @rsc @dr2chase

@mknyszek mknyszek closed this as not planned Won't fix, can't repro, duplicate, stale Aug 9, 2023
@mknyszek mknyszek added this to the Backlog milestone Aug 9, 2023
@zigo101
Copy link
Author

zigo101 commented Aug 9, 2023

okay. How about this one:

package main

func main() {
	for i, ptr := 0, new(int); i < 3; i, ptr = i + 1, &i {
		defer func() {
			println(ptr == &i)
		}()
	}
}

What should it print?

@zigo101
Copy link
Author

zigo101 commented Aug 9, 2023

And this:

package main

import "sync"

func main() {
	const N = 3
	var wg sync.WaitGroup
	wg.Add(N)
	var c = make(chan int, 1)
	for i, ptr := 0, new(int); i < N; i, ptr = i + 1, &i {
		go func() {
			c <- i
			println(ptr == &i, ptr, i)
			<-c
			wg.Done()
		}()
	}
	
	wg.Wait()
}

By my understanding, the loop var i should be allocated on heap. The local i variables should be also allocated on heap.

@mknyszek
Copy link
Contributor

mknyszek commented Aug 9, 2023

Both of those are working correctly AFAICT.

In the first one, both ptr and i get captured as new variables by each closure on each iteration. The new variables are created as a copy of the previous iteration's variable before the update (third clause) happens, hence why you see true printed twice. (In the update, the i on the left and right hand side of = is the same i, it's just the new i already by that point.)

This applies to the second program as well (and explains why it prints true twice). Also, all the variables are separately heap allocated in each goroutine's closure as you would expect. You'll notice that the addresses (the value of ptr printed) are different for each goroutine, and take on typical heap address values (0xc000...).

@zigo101
Copy link
Author

zigo101 commented Aug 10, 2023

Do you mean the i in the 3rd clause are the local i variables? This is a surprising new knowledge I get. (This makes the new semantics even more weird.)

If this is true, how to do the synchronizations for the local i between the the 3rd clause and the closure new goroutine?

[edit]: okay. The 3rd clause happens before the execution of the closure new goroutine.

[edit 2]: there are no synchronization problems at the start of a step, but there are still synchronization problems at the time of end of a step. (And this problem is unrelated to the above discussions. I means how the i_outer = i line is synchronized with closure goroutines.)

@zigo101
Copy link
Author

zigo101 commented Aug 10, 2023

Updated the last comment.

@mknyszek
Copy link
Contributor

mknyszek commented Aug 10, 2023

there are no synchronization problems at the start of a step, but there are still synchronization problems at the time of end of a step. (And this problem is unrelated to the above discussions. I means how the i_outer = i line is synchronized with closure goroutines.)

I think that might be an error in the design document (I assume you're referring to the first example here: https://go.googlesource.com/proposal/+/master/design/60078-loopvar.md#language-specification). i_outer = i can happen before the loop body and AFAICT that's approximately the way it works in the implementation. The disassembly seems to support that and I'll note that the race detector doesn't appear to find any issues.

Disclaimer: I'm not going to say that there are definitely no bugs, but it does appear just from a quick look that there aren't any synchronization issues.

EDIT: This is incorrect.

@mknyszek
Copy link
Contributor

mknyszek commented Aug 10, 2023

Sorry, no. I was interpreting the semantics incorrectly. The spec at https://go.googlesource.com/proposal/+/master/design/60078-loopvar.md#language-specification is correct. The loop must do the update after the loop body to capture any changes to the loop variable, but that update always happens on the new variable.

As a result, I believe your example would be racy if the closure updated i or ptr, and that remains racy in the new semantics as well. This is equivalent to any kind of racy update when calling a closure, which remains a hazard everywhere. For example:

x := 5
go func() {
    x += 1 // race
    println(x)
}()
x += 1 // race

One could argue that it would be nice if captures always made a full copy, but in the language spec they do not. And the new loop semantics are consistent with that.

@zigo101
Copy link
Author

zigo101 commented Aug 10, 2023

But, with the old semantics, it is clear that the cause of the racy situation is users' mistakes. No people will argue on this.

With the new semantics, it is not obvious that it is the users' faults, because the assignment i_outer = i is implicit.
Now, we must remind that it is users' responsibility to do synchronizations for the captured loop vars if they will be modified
in step goroutines.
This is just one more new found drawback of the change (other drawbacks: #60078 (comment) and #60078 (comment),
and the most important one: #60078 (comment),
And I would add one more: the new semantics need to too many explanations on how it works.).

In my honest opinion, the benefits of the change is tiny and rare, whereas the drawbacks are too many.
It is not worthy to make this change.

BTW, you ever said one reason for this change to make a consistency between for-range and for;; loops.
In my opinion, the "for;;" loop block in Go is equivalent to the "while" block in some other languages, and
the "for-range" loop block in Go is equivalent to the "for .. in x" or for each block in some other languages.
Seeking for a consistency from the two blocks is a bad intention.

@mknyszek
Copy link
Contributor

mknyszek commented Aug 10, 2023

But, with the old semantics, it is clear that the cause of the racy situation is users' mistakes. No people will argue on this.

To be clear, I'm not trying to argue the merits of the new semantics, just whether there's a bug here or not. I got confused because your post earlier implied the design suggested synchronized semantics, but it in fact does not (I have not been following the discussion too closely).

AFAICT, the implementation follows the spec (and there doesn't appear to be some glaring hole in the spec, at least to me), so there's nothing left to do here.

BTW, you ever said one reason for this change to make a consistency

FWIW I don't recall participating in the loop variable discussion. If you'd like to continue the discussion, a better place to do that would be either the existing proposal issue #60078 or the golang-dev and/or golang-nuts mailing lists.

@zigo101
Copy link
Author

zigo101 commented Aug 11, 2023

Ah, it might be another people in the compile/runtime team and I mistook it is you. Sorry for the wrong mention.

I got confused because your post earlier implied the design suggested synchronized semantics, but it in fact does not (I have not been following the discussion too closely).

I didn't say the design makes the guarantee. I just said that it is easy for people to make synchronization mistakes when they modified loop vars in step goroutines but they tend to think there are no mistakes, and this is a drawback of the new semantics.

@zigo101
Copy link
Author

zigo101 commented Aug 11, 2023

And I will not participate any discussions in #60078, for #60078 (comment).

I surely will continue the discussions with my twitter account and my blog.

@mknyszek
Copy link
Contributor

mknyszek commented Aug 11, 2023

I didn't say the design makes the guarantee.

Indeed, you did not. It was my misunderstanding (and that's all I meant).

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.
Projects
None yet
Development

No branches or pull requests

3 participants