go/types, types2: guard Named.underlying with Named.mu

It appears that CL 695977 introduced a data race on Named.underlying.
This fixes that race by specifying a new namedState called underlying,
which, perhaps unsurprisingly, signals that Named.underlying is populated.

Unfortunately, the underlying namedState is independent of the complete
namedState (unsurprising since methods and the underlying type are not related).

Hence, they cannot be ordered and thus do not fit the current integer-based
state representation. To account for combinations of states, we introduce a
bit set representation for namedState instead. The namedState field is also
renamed to stateMask to reflect this new representation.

Methods that operate on the stateMask are adjusted and exposition is added
throughout.

Fixes #75963

Change-Id: Icfa188ea2fa7916804c06f80668e99176bf4e978
Reviewed-on: https://go-review.googlesource.com/c/go/+/712720
Auto-Submit: Mark Freeman <markfreeman@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Griesemer <gri@google.com>

src/cmd/compile/internal/types2/named.go

@@ -47,10 +47,8 @@ import (
 // soon.
 //
 // We achieve this by tracking state with an atomic state variable, and
-// guarding potentially concurrent calculations with a mutex. At any point in
+// guarding potentially concurrent calculations with a mutex. See [stateMask]
-// time this state variable determines which data on N may be accessed. As
+// for details.
-// state monotonically progresses, any data available at state M may be
-// accessed without acquiring the mutex at state N, provided N >= M.
 //
 // GLOSSARY: Here are a few terms used in this file to describe Named types:
 //  - We say that a Named type is "instantiated" if it has been constructed by
@@ -111,13 +109,13 @@ type Named struct {
 	allowNilRHS        bool // same as below, as well as briefly during checking of a type declaration
 	allowNilUnderlying bool // may be true from creation via [NewNamed] until [Named.SetUnderlying]
 
-	underlying Type      // underlying type, or nil
+	inst *instance // information for instantiated types; nil otherwise
-	inst       *instance // information for instantiated types; nil otherwise
 
-	mu      sync.Mutex     // guards all fields below
+	mu         sync.Mutex     // guards all fields below
-	state_  uint32         // the current state of this type; must only be accessed atomically
+	state_     uint32         // the current state of this type; must only be accessed atomically or when mu is held
-	fromRHS Type           // the declaration RHS this type is derived from
+	fromRHS    Type           // the declaration RHS this type is derived from
-	tparams *TypeParamList // type parameters, or nil
+	tparams    *TypeParamList // type parameters, or nil
+	underlying Type           // underlying type, or nil
 
 	// methods declared for this type (not the method set of this type)
 	// Signatures are type-checked lazily.
@@ -139,15 +137,43 @@ type instance struct {
 	ctxt            *Context  // local Context; set to nil after full expansion
 }
 
-// namedState represents the possible states that a named type may assume.
+// stateMask represents each state in the lifecycle of a named type.
-type namedState uint32
+//
+// Each named type begins in the unresolved state. A named type may transition to a new state
+// according to the below diagram:
+//
+//	unresolved
+//	loaded
+//	resolved
+//	└── complete
+//	└── underlying
+//
+// That is, descent down the tree is mostly linear (unresolved through resolved), except upon
+// reaching the leaves (complete and underlying). A type may occupy any combination of the
+// leaf states at once (they are independent states).
+//
+// To represent this independence, the set of active states is represented with a bit set. State
+// transitions are monotonic. Once a state bit is set, it remains set.
+//
+// The above constraints significantly narrow the possible bit sets for a named type. With bits
+// set left-to-right, they are:
+//
+//	0000 | unresolved
+//	1000 | loaded
+//	1100 | resolved, which implies loaded
+//	1110 | completed, which implies resolved (which in turn implies loaded)
+//	1101 | underlying, which implies resolved ...
+//	1111 | both completed and underlying which implies resolved ...
+//
+// To read the state of a named type, use [Named.stateHas]; to write, use [Named.setState].
+type stateMask uint32
 
-// Note: the order of states is relevant
 const (
-	unresolved namedState = iota // type parameters, RHS, underlying, and methods might be unavailable
+	// before resolved, type parameters, RHS, underlying, and methods might be unavailable
-	resolved                     // resolve has run; methods might be unexpanded (for instances)
+	resolved   stateMask = 1 << iota // methods might be unexpanded (for instances)
-	loaded                       // loader has run; constraints might be unexpanded (for generic types)
+	complete                         // methods are all expanded (for instances)
-	complete                     // all data is known
+	loaded                           // methods are available, but constraints might be unexpanded (for generic types)
+	underlying                       // underlying type is available
 )
 
 // NewNamed returns a new named type for the given type name, underlying type, and associated methods.
@@ -188,7 +214,7 @@ func NewNamed(obj *TypeName, underlying Type, methods []*Func) *Named {
 // All others:
 // Effectively, nothing happens.
 func (n *Named) resolve() *Named {
-	if n.state() >= resolved { // avoid locking below
+	if n.stateHas(resolved | loaded) { // avoid locking below
 		return n
 	}
 
@@ -197,10 +223,14 @@ func (n *Named) resolve() *Named {
 	n.mu.Lock()
 	defer n.mu.Unlock()
 
-	if n.state() >= resolved {
+	// only atomic for consistency; we are holding the mutex
+	if n.stateHas(resolved | loaded) {
 		return n
 	}
 
+	// underlying comes after resolving, do not set it
+	defer (func() { assert(!n.stateHas(underlying)) })()
+
 	if n.inst != nil {
 		assert(n.fromRHS == nil) // instantiated types are not declared types
 		assert(n.loader == nil)  // cannot import an instantiation
@@ -212,7 +242,7 @@ func (n *Named) resolve() *Named {
 		n.tparams = orig.tparams
 
 		if len(orig.methods) == 0 {
-			n.setState(complete) // nothing further to do
+			n.setState(resolved | complete) // nothing further to do
 			n.inst.ctxt = nil
 		} else {
 			n.setState(resolved)
@@ -239,27 +269,24 @@ func (n *Named) resolve() *Named {
 		n.methods = methods
 
 		n.setState(loaded) // avoid deadlock calling delayed functions
-
 		for _, f := range delayed {
 			f()
 		}
 	}
 
-	assert(n.fromRHS != nil || n.allowNilRHS)
+	n.setState(resolved | complete)
-	assert(n.underlying == nil) // underlying comes after resolving
-	n.setState(complete)
 	return n
 }
 
-// state atomically accesses the current state of the receiver.
+// stateHas atomically determines whether the current state includes any active bit in sm.
-func (n *Named) state() namedState {
+func (n *Named) stateHas(sm stateMask) bool {
-	return namedState(atomic.LoadUint32(&n.state_))
+	return atomic.LoadUint32(&n.state_)&uint32(sm) != 0
 }
 
-// setState atomically stores the given state for n.
+// setState atomically sets the current state to include each active bit in sm.
 // Must only be called while holding n.mu.
-func (n *Named) setState(state namedState) {
+func (n *Named) setState(sm stateMask) {
-	atomic.StoreUint32(&n.state_, uint32(state))
+	atomic.OrUint32(&n.state_, uint32(sm))
 }
 
 // newNamed is like NewNamed but with a *Checker receiver.
@@ -369,7 +396,7 @@ func (t *Named) NumMethods() int {
 func (t *Named) Method(i int) *Func {
 	t.resolve()
 
-	if t.state() >= complete {
+	if t.stateHas(complete) {
 		return t.methods[i]
 	}
 
@@ -461,20 +488,25 @@ func (t *Named) expandMethod(i int) *Func {
 
 // SetUnderlying sets the underlying type and marks t as complete.
 // t must not have type arguments.
-func (t *Named) SetUnderlying(underlying Type) {
+func (t *Named) SetUnderlying(u Type) {
 	assert(t.inst == nil)
-	if underlying == nil {
+	if u == nil {
 		panic("underlying type must not be nil")
 	}
-	if asNamed(underlying) != nil {
+	if asNamed(u) != nil {
 		panic("underlying type must not be *Named")
 	}
-	// Invariant: Presence of underlying type implies it was resolved.
+	// be careful to uphold the state invariants
-	t.fromRHS = underlying
+	t.mu.Lock()
+	defer t.mu.Unlock()
+
+	t.fromRHS = u
 	t.allowNilRHS = false
-	t.resolve()
+	t.setState(resolved | complete) // TODO(markfreeman): Why complete?
-	t.underlying = underlying
+
+	t.underlying = u
 	t.allowNilUnderlying = false
+	t.setState(underlying)
 }
 
 // AddMethod adds method m unless it is already in the method list.
@@ -553,9 +585,10 @@ func (t *Named) String() string { return TypeString(t, nil) }
 // cycle is found, the result is Typ[Invalid]; if n.check != nil, the
 // cycle is also reported.
 func (n *Named) under() Type {
-	assert(n.state() >= resolved)
+	assert(n.stateHas(resolved))
 
-	if n.underlying != nil {
+	// optimization for likely case
+	if n.stateHas(underlying) {
 		return n.underlying
 	}
 
@@ -569,19 +602,34 @@ func (n *Named) under() Type {
 		switch t := rhs.(type) {
 		case nil:
 			u = Typ[Invalid]
+
 		case *Alias:
 			rhs = unalias(t)
+
 		case *Named:
 			if i, ok := seen[t]; ok {
 				n.check.cycleError(path[i:], firstInSrc(path[i:]))
 				u = Typ[Invalid]
 				break
 			}
+
+			// acquire the lock without checking; performance is probably fine
+			t.resolve()
+			t.mu.Lock()
+			defer t.mu.Unlock()
+
+			// t.underlying might have been set while we were locking
+			if t.stateHas(underlying) {
+				u = t.underlying
+				break
+			}
+
 			seen[t] = len(seen)
 			path = append(path, t.obj)
-			t.resolve()
+
 			assert(t.fromRHS != nil || t.allowNilRHS)
 			rhs = t.fromRHS
+
 		default:
 			u = rhs // any type literal works
 		}
@@ -590,6 +638,7 @@ func (n *Named) under() Type {
 	// go back up the chain
 	for t := range seen {
 		t.underlying = u
+		t.setState(underlying)
 	}
 
 	return u
@@ -659,7 +708,8 @@ func (n *Named) expandRHS() (rhs Type) {
 		}()
 	}
 
-	assert(n.state() == unresolved)
+	assert(!n.stateHas(resolved))
+	assert(n.inst.orig.stateHas(resolved | loaded))
 
 	if n.inst.ctxt == nil {
 		n.inst.ctxt = NewContext()
@@ -668,9 +718,6 @@ func (n *Named) expandRHS() (rhs Type) {
 	ctxt := n.inst.ctxt
 	orig := n.inst.orig
 
-	assert(orig.state() >= resolved)
-	assert(orig.fromRHS != nil)
-
 	targs := n.inst.targs
 	tpars := orig.tparams
 

src/go/types/named.go

@@ -50,10 +50,8 @@ import (
 // soon.
 //
 // We achieve this by tracking state with an atomic state variable, and
-// guarding potentially concurrent calculations with a mutex. At any point in
+// guarding potentially concurrent calculations with a mutex. See [stateMask]
-// time this state variable determines which data on N may be accessed. As
+// for details.
-// state monotonically progresses, any data available at state M may be
-// accessed without acquiring the mutex at state N, provided N >= M.
 //
 // GLOSSARY: Here are a few terms used in this file to describe Named types:
 //  - We say that a Named type is "instantiated" if it has been constructed by
@@ -114,13 +112,13 @@ type Named struct {
 	allowNilRHS        bool // same as below, as well as briefly during checking of a type declaration
 	allowNilUnderlying bool // may be true from creation via [NewNamed] until [Named.SetUnderlying]
 
-	underlying Type      // underlying type, or nil
+	inst *instance // information for instantiated types; nil otherwise
-	inst       *instance // information for instantiated types; nil otherwise
 
-	mu      sync.Mutex     // guards all fields below
+	mu         sync.Mutex     // guards all fields below
-	state_  uint32         // the current state of this type; must only be accessed atomically
+	state_     uint32         // the current state of this type; must only be accessed atomically or when mu is held
-	fromRHS Type           // the declaration RHS this type is derived from
+	fromRHS    Type           // the declaration RHS this type is derived from
-	tparams *TypeParamList // type parameters, or nil
+	tparams    *TypeParamList // type parameters, or nil
+	underlying Type           // underlying type, or nil
 
 	// methods declared for this type (not the method set of this type)
 	// Signatures are type-checked lazily.
@@ -142,15 +140,43 @@ type instance struct {
 	ctxt            *Context  // local Context; set to nil after full expansion
 }
 
-// namedState represents the possible states that a named type may assume.
+// stateMask represents each state in the lifecycle of a named type.
-type namedState uint32
+//
+// Each named type begins in the unresolved state. A named type may transition to a new state
+// according to the below diagram:
+//
+//	unresolved
+//	loaded
+//	resolved
+//	└── complete
+//	└── underlying
+//
+// That is, descent down the tree is mostly linear (unresolved through resolved), except upon
+// reaching the leaves (complete and underlying). A type may occupy any combination of the
+// leaf states at once (they are independent states).
+//
+// To represent this independence, the set of active states is represented with a bit set. State
+// transitions are monotonic. Once a state bit is set, it remains set.
+//
+// The above constraints significantly narrow the possible bit sets for a named type. With bits
+// set left-to-right, they are:
+//
+//	0000 | unresolved
+//	1000 | loaded
+//	1100 | resolved, which implies loaded
+//	1110 | completed, which implies resolved (which in turn implies loaded)
+//	1101 | underlying, which implies resolved ...
+//	1111 | both completed and underlying which implies resolved ...
+//
+// To read the state of a named type, use [Named.stateHas]; to write, use [Named.setState].
+type stateMask uint32
 
-// Note: the order of states is relevant
 const (
-	unresolved namedState = iota // type parameters, RHS, underlying, and methods might be unavailable
+	// before resolved, type parameters, RHS, underlying, and methods might be unavailable
-	resolved                     // resolve has run; methods might be unexpanded (for instances)
+	resolved   stateMask = 1 << iota // methods might be unexpanded (for instances)
-	loaded                       // loader has run; constraints might be unexpanded (for generic types)
+	complete                         // methods are all expanded (for instances)
-	complete                     // all data is known
+	loaded                           // methods are available, but constraints might be unexpanded (for generic types)
+	underlying                       // underlying type is available
 )
 
 // NewNamed returns a new named type for the given type name, underlying type, and associated methods.
@@ -191,7 +217,7 @@ func NewNamed(obj *TypeName, underlying Type, methods []*Func) *Named {
 // All others:
 // Effectively, nothing happens.
 func (n *Named) resolve() *Named {
-	if n.state() >= resolved { // avoid locking below
+	if n.stateHas(resolved | loaded) { // avoid locking below
 		return n
 	}
 
@@ -200,10 +226,14 @@ func (n *Named) resolve() *Named {
 	n.mu.Lock()
 	defer n.mu.Unlock()
 
-	if n.state() >= resolved {
+	// only atomic for consistency; we are holding the mutex
+	if n.stateHas(resolved | loaded) {
 		return n
 	}
 
+	// underlying comes after resolving, do not set it
+	defer (func() { assert(!n.stateHas(underlying)) })()
+
 	if n.inst != nil {
 		assert(n.fromRHS == nil) // instantiated types are not declared types
 		assert(n.loader == nil)  // cannot import an instantiation
@@ -215,7 +245,7 @@ func (n *Named) resolve() *Named {
 		n.tparams = orig.tparams
 
 		if len(orig.methods) == 0 {
-			n.setState(complete) // nothing further to do
+			n.setState(resolved | complete) // nothing further to do
 			n.inst.ctxt = nil
 		} else {
 			n.setState(resolved)
@@ -242,27 +272,24 @@ func (n *Named) resolve() *Named {
 		n.methods = methods
 
 		n.setState(loaded) // avoid deadlock calling delayed functions
-
 		for _, f := range delayed {
 			f()
 		}
 	}
 
-	assert(n.fromRHS != nil || n.allowNilRHS)
+	n.setState(resolved | complete)
-	assert(n.underlying == nil) // underlying comes after resolving
-	n.setState(complete)
 	return n
 }
 
-// state atomically accesses the current state of the receiver.
+// stateHas atomically determines whether the current state includes any active bit in sm.
-func (n *Named) state() namedState {
+func (n *Named) stateHas(sm stateMask) bool {
-	return namedState(atomic.LoadUint32(&n.state_))
+	return atomic.LoadUint32(&n.state_)&uint32(sm) != 0
 }
 
-// setState atomically stores the given state for n.
+// setState atomically sets the current state to include each active bit in sm.
 // Must only be called while holding n.mu.
-func (n *Named) setState(state namedState) {
+func (n *Named) setState(sm stateMask) {
-	atomic.StoreUint32(&n.state_, uint32(state))
+	atomic.OrUint32(&n.state_, uint32(sm))
 }
 
 // newNamed is like NewNamed but with a *Checker receiver.
@@ -372,7 +399,7 @@ func (t *Named) NumMethods() int {
 func (t *Named) Method(i int) *Func {
 	t.resolve()
 
-	if t.state() >= complete {
+	if t.stateHas(complete) {
 		return t.methods[i]
 	}
 
@@ -464,20 +491,25 @@ func (t *Named) expandMethod(i int) *Func {
 
 // SetUnderlying sets the underlying type and marks t as complete.
 // t must not have type arguments.
-func (t *Named) SetUnderlying(underlying Type) {
+func (t *Named) SetUnderlying(u Type) {
 	assert(t.inst == nil)
-	if underlying == nil {
+	if u == nil {
 		panic("underlying type must not be nil")
 	}
-	if asNamed(underlying) != nil {
+	if asNamed(u) != nil {
 		panic("underlying type must not be *Named")
 	}
-	// Invariant: Presence of underlying type implies it was resolved.
+	// be careful to uphold the state invariants
-	t.fromRHS = underlying
+	t.mu.Lock()
+	defer t.mu.Unlock()
+
+	t.fromRHS = u
 	t.allowNilRHS = false
-	t.resolve()
+	t.setState(resolved | complete) // TODO(markfreeman): Why complete?
-	t.underlying = underlying
+
+	t.underlying = u
 	t.allowNilUnderlying = false
+	t.setState(underlying)
 }
 
 // AddMethod adds method m unless it is already in the method list.
@@ -556,9 +588,10 @@ func (t *Named) String() string { return TypeString(t, nil) }
 // cycle is found, the result is Typ[Invalid]; if n.check != nil, the
 // cycle is also reported.
 func (n *Named) under() Type {
-	assert(n.state() >= resolved)
+	assert(n.stateHas(resolved))
 
-	if n.underlying != nil {
+	// optimization for likely case
+	if n.stateHas(underlying) {
 		return n.underlying
 	}
 
@@ -572,19 +605,34 @@ func (n *Named) under() Type {
 		switch t := rhs.(type) {
 		case nil:
 			u = Typ[Invalid]
+
 		case *Alias:
 			rhs = unalias(t)
+
 		case *Named:
 			if i, ok := seen[t]; ok {
 				n.check.cycleError(path[i:], firstInSrc(path[i:]))
 				u = Typ[Invalid]
 				break
 			}
+
+			// acquire the lock without checking; performance is probably fine
+			t.resolve()
+			t.mu.Lock()
+			defer t.mu.Unlock()
+
+			// t.underlying might have been set while we were locking
+			if t.stateHas(underlying) {
+				u = t.underlying
+				break
+			}
+
 			seen[t] = len(seen)
 			path = append(path, t.obj)
-			t.resolve()
+
 			assert(t.fromRHS != nil || t.allowNilRHS)
 			rhs = t.fromRHS
+
 		default:
 			u = rhs // any type literal works
 		}
@@ -593,6 +641,7 @@ func (n *Named) under() Type {
 	// go back up the chain
 	for t := range seen {
 		t.underlying = u
+		t.setState(underlying)
 	}
 
 	return u
@@ -662,7 +711,8 @@ func (n *Named) expandRHS() (rhs Type) {
 		}()
 	}
 
-	assert(n.state() == unresolved)
+	assert(!n.stateHas(resolved))
+	assert(n.inst.orig.stateHas(resolved | loaded))
 
 	if n.inst.ctxt == nil {
 		n.inst.ctxt = NewContext()
@@ -671,9 +721,6 @@ func (n *Named) expandRHS() (rhs Type) {
 	ctxt := n.inst.ctxt
 	orig := n.inst.orig
 
-	assert(orig.state() >= resolved)
-	assert(orig.fromRHS != nil)
-
 	targs := n.inst.targs
 	tpars := orig.tparams