[tools] go/analysis/passes/atomicalign: handle various selector types
9 views
Skip to first unread message
Aurélien Rainone (Gerrit)
Nov 17, 2019, 11:28:20 AM11/17/19
to Ian Lance Taylor, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Aurélien Rainone has uploaded this change for review.
go/analysis/passes/atomicalign: handle various selector types
Address a few problems in the analyzer:
- Only '&struct-var.field' expressions were analyzed
- Crashed when selector expressions involved a package identifier.
This CL adopts a more generic approach and looks for an analizable
selector inside an argument to a sync/atomic function call
Fixes golang/go#34645
Change-Id: I1d2bd1f7f1c13364d1a8f944d6ad082428b8e291
---
M go/analysis/passes/atomicalign/atomicalign.go
M go/analysis/passes/atomicalign/testdata/src/a/a.go
M go/analysis/passes/atomicalign/testdata/src/a/stub.go
M go/analysis/passes/atomicalign/testdata/src/b/b.go
A go/analysis/passes/atomicalign/testdata/src/b/lib.go
D go/analysis/passes/atomicalign/testdata/src/b/stub.go
6 files changed, 67 insertions(+), 20 deletions(-)
diff --git a/go/analysis/passes/atomicalign/atomicalign.go b/go/analysis/passes/atomicalign/atomicalign.go
index 504b1c2..4bd6180 100644
--- a/go/analysis/passes/atomicalign/atomicalign.go
+++ b/go/analysis/passes/atomicalign/atomicalign.go
@@ -69,18 +69,31 @@
return nil, nil
}
-func check64BitAlignment(pass *analysis.Pass, funcName string, arg ast.Expr) {
- // Checks the argument is made of the address operator (&) applied to
- // to a struct field (as opposed to a variable as the first word of
- // uint64 and int64 variables can be relied upon to be 64-bit aligned.
- unary, ok := arg.(*ast.UnaryExpr)
- if !ok || unary.Op != token.AND {
- return
+// varSelector returns the variable behind a selector expression, or false.
+func varSelector(pass *analysis.Pass, expr ast.Expr) (*ast.SelectorExpr, bool) {
+ switch a := expr.(type) {
+ case *ast.UnaryExpr:
+ if a.Op == token.AND {
+ return varSelector(pass, a.X)
+ }
+
+ case *ast.SelectorExpr:
+ switch sel := a.X.(type) {
+ case (*ast.Ident):
+ _, ok := pass.TypesInfo.Uses[sel].(*types.PkgName)
+ if ok { // walk down the package selector
+ return varSelector(pass, a.Sel)
+ }
+ }
+
+ return a, true
}
- // Retrieve the types.Struct in order to get the offset of the
- // atomically accessed field.
- sel, ok := unary.X.(*ast.SelectorExpr)
+ return nil, false
+}
+
+func check64BitAlignment(pass *analysis.Pass, funcName string, arg ast.Expr) {
+ sel, ok := varSelector(pass, arg)
if !ok {
return
}
diff --git a/go/analysis/passes/atomicalign/testdata/src/a/a.go b/go/analysis/passes/atomicalign/testdata/src/a/a.go
index 45dd73d..66078b6 100644
--- a/go/analysis/passes/atomicalign/testdata/src/a/a.go
+++ b/go/analysis/passes/atomicalign/testdata/src/a/a.go
@@ -6,11 +6,13 @@
// +build arm 386
-package testdata
+package a
import (
"io"
"sync/atomic"
+
+ "b"
)
func intsAlignment() {
@@ -228,3 +230,15 @@
atomic.AddUint64(&s1.b, 9) // want "address of non 64-bit aligned field .b passed to atomic.AddUint64"
atomic.AddInt64(&s1.c, 9)
}
+
+var a int64
+
+func globals() {
+ atomic.StoreInt64(&a, 1)
+ atomic.StoreInt64(&b.A, 1)
+ atomic.StoreInt64(b.B, 1)
+ atomic.StoreInt64(&b.C.A, 1) // want "address of non 64-bit aligned field .A passed to atomic.StoreInt64"
+ atomic.StoreInt64(&b.C.B, 1)
+ atomic.StoreInt64(b.D.A, 1) // want "address of non 64-bit aligned field .A passed to atomic.StoreInt64"
+ atomic.StoreInt64(b.D.B, 1)
+}
diff --git a/go/analysis/passes/atomicalign/testdata/src/a/stub.go b/go/analysis/passes/atomicalign/testdata/src/a/stub.go
index 09454f5..dca735e 100644
--- a/go/analysis/passes/atomicalign/testdata/src/a/stub.go
+++ b/go/analysis/passes/atomicalign/testdata/src/a/stub.go
@@ -4,4 +4,4 @@
// This file is only here to not trigger "build constraints exclude all Go files" during tests
-package testdata
+package a
diff --git a/go/analysis/passes/atomicalign/testdata/src/b/b.go b/go/analysis/passes/atomicalign/testdata/src/b/b.go
index adb352a..8802206 100644
--- a/go/analysis/passes/atomicalign/testdata/src/b/b.go
+++ b/go/analysis/passes/atomicalign/testdata/src/b/b.go
@@ -4,7 +4,7 @@
// +build !arm,!386
-package testdata
+package b
import (
"sync/atomic"
diff --git a/go/analysis/passes/atomicalign/testdata/src/b/lib.go b/go/analysis/passes/atomicalign/testdata/src/b/lib.go
new file mode 100644
index 0000000..688d0cb
--- /dev/null
+++ b/go/analysis/passes/atomicalign/testdata/src/b/lib.go
@@ -0,0 +1,27 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package b
+
+var A int64
+var B *int64
+
+type S1 struct {
+ _ int32
+ A int64
+ _ int32
+ B int64
+}
+
+var C S1
+
+type S2 struct {
+ _ int32
+ A *int64
+ _ int32
+ _ int32
+ B *int64
+}
+
+var D S2
diff --git a/go/analysis/passes/atomicalign/testdata/src/b/stub.go b/go/analysis/passes/atomicalign/testdata/src/b/stub.go
deleted file mode 100644
index 09454f5..0000000
--- a/go/analysis/passes/atomicalign/testdata/src/b/stub.go
+++ /dev/null
@@ -1,7 +0,0 @@
-// Copyright 2019 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-// This file is only here to not trigger "build constraints exclude all Go files" during tests
-
-package testdata
To view, visit change 207289. To unsubscribe, or for help writing mail filters, visit settings.
Michael Matloob (Gerrit)
Nov 18, 2019, 4:22:54 PM11/18/19
to Aurélien Rainone, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
2 comments:
File go/analysis/passes/atomicalign/atomicalign.go:
Patch Set #1, Line 73:
varSelectorI'm wondering if we could still constrain the number of possible expressions. My understanding of the accepted set of expressions is an optional addressof operator, which contains a selector.
Could we keep the earlier code, but instead keep walking down on line 83:
exp := unary
for {
sel, ok := exp.X.(*ast.SelectorExpr)
if !ok {
break
}
exp = sel
}Then use exp instead of sel.
-----------------
Another nit. I wonder if we can pick a better name: the result might not be a selector at all in the case of "&x"
switch sel := a.X.(type) {
case (*ast.Ident):
_, ok := pass.TypesInfo.Uses[sel].(*types.PkgName)
if ok { // walk down the package selector
return varSelector(pass, a.Sel)
}
}nit: can this be an if statement?
To view, visit change 207289. To unsubscribe, or for help writing mail filters, visit settings.
Aurélien Rainone (Gerrit)
Dec 1, 2019, 11:28:21 AM12/1/19
to Michael Matloob, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Aurélien Rainone uploaded patch set #2 to this change.
go/analysis/passes/atomicalign: handle various selector types
DO NOT SUBMIT
Address a few problems in the analyzer:
- Only '&struct-var.field' expressions were analyzed
- Crashed when selector expressions involved a package identifier.
This CL adopts a more generic approach and looks for an analizable
selector inside an argument to a sync/atomic function call.
TODO
Do not submit for now, this CL fails on arm for a new test
I just added (and would have always had). This test checks the
behavior with variables that are fieldsin nested structs.
Even if a false negative could be acceptable, it curently produces
false positive.
I'll update this CL to at least sort the false positives.
Fixes golang/go#34645
Change-Id: I1d2bd1f7f1c13364d1a8f944d6ad082428b8e291
---
M go/analysis/passes/atomicalign/atomicalign.go
M go/analysis/passes/atomicalign/testdata/src/a/a.go
M go/analysis/passes/atomicalign/testdata/src/a/stub.go
M go/analysis/passes/atomicalign/testdata/src/b/b.go
A go/analysis/passes/atomicalign/testdata/src/b/lib.go
D go/analysis/passes/atomicalign/testdata/src/b/stub.go
6 files changed, 90 insertions(+), 18 deletions(-)
To view, visit change 207289. To unsubscribe, or for help writing mail filters, visit settings.
Aurélien Rainone (Gerrit)
Dec 1, 2019, 11:31:50 AM12/1/19
to goph...@pubsubhelper.golang.org, Michael Matloob, golang-co...@googlegroups.com
I added DO NOT SUBMIT for now. The original bug is fixed but there's a way to produce false negative with nested structs. Please see the update commit message.
2 comments:
Patch Set #1, Line 73:
varSelector
I'm wondering if we could still constrain the number of possible expressions. […]
Done
switch sel := a.X.(type) {
case (*ast.Ident):
_, ok := pass.TypesInfo.Uses[sel].(*types.PkgName)
if ok { // walk down the package selector
return varSelector(pass, a.Sel)
}
}
nit: can this be an if statement?
Done
To view, visit change 207289. To unsubscribe, or for help writing mail filters, visit settings.
Michael Matloob (Gerrit)
Dec 4, 2019, 4:39:45 PM12/4/19
to Aurélien Rainone, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
I'll wait for your go ahead before continuing on the review.
1 comment:
File go/analysis/passes/atomicalign/atomicalign.go:
if selexp, ok := exp.(*ast.SelectorExpr); ok {
if ident, ok := selexp.X.(*ast.Ident); ok {
if _, ok := pass.TypesInfo.Uses[ident].(*types.PkgName); ok {
exp = selexp.Sel
continue
}
}
sel = selexp
break
}
returncould we indent the error flow here? I think it will make it easier to read.
// If expression is a selector, find the contained variable.
selexp, ok := exp.(*ast.SelectorExpr)
if !ok {
return
}and (optionally) we could do the rest with the inner two ifs too, but i think that's more a matter of taste.
To view, visit change 207289. To unsubscribe, or for help writing mail filters, visit settings.
Aurelien RAINONE (Gerrit)
Jun 6, 2026, 5:54:54 PM (8 hours ago) Jun 6
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Aurelien RAINONE abandoned this change.
Abandoned
Aurelien RAINONE abandoned this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Review
Do-Not-Submit
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages