From c69dfaffd78ea784616c6efc7ece5eea34eaa8c0 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 8 Apr 2024 16:03:41 -0400 Subject: [PATCH] [gopls-release-branch.0.15] gopls/internal/cache: add debug assertions to refine golang/go#66732 Also, use go1.19 generic helpers for atomics. Updates golang/go#66732 Updates golang/go#66730 Change-Id: I7aa447144353ff2ac5906ca746e2f98b115aa732 Reviewed-on: https://go-review.googlesource.com/c/tools/+/577435 LUCI-TryBot-Result: Go LUCI Reviewed-by: Robert Findley Auto-Submit: Alan Donovan (cherry picked from commit f6298eb1183ae4a636b2240a31cf359ade3e4f26) Reviewed-on: https://go-review.googlesource.com/c/tools/+/577596 --- gopls/internal/cache/analysis.go | 50 +++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index d462a9e3dc6..bb27142e779 100644 --- a/gopls/internal/cache/analysis.go +++ b/gopls/internal/cache/analysis.go @@ -41,6 +41,7 @@ import ( "golang.org/x/tools/gopls/internal/util/astutil" "golang.org/x/tools/gopls/internal/util/bug" "golang.org/x/tools/gopls/internal/util/frob" + "golang.org/x/tools/gopls/internal/util/maps" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/event/tag" "golang.org/x/tools/internal/facts" @@ -177,8 +178,6 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac var tagStr string // sorted comma-separated list of PackageIDs { - // TODO(adonovan): replace with a generic map[S]any -> string - // function in the tag package, and use maps.Keys + slices.Sort. keys := make([]string, 0, len(pkgs)) for id := range pkgs { keys = append(keys, string(id)) @@ -303,10 +302,10 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac } // Add edge from predecessor. if from != nil { - atomic.AddInt32(&from.unfinishedSuccs, 1) // TODO(adonovan): use generics + from.unfinishedSuccs.Add(+1) // incref an.preds = append(an.preds, from) } - atomic.AddInt32(&an.unfinishedPreds, 1) + an.unfinishedPreds.Add(+1) return an, nil } @@ -387,7 +386,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac // prevents workers from enqeuing, and thus finishing, and thus allowing the // group to make progress: deadlock. limiter := make(chan unit, runtime.GOMAXPROCS(0)) - var completed int64 + var completed atomic.Int64 var enqueue func(*analysisNode) enqueue = func(an *analysisNode) { @@ -399,13 +398,13 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac if err != nil { return err // cancelled, or failed to produce a package } - maybeReport(atomic.AddInt64(&completed, 1)) + maybeReport(completed.Add(1)) an.summary = summary // Notify each waiting predecessor, // and enqueue it when it becomes a leaf. for _, pred := range an.preds { - if atomic.AddInt32(&pred.unfinishedSuccs, -1) == 0 { + if pred.unfinishedSuccs.Add(-1) == 0 { // decref enqueue(pred) } } @@ -427,6 +426,18 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac return nil, err // cancelled, or failed to produce a package } + // Inv: all root nodes now have a summary (#66732). + // + // We know this is falsified empirically. This means either + // the summary was "successfully" set to nil (above), or there + // is a problem with the graph such the enqueuing leaves does + // not lead to completion of roots (or an error). + for _, root := range roots { + if root.summary == nil { + bug.Report("root analysisNode has nil summary") + } + } + // Report diagnostics only from enabled actions that succeeded. // Errors from creating or analyzing packages are ignored. // Diagnostics are reported in the order of the analyzers argument. @@ -458,6 +469,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac } // Inv: root.summary is the successful result of run (via runCached). + // TODO(adonovan): fix: root.summary is sometimes nil! (#66732). summary, ok := root.summary.Actions[stableNames[a]] if summary == nil { panic(fmt.Sprintf("analyzeSummary.Actions[%q] = (nil, %t); got %v (#60551)", @@ -475,7 +487,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac } func (an *analysisNode) decrefPreds() { - if atomic.AddInt32(&an.unfinishedPreds, -1) == 0 { + if an.unfinishedPreds.Add(-1) == 0 { an.summary.Actions = nil } } @@ -510,8 +522,8 @@ type analysisNode struct { analyzers []*analysis.Analyzer // set of analyzers to run preds []*analysisNode // graph edges: succs map[PackageID]*analysisNode // (preds -> self -> succs) - unfinishedSuccs int32 - unfinishedPreds int32 // effectively a summary.Actions refcount + unfinishedSuccs atomic.Int32 + unfinishedPreds atomic.Int32 // effectively a summary.Actions refcount allDeps map[PackagePath]*analysisNode // all dependencies including self exportDeps map[PackagePath]*analysisNode // subset of allDeps ref'd by export data (+self) summary *analyzeSummary // serializable result of analyzing this package @@ -664,6 +676,9 @@ func (an *analysisNode) runCached(ctx context.Context) (*analyzeSummary, error) if data, err := filecache.Get(cacheKind, key); err == nil { // cache hit analyzeSummaryCodec.Decode(data, &summary) + if summary == nil { // debugging #66732 + bug.Reportf("analyzeSummaryCodec.Decode yielded nil *analyzeSummary") + } } else if err != filecache.ErrNotFound { return nil, bug.Errorf("internal error reading shared cache: %v", err) } else { @@ -673,8 +688,11 @@ func (an *analysisNode) runCached(ctx context.Context) (*analyzeSummary, error) if err != nil { return nil, err } + if summary == nil { // debugging #66732 (can't happen) + bug.Reportf("analyzeNode.run returned nil *analyzeSummary") + } - atomic.AddInt32(&an.unfinishedPreds, +1) // incref + an.unfinishedPreds.Add(+1) // incref go func() { defer an.decrefPreds() //decref @@ -742,13 +760,11 @@ func (an *analysisNode) cacheKey() [sha256.Size]byte { } // vdeps, in PackageID order - depIDs := make([]string, 0, len(an.succs)) - for depID := range an.succs { - depIDs = append(depIDs, string(depID)) - } - sort.Strings(depIDs) // TODO(adonovan): avoid conversions by using slices.Sort[PackageID] + depIDs := maps.Keys(an.succs) + // TODO(adonovan): use go1.2x slices.Sort(depIDs). + sort.Slice(depIDs, func(i, j int) bool { return depIDs[i] < depIDs[j] }) for _, depID := range depIDs { - vdep := an.succs[PackageID(depID)] + vdep := an.succs[depID] fmt.Fprintf(hasher, "dep: %s\n", vdep.mp.PkgPath) fmt.Fprintf(hasher, "export: %s\n", vdep.summary.DeepExportHash)