[checklocks] Support iterators

[fancycode]

Description

Currently locked access to struct members inside a loop over an iterator is not detected correctly.

Example:

package main

import (
	"strings"
	"sync"
)

type Entries struct {
	mu sync.Mutex
	// +checklocks:mu
	entries []string
}

func (e *Entries) Add(s string) {
	e.mu.Lock()
	defer e.mu.Unlock()

	for item := range strings.SplitSeq(s, " ") {
		e.entries = append(e.entries, item)
	}
}

Error:

go vet -vettool=$HOME/go/bin/checklocks iter_seq_checklocks.go 
# command-line-arguments
./iter_seq_checklocks.go:19:24: invalid field access, mu (&({freevar:e}.mu)) must be locked when accessing entries (locks: no locks held)
./iter_seq_checklocks.go:19:5: invalid field access, mu (&({freevar:e}.mu)) must be locked when accessing entries (locks: no locks held)

This works if the lock is acquired inside the loop.

func (e *Entries) Add(s string) {
	for item := range strings.SplitSeq(s, " ") {
		e.mu.Lock()
		e.entries = append(e.entries, item)
		e.mu.Unlock()
	}
}

Is this feature related to a specific bug?

No response

Do you have a specific solution in mind?

No response

[uzairnawaz]

Hello! We (me and @nicolasgarza) are students at UT Austin working on issues for our virtualization class. Could we get this ticket assigned to us? Thank you!

[uzairnawaz]

This may require a more complex implementation than I originally expected. I likely will not be able to fully implement this enhancement, but I just wanted to share some thoughts:

Ideally we would like the closure created by the iterator to be analyzed with the current lock state since it is constructed and called immediately.

This analysis path does already exist for immediately applied closures (see the ssa.MakeClosure case in checkCall), but for iterator closures, the corresponding call object looks like a regular dynamic dispatch function call (falls into category (d) below). This means that at this point in the code path we can't statically determine which function the call instruction is trying to run, so we can't perform the analysis with the current lock state.

For reference I'm referring to the four possible types of calls described in checkCall:

// "call" mode: when Method is nil (!IsInvoke), a CallCommon represents an ordinary // function call of the value in Value, which may be a *Builtin, a *Function or any // other value of kind 'func'. // // Value may be one of: // (a) a *Function, indicating a statically dispatched call // to a package-level function, an anonymous function, or // a method of a named type. // // (b) a *MakeClosure, indicating an immediately applied // function literal with free variables. // // (c) a *Builtin, indicating a statically dispatched call // to a built-in function. // // (d) any other value, indicating a dynamically dispatched // function call.

This feature is something that should definitely be possible to implement, but the buildssa package that is generating these instructions just isn't detecting the relationship between the MakeClosure instruction to construct the closure and the call instruction that follows it (seemingly by design).

I don't see an obvious way to handle this issue, so I would love to hear anyone else's thoughts!