[tools] go/analysis/passes/atomicalign: handle pointers to struct
Aurélien Rainone (Gerrit)
Aurélien Rainone has uploaded this change for review.
go/analysis/passes/atomicalign: handle pointers to struct
The atomicalign checker detects non-64-bit aligned struct field
arguments to sync/atomic functions but currently misses out cases
where the struct variable identifier is a pointer to struct. This
is very common as it happens when the 64-bit field is accessed
in a method with pointer receiver, where the struct is itself the
method receiver. Add some tests to cover that new case.
While I'm at it, fix some typos.
Change-Id: Ifdab50879af8e978a31bea1cf100fc020f92d722
---
M go/analysis/passes/atomicalign/atomicalign.go
M go/analysis/passes/atomicalign/testdata/src/a/a.go
2 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/go/analysis/passes/atomicalign/atomicalign.go b/go/analysis/passes/atomicalign/atomicalign.go
index d3fc3e2..0326bf5 100644
--- a/go/analysis/passes/atomicalign/atomicalign.go
+++ b/go/analysis/passes/atomicalign/atomicalign.go
@@ -21,7 +21,7 @@
var Analyzer = &analysis.Analyzer{
Name: "atomicalign",
- Doc: "check for non-64-bits-aligned arguments to sync/atomic functions",
+ Doc: "check for non-64-bit-aligned arguments to sync/atomic functions",
Requires: []*analysis.Analyzer{inspect.Analyzer},
Run: run,
}
@@ -70,7 +70,7 @@
}
func check64BitAlignment(pass *analysis.Pass, funcName string, arg ast.Expr) {
- // Checks the argument is made of the address operator (&) applied to
+ // Checks the argument is made of the address operator (&) applied
// 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)
@@ -80,16 +80,18 @@
// Retrieve the types.Struct in order to get the offset of the
// atomically accessed field.
- sel, ok := unary.X.(*ast.SelectorExpr)
+ selector, ok := unary.X.(*ast.SelectorExpr)
if !ok {
return
}
- tvar, ok := pass.TypesInfo.Selections[sel].Obj().(*types.Var)
+
+ sel := pass.TypesInfo.Selections[selector]
+ tvar, ok := sel.Obj().(*types.Var)
if !ok || !tvar.IsField() {
return
}
- stype, ok := pass.TypesInfo.Types[sel.X].Type.Underlying().(*types.Struct)
+ stype, ok := sel.Recv().Underlying().(*types.Struct)
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..7aa7278 100644
--- a/go/analysis/passes/atomicalign/testdata/src/a/a.go
+++ b/go/analysis/passes/atomicalign/testdata/src/a/a.go
@@ -228,3 +228,22 @@
atomic.AddUint64(&s1.b, 9) // want "address of non 64-bit aligned field .b passed to atomic.AddUint64"
atomic.AddInt64(&s1.c, 9)
}
+
+type t struct {
+ _ int32
+ a int64
+ _ int16
+ _ int16
+ b uint64
+}
+
+func (t *t) structPointerReceiver() {
+ atomic.LoadInt64(&t.a) // want "address of non 64-bit aligned field .a passed to atomic.LoadInt64"
+ atomic.LoadUint64(&t.b)
+}
+
+func structPointer() {
+ t := &t{}
+ atomic.StoreInt64(&t.a, -1) // want "address of non 64-bit aligned field .a passed to atomic.StoreInt64"
+ atomic.StoreUint64(&t.b, 1)
+}
To view, visit change 163997. To unsubscribe, or for help writing mail filters, visit settings.
Aurélien Rainone (Gerrit)
This is the original, untouched CL 158999, that was reverted by 160839.
It should pass on arm but still on 386.
Aurélien Rainone (Gerrit)
Patch Set 1:
This is the original, untouched CL 158999, that was reverted by 160839.
It should pass on arm but still on 386.
still *fail* on 386
Alan Donovan (Gerrit)
Patch set 1:Run-TryBot +1
Gobot Gobot (Gerrit)
Gobot Gobot (Gerrit)
Build is still in progress...
This change failed on linux-386:
See https://storage.googleapis.com/go-build-log/1f17d610/linux-386_8fe595f5.log
Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report.
Gobot Gobot (Gerrit)
8 of 18 TryBots failed:
Failed on linux-386: https://storage.googleapis.com/go-build-log/1f17d610/linux-386_8fe595f5.log
Failed on linux-amd64-race: https://storage.googleapis.com/go-build-log/1f17d610/linux-amd64-race_5465c046.log
Failed on misc-vet-vetall: https://storage.googleapis.com/go-build-log/1f17d610/misc-vet-vetall_31e5600a.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/1f17d610/linux-amd64_fc75b8db.log
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/1f17d610/freebsd-amd64-12_0_41493a7c.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/1f17d610/openbsd-amd64-64_8827543a.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/1f17d610/windows-amd64-2016_398775c1.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/1f17d610/windows-386-2008_de2d5b73.log
Consult https://build.golang.org/ to see whether they are new failures.
Patch set 1:TryBot-Result -1
Aurélien Rainone (Gerrit)
Aurélien Rainone uploaded patch set #3 to this change.
go/analysis/passes/atomicalign: handle pointers to struct
The atomicalign checker detects non-64-bit aligned struct field
arguments to sync/atomic functions but currently misses out cases
where the struct variable identifier is a pointer to struct. This
is very common as it happens when the 64-bit field is accessed
in a method with pointer receiver, where the struct is itself the
method receiver. Add some tests to cover that new case.
While I'm at it, fix some typos.
Change-Id: Ifdab50879af8e978a31bea1cf100fc020f92d722
---
M go/analysis/passes/atomicalign/atomicalign.go
M go/analysis/passes/atomicalign/testdata/src/a/a.go
2 files changed, 31 insertions(+), 7 deletions(-)
To view, visit change 163997. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
1 comment:
File go/analysis/passes/atomicalign/atomicalign.go:
Patch Set #3, Line 32:
"mips"What about "mipsle"?
But what was wrong with the old code?
To view, visit change 163997. To unsubscribe, or for help writing mail filters, visit settings.
Aurélien Rainone (Gerrit)
1 comment:
Patch Set #3, Line 32:
"mips"
What about "mipsle"? […]
As this had been reverted because it was failing on some architectures on which I hadn't plan that it would :) I though I'd give it another try by limiting the affected archs to the list of arches specified in the sync/atomic BUG note.
To view, visit change 163997. To unsubscribe, or for help writing mail filters, visit settings.
Aurélien Rainone (Gerrit)
Honestly I'm a bit in the dark, I don't know wether the failed builds were due to the logic code being wrong somewhere or the list of architectures on which this runs not enough restrictive
Aurélien Rainone (Gerrit)
Aurélien Rainone uploaded patch set #4 to this change.
go/analysis/passes/atomicalign: handle pointers to struct
The atomicalign checker detects non-64-bit aligned struct field
arguments to sync/atomic functions but currently misses out cases
where the struct variable identifier is a pointer to struct. This
is very common as it happens when the 64-bit field is accessed
in a method with pointer receiver, where the struct is itself the
method receiver. Add some tests to cover that new case.
While I'm at it, fix some typos.
Change-Id: Ifdab50879af8e978a31bea1cf100fc020f92d722
---
M go/analysis/passes/atomicalign/atomicalign.go
M go/analysis/passes/atomicalign/testdata/src/a/a.go
2 files changed, 31 insertions(+), 7 deletions(-)
To view, visit change 163997. To unsubscribe, or for help writing mail filters, visit settings.
Aurélien Rainone (Gerrit)
Could somebody run try-bots to know wether at least some platforms are fixed?
Daniel Martí (Gerrit)
Patch set 4:Run-TryBot +1
Gobot Gobot (Gerrit)
Gobot Gobot (Gerrit)
Build is still in progress...
This change failed on linux-386:
See https://storage.googleapis.com/go-build-log/ce656af9/linux-386_109185b7.log
Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.
Gobot Gobot (Gerrit)
3 of 10 TryBots failed:
Failed on linux-386: https://storage.googleapis.com/go-build-log/ce656af9/linux-386_109185b7.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/ce656af9/windows-386-2008_42f8d548.log
Failed on linux-amd64-race: https://storage.googleapis.com/go-build-log/ce656af9/linux-amd64-race_9d4c3552.log
Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test *exactly* your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.
Patch set 4:TryBot-Result -1
Aurélien Rainone (Gerrit)
Thanks for the try-bots Daniel.
So among the 3 failures, I believe only 2 were due to this CL: linux windows-386 and linux-386, 2 platforms that should be affected by the 64-bit alignment bug, per the sync/atomic BUG note.
The tests failures are located on the exact same lines for both platforms.
Basically the test is doing: on platforms that should be affected, I atomically access a field that I arranged NOT to be 64-bit aligned. The problem is that the fields gets 64-bit aligned, making the analyzer fail.
NOTE: this behavior is only triggered for struct fields accessed in a method in which the received is the containing struct. When the field is accessed in a free function, the test passes, which should mean the fields aren't 64-bit aligned, are caught by the analyzer, the test pass.
To sum up:
The analyzer follows the BUG note, that says ARM, x86-32, and 32-bit MIPS should have the problem, but test only are green on ARM.
Could it be that the alignment problem is deterministic on ARM but not on x86-32 and mips?
Daniel Martí (Gerrit)
Sorry, this is where my knowledge of architectures falls short. Perhaps Michael or Ian know better, who maintain go/analysis.
Daniel Martí (Gerrit)
Daniel Martí removed Brad Fitzpatrick from this change.
To view, visit change 163997. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Martí (Gerrit)
Daniel Martí removed Alan Donovan from this change.
To view, visit change 163997. To unsubscribe, or for help writing mail filters, visit settings.
Aurélien Rainone (Gerrit)
NOTE: ... which the received is ...
Oups: obviously I meant "receiver" here
Michael Matloob (Gerrit)
Michael Matloob added 1 comment![]( "Open in Gerrit")
Hi sorry this fell through the cracks. If this still needs review, we should find someone with more context to review this.
Related details
- Aurelien RAINONE
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aurelien RAINONE (Gerrit)
Aurelien RAINONE abandoned this change.
Aurelien RAINONE abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |