Add automatic lock inference and globals support.

Lock inference will apply annotations to all fields that seem to be
protected. This is currently disabled for all code by default, but it
can be enabled as annotations are applied more broadly.

PiperOrigin-RevId: 407501915

nogo.yaml

@@ -48,6 +48,7 @@ global:
       - "duplicate import"
       # These will never be annotated.
       - "unexpected call to atomic function"
+      - "may require checklocks annotation for"
       # Generated proto code creates declarations like 'var start int = iNdEx'
       - "should omit type .* from declaration; it will be inferred from the right-hand side"
   internal:
@@ -63,6 +64,7 @@ global:
       - "unexpected call to atomic function.*"
       - "return with unexpected locks held.*"
       - "incompatible return states.*"
+      - "may require checklocks annotation for.*"
     exclude:
       # Generated: exempt all.
       - pkg/shim/runtimeoptions/runtimeoptions_cri.go

tools/checklocks/README.md

@@ -1,6 +1,6 @@
 # CheckLocks Analyzer
 
-<!--* freshness: { owner: 'gvisor-eng' reviewed: '2021-10-15' } *-->
+<!--* freshness: { owner: 'gvisor-eng' reviewed: '2021-10-20' } *-->
 
 Checklocks is an analyzer for lock and atomic constraints. The analyzer relies
 on explicit annotations to identify fields that should be checked for access.
@@ -75,7 +75,26 @@ annotation refers either to something that is not a 'sync.Mutex' or
 'sync.RWMutex' or where the field does not exist at all. This will prevent the
 annotations from becoming stale over time as fields are renamed, etc.
 
-# Currently not supported
+## Lock suggestions
+
+Based on locks held during field access, the analyzer will suggest annotations.
+These can be ignored with the standard `+checklocksignore` annotation.
+
+The annotation will be generated when the lock is held the vast majority of the
+time the field is accessed. Note that it is possible for this frequency to be
+greater than 100%, if the lock is held multiple times. For example:
+
+```go
+func foo(ts1 *testStruct, ts2 *testStruct) {
+  ts1.Lock()
+  ts2.Lock()
+  ts1.gaurdedField = 1 // 200% locks held.
+  ts1.Unlock()
+  ts2.Unlock()
+}
+```
+
+## Currently not supported
 
 1.  Anonymous functions are not correctly evaluated. The analyzer does not
     currently support specifying annotations on anonymous functions as a result

tools/checklocks/analysis.go

@@ -168,19 +168,19 @@ func resolveStruct(typ types.Type) (*types.Struct, bool) {
 
 func findField(typ types.Type, field int) (types.Object, bool) {
 	structType, ok := resolveStruct(typ)
-	if !ok {
+	if !ok || field >= structType.NumFields() {
 		return nil, false
 	}
 	return structType.Field(field), true
 }
 
-// instructionWithReferrers is a generalization over ssa.Field, ssa.FieldAddr.
-type instructionWithReferrers interface {
-	ssa.Instruction
+// almostInst is a generalization over ssa.Field, ssa.FieldAddr, ssa.Global.
+type almostInst interface {
+	Pos() token.Pos
 	Referrers() *[]ssa.Instruction
 }
 
-// checkFieldAccess checks the validity of a field access.
+// checkGuards checks the guards held.
 //
 // This also enforces atomicity constraints for fields that must be accessed
 // atomically. The parameter isWrite indicates whether this field is used
@@ -188,41 +188,46 @@ type instructionWithReferrers interface {
 //
 // Note that this function is not called if lff.Ignore is true, since it cannot
 // discover any local anonymous functions or closures.
-func (pc *passContext) checkFieldAccess(inst instructionWithReferrers, structObj ssa.Value, field int, ls *lockState, isWrite bool) {
+func (pc *passContext) checkGuards(inst almostInst, from ssa.Value, accessObj types.Object, ls *lockState, isWrite bool) {
 	var (
-		lff         lockFieldFacts
 		lgf         lockGuardFacts
 		guardsFound int
-		guardsHeld  int
+		guardsHeld  = make(map[string]struct{}) // Keyed by resolved string.
 	)
 
-	fieldObj, _ := findField(structObj.Type(), field)
-	pc.pass.ImportObjectFact(fieldObj, &lff)
-	pc.pass.ImportObjectFact(fieldObj, &lgf)
+	// Load the facts for the object accessed.
+	pc.pass.ImportObjectFact(accessObj, &lgf)
 
-	for guardName, fl := range lgf.GuardedBy {
+	// Check guards held.
+	for guardName, fgr := range lgf.GuardedBy {
 		guardsFound++
-		r := fl.resolve(structObj)
+		r := fgr.resolveField(pc, ls, from)
+		if !r.valid() {
+			// See above; this cannot be forced.
+			pc.maybeFail(inst.Pos(), "field %s cannot be resolved", guardName)
+			continue
+		}
 		s, ok := ls.isHeld(r, isWrite)
 		if ok {
-			guardsHeld++
+			guardsHeld[s] = struct{}{}
 			continue
 		}
 		if _, ok := pc.forced[pc.positionKey(inst.Pos())]; ok {
 			// Mark this as locked, since it has been forced. All
 			// forces are treated as an exclusive lock.
-			ls.lockField(r, true /* exclusive */)
-			guardsHeld++
+			s, _ := ls.lockField(r, true /* exclusive */)
+			guardsHeld[s] = struct{}{}
 			continue
 		}
 		// Note that we may allow this if the disposition is atomic,
 		// and we are allowing atomic reads only. This will fall into
 		// the atomic disposition check below, which asserts that the
-		// access is atomic. Further, guardsHeld < guardsFound will be
-		// true for this case, so we require it to be read-only.
+		// access is atomic. Further, len(guardsHeld) < guardsFound
+		// will be true for this case, so we require it to be
+		// read-only.
 		if lgf.AtomicDisposition != atomicRequired {
 			// There is no force key, no atomic access and no lock held.
-			pc.maybeFail(inst.Pos(), "invalid field access, must hold %s (%s) when accessing %s (locks: %s)", guardName, s, fieldObj.Name(), ls.String())
+			pc.maybeFail(inst.Pos(), "invalid field access, %s (%s) must be locked when accessing %s (locks: %s)", guardName, s, accessObj.Name(), ls.String())
 		}
 	}
 
@@ -230,25 +235,75 @@ func (pc *passContext) checkFieldAccess(inst instructionWithReferrers, structObj
 	switch lgf.AtomicDisposition {
 	case atomicRequired:
 		// Check that this is used safely as an input.
-		readOnly := guardsHeld < guardsFound
+		readOnly := len(guardsHeld) < guardsFound
 		if refs := inst.Referrers(); refs != nil {
 			for _, otherInst := range *refs {
-				pc.checkAtomicCall(otherInst, fieldObj, true, readOnly)
+				pc.checkAtomicCall(otherInst, accessObj, true, readOnly)
 			}
 		}
 		// Check that this is not otherwise written non-atomically,
 		// even if we do hold all the locks.
 		if isWrite {
-			pc.maybeFail(inst.Pos(), "non-atomic write of field %s, writes must still be atomic with locks held (locks: %s)", fieldObj.Name(), ls.String())
+			pc.maybeFail(inst.Pos(), "non-atomic write of field %s, writes must still be atomic with locks held (locks: %s)", accessObj.Name(), ls.String())
 		}
 	case atomicDisallow:
 		// Check that this is *not* used atomically.
 		if refs := inst.Referrers(); refs != nil {
 			for _, otherInst := range *refs {
-				pc.checkAtomicCall(otherInst, fieldObj, false, false)
+				pc.checkAtomicCall(otherInst, accessObj, false, false)
 			}
 		}
 	}
+
+	// Check inferred locks.
+	if accessObj.Pkg() == pc.pass.Pkg {
+		oo := pc.observationsFor(accessObj)
+		oo.total++
+		for s, info := range ls.lockedMutexes {
+			// Is this an object for which we have facts? If there
+			// is no ability to name this object, then we don't
+			// bother with any inferrence. We also ignore any self
+			// references (e.g. accessing a mutex while you are
+			// holding that exact mutex).
+			if info.object == nil || accessObj == info.object {
+				continue
+			}
+			// Has this already been held?
+			if _, ok := guardsHeld[s]; ok {
+				oo.counts[info.object]++
+				continue
+			}
+			// Is this a global? Record directly.
+			if _, ok := from.(*ssa.Global); ok {
+				oo.counts[info.object]++
+				continue
+			}
+			// Is the object a sibling to the accessObj? We need to
+			// check all fields and see if they match. We accept
+			// only siblings and globals for this recommendation.
+			structType, ok := resolveStruct(from.Type())
+			if !ok {
+				continue
+			}
+			for i := 0; i < structType.NumFields(); i++ {
+				if fieldObj := structType.Field(i); fieldObj == info.object {
+					// Add to the maybe list.
+					oo.counts[info.object]++
+				}
+			}
+		}
+	}
+}
+
+// checkFieldAccess checks the validity of a field access.
+func (pc *passContext) checkFieldAccess(inst almostInst, structObj ssa.Value, field int, ls *lockState, isWrite bool) {
+	fieldObj, _ := findField(structObj.Type(), field)
+	pc.checkGuards(inst, structObj, fieldObj, ls, isWrite)
+}
+
+// checkGlobalAccess checks the validity of a global access.
+func (pc *passContext) checkGlobalAccess(g *ssa.Global, ls *lockState, isWrite bool) {
+	pc.checkGuards(g, g, g.Object(), ls, isWrite)
 }
 
 func (pc *passContext) checkCall(call callCommon, lff *lockFunctionFacts, ls *lockState) {
@@ -320,8 +375,13 @@ func (pc *passContext) postFunctionCallUpdate(call callCommon, lff *lockFunction
 		if fg.IsAlias && !aliases {
 			continue
 		}
-		r := fg.resolveCall(call.Common().Args, call.Value())
-		if s, ok := ls.unlockField(r, fg.Exclusive); !ok {
+		r := fg.Resolver.resolveCall(pc, ls, call.Common().Args, call.Value())
+		if !r.valid() {
+			// See above: this cannot be forced.
+			pc.maybeFail(call.Pos(), "field %s cannot be resolved", fieldName)
+			continue
+		}
+		if s, ok := ls.unlockField(r, fg.Exclusive); !ok && !lff.Ignore {
 			if _, ok := pc.forced[pc.positionKey(call.Pos())]; !ok && !lff.Ignore {
 				pc.maybeFail(call.Pos(), "attempt to release %s (%s), but not held (locks: %s)", fieldName, s, ls.String())
 			}
@@ -337,8 +397,8 @@ func (pc *passContext) postFunctionCallUpdate(call callCommon, lff *lockFunction
 			continue
 		}
 		// Acquire the lock per the annotation.
-		r := fg.resolveCall(call.Common().Args, call.Value())
-		if s, ok := ls.lockField(r, fg.Exclusive); !ok {
+		r := fg.Resolver.resolveCall(pc, ls, call.Common().Args, call.Value())
+		if s, ok := ls.lockField(r, fg.Exclusive); !ok && !lff.Ignore {
 			if _, ok := pc.forced[pc.positionKey(call.Pos())]; !ok && !lff.Ignore {
 				pc.maybeFail(call.Pos(), "attempt to acquire %s (%s), but already held (locks: %s)", fieldName, s, ls.String())
 			}
@@ -361,17 +421,15 @@ func exclusiveStr(exclusive bool) string {
 // instruction order).
 func (pc *passContext) checkFunctionCall(call callCommon, fn *types.Func, lff *lockFunctionFacts, ls *lockState) {
 	// Extract the "receiver" properly.
-	var rcvr ssa.Value
+	var args []ssa.Value
 	if call.Common().Method != nil {
 		// This is an interface dispatch for sync.Locker.
-		rcvr = call.Common().Value
-	} else if args := call.Common().Args; len(args) > 0 && fn.Type().(*types.Signature).Recv() != nil {
+		args = append([]ssa.Value{call.Common().Value}, call.Common().Args...)
+	} else {
 		// This matches the signature for the relevant
 		// sync.Lock/sync.Unlock functions below.
-		rcvr = args[0]
+		args = call.Common().Args
 	}
-	// Note that at this point, rcvr may be nil, but it should not match any
-	// of the function signatures below where rcvr may be used.
 
 	// Check all guards required are held. Note that this explicitly does
 	// not include aliases, hence false being passed below.
@@ -379,7 +437,7 @@ func (pc *passContext) checkFunctionCall(call callCommon, fn *types.Func, lff *l
 		if fg.IsAlias {
 			continue
 		}
-		r := fg.resolveCall(call.Common().Args, call.Value())
+		r := fg.Resolver.resolveCall(pc, ls, args, call.Value())
 		if s, ok := ls.isHeld(r, fg.Exclusive); !ok {
 			if _, ok := pc.forced[pc.positionKey(call.Pos())]; !ok && !lff.Ignore {
 				pc.maybeFail(call.Pos(), "must hold %s %s (%s) to call %s, but not held (locks: %s)", fieldName, exclusiveStr(fg.Exclusive), s, fn.Name(), ls.String())
@@ -395,15 +453,16 @@ func (pc *passContext) checkFunctionCall(call callCommon, fn *types.Func, lff *l
 
 	// Check if it's a method dispatch for something in the sync package.
 	// See: https://godoc.org/golang.org/x/tools/go/ssa#Function
-	if fn.Pkg() != nil && fn.Pkg().Name() == "sync" {
+	if fn.Pkg() != nil && fn.Pkg().Name() == "sync" && len(args) > 0 {
+		rv := makeResolvedValue(args[0], nil)
 		isExclusive := false
 		switch fn.Name() {
 		case "Lock":
 			isExclusive = true
 			fallthrough
 		case "RLock":
-			if s, ok := ls.lockField(resolvedValue{value: rcvr, valid: true}, isExclusive); !ok {
-				if _, ok := pc.forced[pc.positionKey(call.Pos())]; !ok && !lff.Ignore {
+			if s, ok := ls.lockField(rv, isExclusive); !ok && !lff.Ignore {
+				if _, ok := pc.forced[pc.positionKey(call.Pos())]; !ok {
 					// Double locking a mutex that is already locked.
 					pc.maybeFail(call.Pos(), "%s already locked (locks: %s)", s, ls.String())
 				}
@@ -412,14 +471,14 @@ func (pc *passContext) checkFunctionCall(call callCommon, fn *types.Func, lff *l
 			isExclusive = true
 			fallthrough
 		case "RUnlock":
-			if s, ok := ls.unlockField(resolvedValue{value: rcvr, valid: true}, isExclusive); !ok {
-				if _, ok := pc.forced[pc.positionKey(call.Pos())]; !ok && !lff.Ignore {
+			if s, ok := ls.unlockField(rv, isExclusive); !ok && !lff.Ignore {
+				if _, ok := pc.forced[pc.positionKey(call.Pos())]; !ok {
 					// Unlocking something that is already unlocked.
 					pc.maybeFail(call.Pos(), "%s already unlocked or locked differently (locks: %s)", s, ls.String())
 				}
 			}
 		case "DowngradeLock":
-			if s, ok := ls.downgradeField(resolvedValue{value: call.Common().Args[0], valid: true}); !ok {
+			if s, ok := ls.downgradeField(rv); !ok {
 				if _, ok := pc.forced[pc.positionKey(call.Pos())]; !ok && !lff.Ignore {
 					// Downgrading something that may not be downgraded.
 					pc.maybeFail(call.Pos(), "%s already unlocked or not exclusive (locks: %s)", s, ls.String())
@@ -497,6 +556,24 @@ type callCommon interface {
 // checkInstruction checks the legality the single instruction based on the
 // current lockState.
 func (pc *passContext) checkInstruction(inst ssa.Instruction, lff *lockFunctionFacts, ls *lockState) (*ssa.Return, *lockState) {
+	// Record any observed globals, and check for violations. The global
+	// value is not itself an instruction, but we check all referrers to
+	// see where they are consumed.
+	var stackLocal [16]*ssa.Value
+	ops := inst.Operands(stackLocal[:])
+	for _, v := range ops {
+		if v == nil {
+			continue
+		}
+		g, ok := (*v).(*ssa.Global)
+		if !ok {
+			continue
+		}
+		_, isWrite := inst.(*ssa.Store)
+		pc.checkGlobalAccess(g, ls, isWrite)
+	}
+
+	// Process the instruction.
 	switch x := inst.(type) {
 	case *ssa.Store:
 		// Record that this value is holding this other value. This is
@@ -611,7 +688,12 @@ func (pc *passContext) checkBasicBlock(fn *ssa.Function, block *ssa.BasicBlock,
 			failed := false
 			// Validate held locks.
 			for fieldName, fg := range lff.HeldOnExit {
-				r := fg.resolveStatic(fn, rv)
+				r := fg.Resolver.resolveStatic(pc, ls, fn, rv)
+				if !r.valid() {
+					// This cannot be forced, since we have no reference.
+					pc.maybeFail(rv.Pos(), "lock %s cannot be resolved", fieldName)
+					continue
+				}
 				if s, ok := rls.isHeld(r, fg.Exclusive); !ok {
 					if _, ok := pc.forced[pc.positionKey(rv.Pos())]; !ok && !lff.Ignore {
 						pc.maybeFail(rv.Pos(), "lock %s (%s) not held %s (locks: %s)", fieldName, s, exclusiveStr(fg.Exclusive), rls.String())
@@ -684,7 +766,12 @@ func (pc *passContext) checkFunction(call callCommon, fn *ssa.Function, lff *loc
 	for fieldName, fg := range lff.HeldOnEntry {
 		// The first is the method object itself so we skip that when looking
 		// for receiver/function parameters.
-		r := fg.resolveStatic(fn, call.Value())
+		r := fg.Resolver.resolveStatic(pc, ls, fn, call.Value())
+		if !r.valid() {
+			// See above: this cannot be forced.
+			pc.maybeFail(fn.Pos(), "lock %s cannot be resolved", fieldName)
+			continue
+		}
 		if s, ok := ls.lockField(r, fg.Exclusive); !ok && !lff.Ignore {
 			// This can only happen if the same value is declared
 			// multiple times, and should be caught by the earlier
@@ -710,3 +797,24 @@ func (pc *passContext) checkFunction(call callCommon, fn *ssa.Function, lff *loc
 		pc.postFunctionCallUpdate(call, lff, parent, true /* aliases */)
 	}
 }
+
+// checkInferred checks for any inferred lock annotations.
+func (pc *passContext) checkInferred() {
+	for obj, oo := range pc.observations {
+		var lgf lockGuardFacts
+		pc.pass.ImportObjectFact(obj, &lgf)
+		for other, count := range oo.counts {
+			// Is this already a guard?
+			if _, ok := lgf.GuardedBy[other.Name()]; ok {
+				continue
+			}
+			// Check to see if this field is used with a given lock
+			// held above the threshold. If yes, provide a helpful
+			// hint that this may something you wish to annotate.
+			const threshold = 0.9
+			if usage := float64(count) / float64(oo.total); usage >= threshold {
+				pc.maybeFail(obj.Pos(), "may require checklocks annotation for %s, used with lock held %2.0f%% of the time", other.Name(), usage*100)
+			}
+		}
+	}
+}

tools/checklocks/checklocks.go

@@ -30,20 +30,61 @@ import (
 
 // Analyzer is the main entrypoint.
 var Analyzer = &analysis.Analyzer{
-	Name:      "checklocks",
-	Doc:       "checks lock preconditions on functions and fields",
-	Run:       run,
-	Requires:  []*analysis.Analyzer{buildssa.Analyzer},
-	FactTypes: []analysis.Fact{(*atomicAlignment)(nil), (*lockFieldFacts)(nil), (*lockGuardFacts)(nil), (*lockFunctionFacts)(nil)},
+	Name:     "checklocks",
+	Doc:      "checks lock preconditions on functions and fields",
+	Run:      run,
+	Requires: []*analysis.Analyzer{buildssa.Analyzer},
+	FactTypes: []analysis.Fact{
+		(*atomicAlignment)(nil),
+		(*lockGuardFacts)(nil),
+		(*lockFunctionFacts)(nil),
+	},
+}
+
+// objectObservations tracks lock correlations.
+type objectObservations struct {
+	counts map[types.Object]int
+	total  int
 }
 
 // passContext is a pass with additional expected failures.
 type passContext struct {
-	pass       *analysis.Pass
-	failures   map[positionKey]*failData
-	exemptions map[positionKey]struct{}
-	forced     map[positionKey]struct{}
-	functions  map[*ssa.Function]struct{}
+	pass         *analysis.Pass
+	failures     map[positionKey]*failData
+	exemptions   map[positionKey]struct{}
+	forced       map[positionKey]struct{}
+	functions    map[*ssa.Function]struct{}
+	observations map[types.Object]*objectObservations
+}
+
+// observationsFor retrieves observations for the given object.
+func (pc *passContext) observationsFor(obj types.Object) *objectObservations {
+	if pc.observations == nil {
+		pc.observations = make(map[types.Object]*objectObservations)
+	}
+	oo, ok := pc.observations[obj]
+	if !ok {
+		oo = &objectObservations{
+			counts: make(map[types.Object]int),
+		}
+		pc.observations[obj] = oo
+	}
+	return oo
+}
+
+// forAllGlobals applies the given function to all globals.
+func (pc *passContext) forAllGlobals(fn func(ts *ast.ValueSpec)) {
+	for _, f := range pc.pass.Files {
+		for _, decl := range f.Decls {
+			d, ok := decl.(*ast.GenDecl)
+			if !ok || d.Tok != token.VAR {
+				continue
+			}
+			for _, gs := range d.Specs {
+				fn(gs.(*ast.ValueSpec))
+			}
+		}
+	}
 }
 
 // forAllTypes applies the given function over all types.
@@ -88,16 +129,17 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	pc.extractLineFailures()
 
 	// Find all struct declarations and export relevant facts.
-	pc.forAllTypes(func(ts *ast.TypeSpec) {
-		if ss, ok := ts.Type.(*ast.StructType); ok {
-			structType := pc.pass.TypesInfo.TypeOf(ts.Name).Underlying().(*types.Struct)
-			pc.exportLockFieldFacts(structType, ss)
+	pc.forAllGlobals(func(vs *ast.ValueSpec) {
+		if ss, ok := vs.Type.(*ast.StructType); ok {
+			structType := pc.pass.TypesInfo.TypeOf(vs.Type).Underlying().(*types.Struct)
+			pc.structLockGuardFacts(structType, ss)
 		}
+		pc.globalLockGuardFacts(vs)
 	})
 	pc.forAllTypes(func(ts *ast.TypeSpec) {
 		if ss, ok := ts.Type.(*ast.StructType); ok {
 			structType := pc.pass.TypesInfo.TypeOf(ts.Name).Underlying().(*types.Struct)
-			pc.exportLockGuardFacts(structType, ss)
+			pc.structLockGuardFacts(structType, ss)
 		}
 	})
 
@@ -112,7 +154,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 
 	// Find all function declarations and export relevant facts.
 	pc.forAllFunctions(func(fn *ast.FuncDecl) {
-		pc.exportFunctionFacts(fn)
+		pc.functionFacts(fn)
 	})
 
 	// Scan all code looking for invalid accesses.
@@ -144,6 +186,9 @@ func run(pass *analysis.Pass) (interface{}, error) {
 		pc.checkFunction(nil, fn, &nolff, nil, false /* force */)
 	}
 
+	// Check for inferred checklocks annotations.
+	pc.checkInferred()
+
 	// Check for expected failures.
 	pc.checkFailures()
 

tools/checklocks/state.go

@@ -24,20 +24,26 @@ import (
 	"golang.org/x/tools/go/ssa"
 )
 
+// lockInfo describes a held lock.
+type lockInfo struct {
+	exclusive bool
+	object    types.Object
+}
+
 // lockState tracks the locking state and aliases.
 type lockState struct {
 	// lockedMutexes is used to track which mutexes in a given struct are
 	// currently locked. Note that most of the heavy lifting is done by
-	// valueAsString below, which maps to specific structure fields, etc.
+	// valueAndObject below, which maps to specific structure fields, etc.
 	//
 	// The value indicates whether this is an exclusive lock.
-	lockedMutexes map[string]bool
+	lockedMutexes map[string]lockInfo
 
 	// stored stores values that have been stored in memory, bound to
 	// FreeVars or passed as Parameterse.
 	stored map[ssa.Value]ssa.Value
 
-	// used is a temporary map, used only for valueAsString. It prevents
+	// used is a temporary map, used only for valueAndObject. It prevents
 	// multiple use of the same memory location.
 	used map[ssa.Value]struct{}
 
@@ -53,7 +59,7 @@ type lockState struct {
 func newLockState() *lockState {
 	refs := int32(1) // Not shared.
 	return &lockState{
-		lockedMutexes: make(map[string]bool),
+		lockedMutexes: make(map[string]lockInfo),
 		used:          make(map[ssa.Value]struct{}),
 		stored:        make(map[ssa.Value]ssa.Value),
 		defers:        make([]*ssa.Defer, 0),
@@ -81,7 +87,7 @@ func (l *lockState) fork() *lockState {
 func (l *lockState) modify() {
 	if atomic.LoadInt32(l.refs) > 1 {
 		// Copy the lockedMutexes.
-		lm := make(map[string]bool)
+		lm := make(map[string]lockInfo)
 		for k, v := range l.lockedMutexes {
 			lm[k] = v
 		}
@@ -110,17 +116,19 @@ func (l *lockState) modify() {
 }
 
 // isHeld indicates whether the field is held is not.
+//
+// Precondition: rv must be valid.
 func (l *lockState) isHeld(rv resolvedValue, exclusiveRequired bool) (string, bool) {
-	if !rv.valid {
-		return rv.valueAsString(l), false
+	if !rv.valid() {
+		panic("invalid resolvedValue passed to isHeld")
 	}
-	s := rv.valueAsString(l)
-	isExclusive, ok := l.lockedMutexes[s]
+	s, _ := rv.valueAndObject(l)
+	info, ok := l.lockedMutexes[s]
 	if !ok {
 		return s, false
 	}
 	// Accept a weaker lock if exclusiveRequired is false.
-	if exclusiveRequired && !isExclusive {
+	if exclusiveRequired && !info.exclusive {
 		return s, false
 	}
 	return s, true
@@ -129,32 +137,39 @@ func (l *lockState) isHeld(rv resolvedValue, exclusiveRequired bool) (string, bo
 // lockField locks the given field.
 //
 // If false is returned, the field was already locked.
+//
+// Precondition: rv must be valid.
 func (l *lockState) lockField(rv resolvedValue, exclusive bool) (string, bool) {
-	if !rv.valid {
-		return rv.valueAsString(l), false
+	if !rv.valid() {
+		panic("invalid resolvedValue passed to isHeld")
 	}
-	s := rv.valueAsString(l)
+	s, obj := rv.valueAndObject(l)
 	if _, ok := l.lockedMutexes[s]; ok {
 		return s, false
 	}
 	l.modify()
-	l.lockedMutexes[s] = exclusive
+	l.lockedMutexes[s] = lockInfo{
+		exclusive: exclusive,
+		object:    obj,
+	}
 	return s, true
 }
 
 // unlockField unlocks the given field.
 //
 // If false is returned, the field was not locked.
+//
+// Precondition: rv must be valid.
 func (l *lockState) unlockField(rv resolvedValue, exclusive bool) (string, bool) {
-	if !rv.valid {
-		return rv.valueAsString(l), false
+	if !rv.valid() {
+		panic("invalid resolvedValue passed to isHeld")
 	}
-	s := rv.valueAsString(l)
-	wasExclusive, ok := l.lockedMutexes[s]
+	s, _ := rv.valueAndObject(l)
+	info, ok := l.lockedMutexes[s]
 	if !ok {
 		return s, false
 	}
-	if wasExclusive != exclusive {
+	if info.exclusive != exclusive {
 		return s, false
 	}
 	l.modify()
@@ -165,20 +180,23 @@ func (l *lockState) unlockField(rv resolvedValue, exclusive bool) (string, bool)
 // downgradeField downgrades the given field.
 //
 // If false was returned, the field was not downgraded.
+//
+// Precondition: rv must be valid.
 func (l *lockState) downgradeField(rv resolvedValue) (string, bool) {
-	if !rv.valid {
-		return rv.valueAsString(l), false
+	if !rv.valid() {
+		panic("invalid resolvedValue passed to isHeld")
 	}
-	s := rv.valueAsString(l)
-	wasExclusive, ok := l.lockedMutexes[s]
+	s, _ := rv.valueAndObject(l)
+	info, ok := l.lockedMutexes[s]
 	if !ok {
 		return s, false
 	}
-	if !wasExclusive {
+	if !info.exclusive {
 		return s, false
 	}
 	l.modify()
-	l.lockedMutexes[s] = false // Downgraded.
+	info.exclusive = false
+	l.lockedMutexes[s] = info // Downgraded.
 	return s, true
 }
 
@@ -190,13 +208,13 @@ func (l *lockState) store(addr ssa.Value, v ssa.Value) {
 
 // isSubset indicates other holds all the locks held by l.
 func (l *lockState) isSubset(other *lockState) bool {
-	for k, isExclusive := range l.lockedMutexes {
-		otherExclusive, otherOk := other.lockedMutexes[k]
+	for k, info := range l.lockedMutexes {
+		otherInfo, otherOk := other.lockedMutexes[k]
 		if !otherOk {
 			return false
 		}
 		// Accept weaker locks as a subset.
-		if isExclusive && !otherExclusive {
+		if info.exclusive && !otherInfo.exclusive {
 			return false
 		}
 	}
@@ -218,25 +236,26 @@ type elemType interface {
 	Elem() types.Type
 }
 
-// valueAsString returns a string for a given value.
+// valueAndObject returns a string for a given value, along with a source level
+// object (if available and relevant).
 //
 // This decomposes the value into the simplest possible representation in terms
 // of parameters, free variables and globals. During resolution, stored values
 // may be transferred, as well as bound free variables.
 //
 // Nil may not be passed here.
-func (l *lockState) valueAsString(v ssa.Value) string {
+func (l *lockState) valueAndObject(v ssa.Value) (string, types.Object) {
 	switch x := v.(type) {
 	case *ssa.Parameter:
 		// Was this provided as a paramter for a local anonymous
 		// function invocation?
 		v, ok := l.stored[x]
 		if ok {
-			return l.valueAsString(v)
+			return l.valueAndObject(v)
 		}
-		return fmt.Sprintf("{param:%s}", x.Name())
+		return fmt.Sprintf("{param:%s}", x.Name()), x.Object()
 	case *ssa.Global:
-		return fmt.Sprintf("{global:%s}", x.Name())
+		return fmt.Sprintf("{global:%s}", x.Name()), x.Object()
 	case *ssa.FreeVar:
 		// Attempt to resolve this, in case we are being invoked in a
 		// scope where all the variables are bound.
@@ -247,16 +266,18 @@ func (l *lockState) valueAsString(v ssa.Value) string {
 			// may map to the same FreeVar, which we can check.
 			stored, ok := l.stored[v]
 			if ok {
-				return l.valueAsString(stored)
+				return l.valueAndObject(stored)
 			}
 		}
-		return fmt.Sprintf("{freevar:%s}", x.Name())
+		// FreeVar does not have a corresponding source-level object
+		// that we can return here.
+		return fmt.Sprintf("{freevar:%s}", x.Name()), nil
 	case *ssa.Convert:
 		// Just disregard conversion.
-		return l.valueAsString(x.X)
+		return l.valueAndObject(x.X)
 	case *ssa.ChangeType:
 		// Ditto, disregard.
-		return l.valueAsString(x.X)
+		return l.valueAndObject(x.X)
 	case *ssa.UnOp:
 		if x.Op != token.MUL {
 			break
@@ -264,7 +285,7 @@ func (l *lockState) valueAsString(v ssa.Value) string {
 		// Is this loading a free variable? If yes, then this can be
 		// resolved in the original isAlias function.
 		if fv, ok := x.X.(*ssa.FreeVar); ok {
-			return l.valueAsString(fv)
+			return l.valueAndObject(fv)
 		}
 		// Should be try to resolve via a memory address? This needs to
 		// be done since a memory location can hold its own value.
@@ -275,12 +296,13 @@ func (l *lockState) valueAsString(v ssa.Value) string {
 			if ok {
 				l.used[x.X] = struct{}{}
 				defer func() { delete(l.used, x.X) }()
-				return l.valueAsString(v)
+				return l.valueAndObject(v)
 			}
 		}
 		// x.X.Type is pointer. We must construct this type
 		// dynamically, since the ssa.Value could be synthetic.
-		return fmt.Sprintf("*(%s)", l.valueAsString(x.X))
+		s, obj := l.valueAndObject(x.X)
+		return fmt.Sprintf("*(%s)", s), obj
 	case *ssa.Field:
 		structType, ok := resolveStruct(x.X.Type())
 		if !ok {
@@ -288,7 +310,8 @@ func (l *lockState) valueAsString(v ssa.Value) string {
 			panic(fmt.Sprintf("structType not available for struct: %#v", x.X))
 		}
 		fieldObj := structType.Field(x.Field)
-		return fmt.Sprintf("%s.%s", l.valueAsString(x.X), fieldObj.Name())
+		s, _ := l.valueAndObject(x.X)
+		return fmt.Sprintf("%s.%s", s, fieldObj.Name()), fieldObj
 	case *ssa.FieldAddr:
 		structType, ok := resolveStruct(x.X.Type())
 		if !ok {
@@ -296,22 +319,30 @@ func (l *lockState) valueAsString(v ssa.Value) string {
 			panic(fmt.Sprintf("structType not available for struct: %#v", x.X))
 		}
 		fieldObj := structType.Field(x.Field)
-		return fmt.Sprintf("&(%s.%s)", l.valueAsString(x.X), fieldObj.Name())
+		s, _ := l.valueAndObject(x.X)
+		return fmt.Sprintf("&(%s.%s)", s, fieldObj.Name()), fieldObj
 	case *ssa.Index:
-		return fmt.Sprintf("%s[%s]", l.valueAsString(x.X), l.valueAsString(x.Index))
+		s, _ := l.valueAndObject(x.X)
+		i, _ := l.valueAndObject(x.Index)
+		return fmt.Sprintf("%s[%s]", s, i), nil
 	case *ssa.IndexAddr:
-		return fmt.Sprintf("&(%s[%s])", l.valueAsString(x.X), l.valueAsString(x.Index))
+		s, _ := l.valueAndObject(x.X)
+		i, _ := l.valueAndObject(x.Index)
+		return fmt.Sprintf("&(%s[%s])", s, i), nil
 	case *ssa.Lookup:
-		return fmt.Sprintf("%s[%s]", l.valueAsString(x.X), l.valueAsString(x.Index))
+		s, _ := l.valueAndObject(x.X)
+		i, _ := l.valueAndObject(x.Index)
+		return fmt.Sprintf("%s[%s]", s, i), nil
 	case *ssa.Extract:
-		return fmt.Sprintf("%s[%d]", l.valueAsString(x.Tuple), x.Index)
+		s, _ := l.valueAndObject(x.Tuple)
+		return fmt.Sprintf("%s[%d]", s, x.Index), nil
 	}
 
 	// In the case of any other type (e.g. this may be an alloc, a return
 	// value, etc.), just return the literal pointer value to the Value.
 	// This will be unique within the ssa graph, and so if two values are
 	// equal, they are from the same type.
-	return fmt.Sprintf("{%T:%p}", v, v)
+	return fmt.Sprintf("{%T:%p}", v, v), nil
 }
 
 // String returns the full lock state.
@@ -320,9 +351,9 @@ func (l *lockState) String() string {
 		return "no locks held"
 	}
 	keys := make([]string, 0, len(l.lockedMutexes))
-	for k, exclusive := range l.lockedMutexes {
+	for k, info := range l.lockedMutexes {
 		// Include the exclusive status of each lock.
-		keys = append(keys, fmt.Sprintf("%s %s", k, exclusiveStr(exclusive)))
+		keys = append(keys, fmt.Sprintf("%s %s", k, exclusiveStr(info.exclusive)))
 	}
 	return strings.Join(keys, ",")
 }

tools/checklocks/test/BUILD

@@ -13,7 +13,9 @@ go_library(
         "branches.go",
         "closures.go",
         "defer.go",
+        "globals.go",
         "incompat.go",
+        "inferred.go",
         "locker.go",
         "methods.go",
         "parameters.go",
@@ -21,4 +23,8 @@ go_library(
         "rwmutex.go",
         "test.go",
     ],
+    # This ensures that there are no dependencies, since we want to explicitly
+    # control expected failures for analysis.
+    marshal = False,
+    stateify = False,
 )

tools/checklocks/test/globals.go

@@ -0,0 +1,85 @@
+// Copyright 2020 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"
+)
+
+var (
+	globalMu   sync.Mutex
+	globalRWMu sync.RWMutex
+)
+
+var globalStruct struct {
+	mu sync.Mutex
+	// +checklocks:mu
+	guardedField int
+}
+
+var otherStruct struct {
+	// +checklocks:globalMu
+	guardedField1 int
+	// +checklocks:globalRWMu
+	guardedField2 int
+	// +checklocks:globalStruct.mu
+	guardedField3 int
+}
+
+func testGlobalValid() {
+	globalMu.Lock()
+	otherStruct.guardedField1 = 1
+	globalMu.Unlock()
+
+	globalRWMu.Lock()
+	otherStruct.guardedField2 = 1
+	globalRWMu.Unlock()
+
+	globalRWMu.RLock()
+	_ = otherStruct.guardedField2
+	globalRWMu.RUnlock()
+
+	globalStruct.mu.Lock()
+	globalStruct.guardedField = 1
+	otherStruct.guardedField3 = 1
+	globalStruct.mu.Unlock()
+}
+
+// +checklocks:globalStruct.mu
+func testGlobalValidPreconditions0() {
+	globalStruct.guardedField = 1
+}
+
+// +checklocks:globalMu
+func testGlobalValidPreconditions1() {
+	otherStruct.guardedField1 = 1
+}
+
+// +checklocks:globalRWMu
+func testGlobalValidPreconditions2() {
+	otherStruct.guardedField2 = 1
+}
+
+// +checklocks:globalStruct.mu
+func testGlobalValidPreconditions3() {
+	otherStruct.guardedField3 = 1
+}
+
+func testGlobalInvalid() {
+	globalStruct.guardedField = 1 // +checklocksfail
+	otherStruct.guardedField1 = 1 // +checklocksfail
+	otherStruct.guardedField2 = 1 // +checklocksfail
+	otherStruct.guardedField3 = 1 // +checklocksfail
+}

tools/checklocks/test/inferred.go

@@ -0,0 +1,35 @@
+// Copyright 2020 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 inferredStruct struct {
+	mu             sync.Mutex
+	guardedField   int // +checklocksfail
+	unguardedField int
+}
+
+func testInferredPositive(tc *inferredStruct) {
+	tc.mu.Lock()
+	tc.guardedField = 1
+	tc.mu.Unlock()
+}
+
+func testInferredNegative(tc *inferredStruct) {
+	tc.unguardedField = 1
+}

tools/checklocks/test/methods.go

@@ -103,7 +103,7 @@ type testMethodsWithEmbedded struct {
 
 	// +checklocks:mu
 	guardedField int
-	p            *testMethodsWithParameters
+	p            *testMethodsWithParameters // +checklocksignore: Inferred as protected by mu.
 }
 
 // +checklocks:t.mu

tools/checklocks/test/test.go

@@ -51,7 +51,7 @@ type twoLocksStruct struct {
 // twoLocksDoubleGuardStruct has two locks and a single field with two guards.
 type twoLocksDoubleGuardStruct struct {
 	mu       sync.Mutex
-	secondMu sync.Mutex
+	secondMu sync.Mutex // +checklocksignore: mu is inferred as requisite.
 	// +checklocks:mu
 	// +checklocks:secondMu
 	doubleGuardedField int