go/types, types2: set t.underlying exactly once in resolveUnderlying

While the locking in Named.resolveUnderlying is mostly fine, we do not
perform an atomic read before the write to underlying.

If resolveUnderlying returns and another thread was waiting on the lock,
it can perform a second (in this case identical) write to t.underlying.
A reader thread on n.underlying can thus observe either of those writes,
tripping the race detector.

Michael was kind enough to provide a diagram:

   T1                                   T2
1. t.stateHas(underlying) // false
2.                                      t.stateHas(underlying) // false
3. t.mu.Lock() // acquired
4.                                      t.mu.Lock() // blocked
5. t.underlying = u
6. t.setState(underlying)
7. t.mu.Unlock()
8.                                      t.underlying = u // overwritten
9.                                      t.setState(underlying)
10.                                     t.mu.Unlock()

Adding a second check before setting t.underlying prevents the write on
line 8.

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

1 files changed, 7 insertions, 0 deletions

diff --git a/src/cmd/compile/internal/types2/named.go b/src/cmd/compile/internal/types2/named.go
index 26be3104b8..a75ca75d0c 100644
--- a/src/cmd/compile/internal/types2/named.go
+++ b/src/cmd/compile/internal/types2/named.go
@@ -630,6 +630,7 @@ func (n *Named) resolveUnderlying() {
 			t.resolve()
 			t.mu.Lock()
 			defer t.mu.Unlock()
+
 			assert(t.fromRHS != nil || t.allowNilRHS)
 			rhs = t.fromRHS
 
@@ -640,6 +641,12 @@ func (n *Named) resolveUnderlying() {
 
 	// set underlying for all Named types in the chain
 	for t := range seen {
+		// Careful, t.underlying has lock-free readers. Since we might be racing
+		// another call to resolveUnderlying, we have to avoid overwriting
+		// t.underlying. Otherwise, the race detector will be tripped.
+		if t.stateHas(underlying) {
+			continue
+		}
 		t.underlying = u
 		t.setState(underlying)
 	}