Skip to content

Conversation

@jkowalski
Copy link
Contributor

@jkowalski jkowalski commented Apr 5, 2023

This allows the use of wrappers such as atomic.Int32 introduced in Go 1.19 without triggering the unexpected call to atomic function.

I tried running recent checklocks on github.com/kopia/kopia and got a ton of such errors, which I don't think should be reported since the wrappers are there to ensure safety of atomic operations. This PR fixes the issue.

@google-cla
Copy link

google-cla bot commented Apr 5, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jkowalski
Copy link
Contributor Author

@amscanne would you mind taking a look at this change?

jkowalski added a commit to jkowalski/kopia that referenced this pull request Apr 14, 2023
We can't enable checklocks on CI yet until
google/gvisor#8807 is merged upstream.

This was tested with private build of checklocks with this patch
applied and the results were clean.
jkowalski added a commit to kopia/kopia that referenced this pull request Apr 14, 2023
We can't enable checklocks on CI yet until
google/gvisor#8807 is merged upstream.

This was tested with private build of checklocks with this patch
applied and the results were clean.
@jkowalski
Copy link
Contributor Author

any interest in that fix?

@jkowalski
Copy link
Contributor Author

@avagin gentle ping, anything I can do to help this get merged? I would really like to re-enable checklocks on Kopia

@avagin
Copy link
Collaborator

avagin commented Aug 18, 2023

Why is it safe to call methods of atomic wrappers in this case?

A few lines bellow, we check that it is not one of Load functions. Why do we want skip Load methods?

 		if !strings.HasPrefix(fn.Name(), "Load") && ar == readOnlyAtomic {

Could you add tests in tools/checklocks/test/? I think it will help to understand the problem.

@jkowalski
Copy link
Contributor Author

atomic.Int32,Int64,etc. are opaque wrappers around int32,int64,... that no longer require calls to package-level functions, such as atomic.LoadNNN(). Instead they handle such things internally and expose type-safe methods.

Current logic will incorrectly flag access such as:

var x atomic.Int32

x.Add(1)
x.Load()
x.Store(3)

My PR simply adds a heuristic which threats all calls to methods (functions with a receiver) in the atomic package as safe.
Let me try to add a test.

@avagin
Copy link
Collaborator

avagin commented Aug 21, 2023

atomic.Int32,Int64,etc. are opaque wrappers around int32,int64,... that no longer require calls to package-level functions, such as atomic.LoadNNN(). Instead they handle such things internally and expose type-safe methods.

I know what atomic.Int32,Int64,etc mean... I was talking about this check:
https://cs.opensource.google/gvisor/gvisor/+/master:tools/checklocks/analysis.go;l=149?q=tools%2Fchecklocks%2Fanalysis.go&ss=gvisor%2Fgvisor
I forgot that these atomic wrappers don't allow to do non-atomic operations or what we call here as mixed access. In gVisor, we have our wrappers that have RaceLoad/RaceStore methods:
https://cs.opensource.google/gvisor/gvisor/+/master:pkg/atomicbitops/aligned_64bit.go;l=80;drc=755c1f242cd410b7311f18807e9cd6d8a116ed09

@avagin
Copy link
Collaborator

avagin commented Aug 21, 2023

Overall, changes look good to me. Please, merge the second commit into the first one or write a normal commit message for it.

@jkowalski jkowalski force-pushed the allow-atomic-methods branch from 6b79fe4 to 591192c Compare August 22, 2023 01:57
This allows the use of wrappers such as atomic.Int32 introduced in
Go 1.19 without triggering the `unexpected call to atomic function`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants