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

sync: document why copyChecker checks the condition twice #40924

Open
robinbraemer opened this issue Aug 20, 2020 · 5 comments
Open

sync: document why copyChecker checks the condition twice #40924

robinbraemer opened this issue Aug 20, 2020 · 5 comments

Comments

@robinbraemer
Copy link

@robinbraemer robinbraemer commented Aug 20, 2020

Why is the uintptr(*c) != uintptr(unsafe.Pointer(c) condition checked a second time after
the atomic operation?

In case this line can be removed I'm happy to make a PR removing the second condition.

// copyChecker holds back pointer to itself to detect object copying.
type copyChecker uintptr

func (c *copyChecker) check() {
	if uintptr(*c) != uintptr(unsafe.Pointer(c)) && // <---- First
		!atomic.CompareAndSwapUintptr((*uintptr)(c), 0, uintptr(unsafe.Pointer(c))) &&
	        uintptr(*c) != uintptr(unsafe.Pointer(c)) {  // <---- Second
		panic("sync.Cond is copied")
	}
}

uintptr(*c) != uintptr(unsafe.Pointer(c)) {

Pinging @dvyukov since he wrote this code 7 years ago.

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Aug 20, 2020

Why is the uintptr(*c) != uintptr(unsafe.Pointer(c) condition checked a second time after the atomic operation?

Because it can change after the atomic operation.

@ALTree
Copy link
Member

@ALTree ALTree commented Aug 20, 2020

Previously discussed at #34516.

@dvyukov
Copy link
Member

@dvyukov dvyukov commented Aug 20, 2020

@robinbraemer if you want to send a PR, a PR that added an explanation comment would be useful it seems :)

@cagedmantis cagedmantis changed the title sync.Cond: Why does copyChecker check condition twice? sync: document why copyChecker checks the condition twice Aug 24, 2020
@cagedmantis cagedmantis added this to the Backlog milestone Aug 24, 2020
@robinbraemer
Copy link
Author

@robinbraemer robinbraemer commented Aug 25, 2020

I will make an explaining comment. Should I also add this and #34516 issue # in the comment?

robinbraemer added a commit to robinbraemer/go that referenced this issue Sep 7, 2020
It adds two internal comments explaining why `c` is checked twice in a row and is intentional.

There has been confusion about why this was done, confusion should not occur anymore:
- golang#34516
- golang#40924
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 7, 2020

Change https://golang.org/cl/253378 mentions this issue: sync/cond.go: add explanation comments to check func

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants