From 6bcacb2fd17fadefbc9fb2eed9059eb36ae2783b Mon Sep 17 00:00:00 2001 From: Adin Scannell Date: Mon, 13 Sep 2021 10:51:23 -0700 Subject: [PATCH] Support anonymous structs in checklocks. Fixes #6558 PiperOrigin-RevId: 396393293 --- pkg/tcpip/stack/nic.go | 20 +++--- tools/checklocks/checklocks.go | 6 +- tools/checklocks/facts.go | 116 ++++++++++++++++++--------------- tools/checklocks/test/BUILD | 1 + tools/checklocks/test/anon.go | 35 ++++++++++ 5 files changed, 111 insertions(+), 67 deletions(-) create mode 100644 tools/checklocks/test/anon.go diff --git a/pkg/tcpip/stack/nic.go b/pkg/tcpip/stack/nic.go index bb56ca9cb..a796942ab 100644 --- a/pkg/tcpip/stack/nic.go +++ b/pkg/tcpip/stack/nic.go @@ -41,17 +41,6 @@ func (l *linkResolver) confirmReachable(addr tcpip.Address) { var _ NetworkInterface = (*nic)(nil) -// TODO(https://gvisor.dev/issue/6558): Use an anonymous struct in nic for this -// once copylocks supports anonymous structs. -type packetEPs struct { - mu sync.RWMutex - - // eps is protected by the mutex, but the values contained in it are not. - // - // +checklocks:mu - eps map[tcpip.NetworkProtocolNumber]*packetEndpointList -} - // nic represents a "network interface card" to which the networking stack is // attached. type nic struct { @@ -85,7 +74,14 @@ type nic struct { promiscuous bool } - packetEPs packetEPs + packetEPs struct { + mu sync.RWMutex + + // eps is protected by the mutex, but the values contained in it are not. + // + // +checklocks:mu + eps map[tcpip.NetworkProtocolNumber]*packetEndpointList + } } // makeNICStats initializes the NIC statistics and associates them to the global diff --git a/tools/checklocks/checklocks.go b/tools/checklocks/checklocks.go index 180f8873f..401fb55ec 100644 --- a/tools/checklocks/checklocks.go +++ b/tools/checklocks/checklocks.go @@ -90,12 +90,14 @@ func run(pass *analysis.Pass) (interface{}, error) { // Find all struct declarations and export relevant facts. pc.forAllTypes(func(ts *ast.TypeSpec) { if ss, ok := ts.Type.(*ast.StructType); ok { - pc.exportLockFieldFacts(ts, ss) + structType := pc.pass.TypesInfo.TypeOf(ts.Name).Underlying().(*types.Struct) + pc.exportLockFieldFacts(structType, ss) } }) pc.forAllTypes(func(ts *ast.TypeSpec) { if ss, ok := ts.Type.(*ast.StructType); ok { - pc.exportLockGuardFacts(ts, ss) + structType := pc.pass.TypesInfo.TypeOf(ts.Name).Underlying().(*types.Struct) + pc.exportLockGuardFacts(structType, ss) } }) diff --git a/tools/checklocks/facts.go b/tools/checklocks/facts.go index 1a43dbbe6..34c9f5ef1 100644 --- a/tools/checklocks/facts.go +++ b/tools/checklocks/facts.go @@ -399,13 +399,12 @@ var ( ) // exportLockFieldFacts finds all struct fields that are mutexes, and ensures -// that they are annotated approperly. +// that they are annotated properly. // // This information is consumed subsequently by exportLockGuardFacts, and this // function must be called first on all structures. -func (pc *passContext) exportLockFieldFacts(ts *ast.TypeSpec, ss *ast.StructType) { - structType := pc.pass.TypesInfo.TypeOf(ts.Name).Underlying().(*types.Struct) - for i := range ss.Fields.List { +func (pc *passContext) exportLockFieldFacts(structType *types.Struct, ss *ast.StructType) { + for i, field := range ss.Fields.List { lff := &lockFieldFacts{ FieldNumber: i, } @@ -426,6 +425,13 @@ func (pc *passContext) exportLockFieldFacts(ts *ast.TypeSpec, ss *ast.StructType // We must always export the lockFieldFacts, since traversal // can take place along any object in the struct. pc.pass.ExportObjectFact(fieldObj, lff) + // If this is an anonymous type, then we won't discover it via + // the AST global declarations. We can recurse from here. + if ss, ok := field.Type.(*ast.StructType); ok { + if st, ok := fieldObj.Type().(*types.Struct); ok { + pc.exportLockFieldFacts(st, ss) + } + } } } @@ -433,59 +439,63 @@ func (pc *passContext) exportLockFieldFacts(ts *ast.TypeSpec, ss *ast.StructType // // This function requires exportLockFieldFacts be called first on all // structures. -func (pc *passContext) exportLockGuardFacts(ts *ast.TypeSpec, ss *ast.StructType) { - structType := pc.pass.TypesInfo.TypeOf(ts.Name).Underlying().(*types.Struct) +func (pc *passContext) exportLockGuardFacts(structType *types.Struct, ss *ast.StructType) { for i, field := range ss.Fields.List { - if field.Doc == nil { - continue - } - var ( - lff lockFieldFacts - lgf lockGuardFacts - ) - pc.pass.ImportObjectFact(structType.Field(i), &lff) fieldObj := structType.Field(i) - for _, l := range field.Doc.List { - pc.extractAnnotations(l.Text, map[string]func(string){ - checkAtomicAnnotation: func(string) { - switch lgf.AtomicDisposition { - case atomicRequired: - pc.maybeFail(fieldObj.Pos(), "annotation is redundant, already atomic required") - case atomicIgnore: - pc.maybeFail(fieldObj.Pos(), "annotation is contradictory, already atomic ignored") - } - lgf.AtomicDisposition = atomicRequired - }, - checkLocksIgnore: func(string) { - switch lgf.AtomicDisposition { - case atomicIgnore: - pc.maybeFail(fieldObj.Pos(), "annotation is redundant, already atomic ignored") - case atomicRequired: - pc.maybeFail(fieldObj.Pos(), "annotation is contradictory, already atomic required") - } - lgf.AtomicDisposition = atomicIgnore - }, - checkLocksAnnotation: func(guardName string) { - // Check for a duplicate annotation. - if _, ok := lgf.GuardedBy[guardName]; ok { - pc.maybeFail(fieldObj.Pos(), "annotation %s specified more than once", guardName) - return - } - fl, ok := pc.resolveField(fieldObj.Pos(), structType, strings.Split(guardName, ".")) - if ok { - // If we successfully resolved - // the field, then save it. - if lgf.GuardedBy == nil { - lgf.GuardedBy = make(map[string]fieldList) + if field.Doc != nil { + var ( + lff lockFieldFacts + lgf lockGuardFacts + ) + pc.pass.ImportObjectFact(structType.Field(i), &lff) + for _, l := range field.Doc.List { + pc.extractAnnotations(l.Text, map[string]func(string){ + checkAtomicAnnotation: func(string) { + switch lgf.AtomicDisposition { + case atomicRequired: + pc.maybeFail(fieldObj.Pos(), "annotation is redundant, already atomic required") + case atomicIgnore: + pc.maybeFail(fieldObj.Pos(), "annotation is contradictory, already atomic ignored") } - lgf.GuardedBy[guardName] = fl - } - }, - }) + lgf.AtomicDisposition = atomicRequired + }, + checkLocksIgnore: func(string) { + switch lgf.AtomicDisposition { + case atomicIgnore: + pc.maybeFail(fieldObj.Pos(), "annotation is redundant, already atomic ignored") + case atomicRequired: + pc.maybeFail(fieldObj.Pos(), "annotation is contradictory, already atomic required") + } + lgf.AtomicDisposition = atomicIgnore + }, + checkLocksAnnotation: func(guardName string) { + // Check for a duplicate annotation. + if _, ok := lgf.GuardedBy[guardName]; ok { + pc.maybeFail(fieldObj.Pos(), "annotation %s specified more than once", guardName) + return + } + fl, ok := pc.resolveField(fieldObj.Pos(), structType, strings.Split(guardName, ".")) + if ok { + // If we successfully resolved + // the field, then save it. + if lgf.GuardedBy == nil { + lgf.GuardedBy = make(map[string]fieldList) + } + lgf.GuardedBy[guardName] = fl + } + }, + }) + } + // Save only if there is something meaningful. + if len(lgf.GuardedBy) > 0 || lgf.AtomicDisposition != atomicDisallow { + pc.pass.ExportObjectFact(structType.Field(i), &lgf) + } } - // Save only if there is something meaningful. - if len(lgf.GuardedBy) > 0 || lgf.AtomicDisposition != atomicDisallow { - pc.pass.ExportObjectFact(structType.Field(i), &lgf) + // See above, for anonymous structure fields. + if ss, ok := field.Type.(*ast.StructType); ok { + if st, ok := fieldObj.Type().(*types.Struct); ok { + pc.exportLockGuardFacts(st, ss) + } } } } diff --git a/tools/checklocks/test/BUILD b/tools/checklocks/test/BUILD index 966bbac22..d4d98c256 100644 --- a/tools/checklocks/test/BUILD +++ b/tools/checklocks/test/BUILD @@ -6,6 +6,7 @@ go_library( name = "test", srcs = [ "alignment.go", + "anon.go", "atomics.go", "basics.go", "branches.go", diff --git a/tools/checklocks/test/anon.go b/tools/checklocks/test/anon.go new file mode 100644 index 000000000..a1f6bddda --- /dev/null +++ b/tools/checklocks/test/anon.go @@ -0,0 +1,35 @@ +// Copyright 2021 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package test + +import "sync" + +type anonStruct struct { + anon struct { + mu sync.RWMutex + // +checklocks:mu + x int + } +} + +func testAnonAccessValid(tc *anonStruct) { + tc.anon.mu.Lock() + tc.anon.x = 1 + tc.anon.mu.Unlock() +} + +func testAnonAccessInvalid(tc *anonStruct) { + tc.anon.x = 1 // +checklocksfail +}