Support anonymous structs in checklocks.

Fixes #6558

PiperOrigin-RevId: 396393293

5 files changed, 111 insertions, 67 deletions

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.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.GuardedBy[guardName] = fl
-					}
-				},
-			})
+						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
+}