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/atomic: remove noCopy from Value #21504

Closed
rsc opened this issue Aug 17, 2017 · 7 comments
Closed

sync/atomic: remove noCopy from Value #21504

rsc opened this issue Aug 17, 2017 · 7 comments

Comments

@rsc
Copy link
Contributor

rsc commented Aug 17, 2017

The noCopy in sync/atomic.Value is causing false positives in my code.
The comment on Value says

Once Store has been called, a Value must not be copied.

But the vet check triggers all the time, not just after the first Store call. In particular it triggers when initializing a composite literal like

X{
	Y: Y{},
},

where Y has a sync.Value in it. In my particular code, in order to keep it vet-safe, I am going to have to use an unsafe.Pointer instead, which is terrible. I shouldn't be forced into unsafe code because of a bad annotation.

It's probably too late for Go 1.9, but we should take noCopy out for Go 1.10, especially if we manage to turn vet on by default.

@rsc rsc added this to the Go1.10 milestone Aug 17, 2017
@odeke-em
Copy link
Member

/cc @josharian

@gopherbot
Copy link

Change https://golang.org/cl/56671 mentions this issue: [dev.boringcrypto] crypto/ecdsa: use unsafe.Pointer instead of atomic.Value

gopherbot pushed a commit that referenced this issue Aug 19, 2017
….Value

Using atomic.Value causes vet errors in code copying
PublicKey or PrivateKey structures. I don't think the errors
are accurate, but it's easier to work around them than
to change vet or change atomic.Value.

See #21504.

Change-Id: I3a3435c1fc664cc5166c81674f6f7c58dab35f21
Reviewed-on: https://go-review.googlesource.com/56671
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/57941 mentions this issue: [dev.boringcrypto.go1.8] crypto/ecdsa: use unsafe.Pointer instead of atomic.Value

gopherbot pushed a commit that referenced this issue Aug 26, 2017
…atomic.Value

Using atomic.Value causes vet errors in code copying
PublicKey or PrivateKey structures. I don't think the errors
are accurate, but it's easier to work around them than
to change vet or change atomic.Value.

See #21504.

Change-Id: I3a3435c1fc664cc5166c81674f6f7c58dab35f21
Reviewed-on: https://go-review.googlesource.com/56671
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/57941
@nightlyone
Copy link
Contributor

A different solution might be to ignore the noCopy property on initialization in a pure literal expression. Before assigning the result of a literal to a specific value there are no concurrency issues to worry about. So the false positive is actually a vet bug related to literals and it might make sense to track and treat it as such after closing this issue.

Note: Pure literal expression means constructed by either trivial assignments of other literals or constants or by calling pure functions (as in https://en.wikipedia.org/wiki/Pure_function).

@cespare
Copy link
Contributor

cespare commented Sep 11, 2017

I wasn't able to reproduce the false positive in the report:

package main

import (
	"sync/atomic"
)

type Y struct {
	v atomic.Value
}

type X struct {
	Y Y
}

func main() {
	var x X
	// None of these trigger the vet nocopy check:
	x = X{
		Y: Y{},
	}
	x.Y = Y{}

	// This does, though:
	y := x.Y
}

Also at https://play.golang.org/p/MfsefrZzGB.

@cespare
Copy link
Contributor

cespare commented Sep 11, 2017

Maybe @rsc was just running into a vet bug of some kind. (Perhaps even one that has been fixed?) I'm thinking of #16227 or #13675.

However, I agree that atomic.Value shouldn't have noCopy because its stated copying semantics are more complicated than "do not copy an atomic.Value". I guess this program should be okay(?) but vet warns about it:

package main

import (
	"fmt"
	"sync/atomic"
)

func main() {
	var v0 atomic.Value
	v1 := v0 // assignment copies lock value to v1: sync/atomic.Value contains sync/atomic.noCopy
	v0.Store(3)
	v1.Store(4)
	fmt.Println(v0.Load())
	fmt.Println(v1.Load())
}

@gopherbot
Copy link

Change https://golang.org/cl/80836 mentions this issue: sync/atomic: remove noCopy from Value

@golang golang locked and limited conversation to collaborators Dec 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants