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

Jump to bottom

sync: document why copyChecker checks the condition twice #40924

Closed
robinbraemer opened this issue Aug 20, 2020 · 6 comments
Closed

sync: document why copyChecker checks the condition twice #40924

robinbraemer opened this issue Aug 20, 2020 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.22

Comments

@robinbraemer
Copy link

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")
	}
}

go/src/sync/cond.go

Line 84 in 46ca7b5

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

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

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 commented Aug 20, 2020

Previously discussed at #34516.

@dvyukov
Copy link
Member

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 :)

@ALTree ALTree mentioned this issue Aug 20, 2020
Closed
@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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 24, 2020
@cagedmantis cagedmantis added this to the Backlog milestone Aug 24, 2020
@robinbraemer
Copy link
Author

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
@robinbraemer
add explanation comment in sync/cond.go
fe392ce 
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
@robinbraemer robinbraemer mentioned this issue Sep 7, 2020
Closed
@gopherbot
Copy link

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

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Aug 12, 2023
@qiulaidongfeng
sync: document why copyChecker checks the condition twice
ba73a43 
Fixes: golang#40924
Change-Id: I249a278be1ec3c67088819af4456e6c393431724
@qiulaidongfeng qiulaidongfeng mentioned this issue Aug 12, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/518961 mentions this issue: sync: document why copyChecker checks the condition twice

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Aug 15, 2023
@qiulaidongfeng
sync: document why copyChecker checks the condition twice
772c7ae 
Fixes golang#40924
Change-Id: I249a278be1ec3c67088819af4456e6c393431724
@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Oct 11, 2023
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.22
Development

Successfully merging a pull request may close this issue.

sync: document why copyChecker checks the condition twice qiulaidongfeng/go
7 participants
@davecheney @cagedmantis @dvyukov @dmitshur @ALTree @gopherbot @robinbraemer