x/tools/internal/lsp: SuggestedFix tests failing on arm64 builders starting between 2022-08-19T17:47:49 and 2022-08-23T21:04:06 #54655

bcmills · 2022-08-24T16:48:12Z

greplogs -l -e FAIL\\s+golang.org/x/tools/internal/lsp --since=2022-08-20 --omit=newcc --details

(46 matching logs)

The text was updated successfully, but these errors were encountered:

bcmills · 2022-08-24T16:53:04Z

The first failure in the logs was at CL 421502 (attn @pjweinb), but It's not clear to me whether this is due to a regression in x/tools proper or in the main Go repo. The dashboard appears to be missing test runs for these builders for a run of several CLs due to the high CL churn rate with the tree reopened.

bcmills · 2022-08-24T17:31:42Z

I'm seeing a lot of these messages in the logs:

 analysis unusedvariable for package golang.org/x/tools/internal/lsp/snippets panicked: invalid start/end token.Pos

CL 424920 would fit that timeframe (CC @mvdan @adonovan), though it's not at all clear to me how that would result in arm64-specific breakage. 🤔

adonovan · 2022-08-24T17:36:39Z

That change looks completely safe, and it shouldn't even affect timing significantly. I think the scope of the recover is too large---it should include only the analysers, not the setup and postprocessing code, which is where I think this problem is occurring. I'll take a look.

bcmills · 2022-08-24T17:39:52Z

The last successful run on these builders was at CL 424902 (2022-08-19T17:47:49Z).

pjweinb · 2022-08-24T21:22:00Z

that CL says the trybots are happy. The errors aren't in the hover code, which is what that CL fixed, but it's definitely weird. Does anyone have a darwin-arm64 to run the tests locally? Hard to see why gopls tests would fail deterministically on one os/architecture combination.

…

On Wed, Aug 24, 2022 at 12:53 PM Bryan C. Mills ***@***.***> wrote: The first failure in the logs was at CL 421502 <https://go.dev/cl/421502> (attn @pjweinb <https://github.com/pjweinb>), but It's not clear to me whether this is due to a regression in x/tools proper or in the main Go repo. The dashboard appears to be missing test runs for these builders for a run of several CLs due to the high CL churn rate with the tree reopened. — Reply to this email directly, view it on GitHub <#54655 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJIAI2PKST4R3EQCZLNYQTV2ZHP3ANCNFSM57QBN3OA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

adonovan · 2022-08-24T21:30:08Z

I've been debugging this for a while (on darwin/arm64), but I still can't reproduce it as shown in the logs. I agree the hover code is not to blame. But there's a lot going on in the log: We have a number of abuses of token.Pos, an overly broad recover intended to cover just the analysis plugins but that masks other bugs, and various nested "return err" statements that make it hard to trace where the errors originate. I'll be sending https://go.dev/cl/425356 to beef up logging momentarily.

bcmills · 2022-08-25T18:18:12Z

I've been debugging this for a while (on darwin/arm64), but I still can't reproduce it as shown in the logs.

Can you reproduce it on the actual builders using a gomote? (There may be hardware or environment differences relevant to the test. Some tests unfortunately even vary their behavior based on the GO_BUILDER_NAME environment variable. 😩)

This is a stopgap until I can diagnost the problem, but in the meantime we need to fix the builders. Updates golang/go#54655 Change-Id: I6260828e8c07e3121c45f99166a26d51aa9805a4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/425575 Run-TryBot: Alan Donovan <[email protected]> TryBot-Result: Gopher Robot <[email protected]> gopls-CI: kokoro <[email protected]> Reviewed-by: Tim King <[email protected]>

This change ensures that the End position provided to span.NewRange in suggestedAnalysisFixes is valid even when the diagnostic has only a valid start position. This seems to be the cause of some panics observed in the ARM builders in the attached issue. Also, reduce the scope of the recover operation to just the analyzer's run method: we don't want to hide further bugs (or discard stack traces) in the setup or postprocessing logic. Also: - split a single assertion in span.NewRange into two. - Add information to various error messages to help identify causes. - Add TODO comments about inconsistent treatment of token.File in span.FileSpan, and temporarily remove bug.Errorf that is obviously reachable from valid inputs. - Add TODO to fix another panic in an analyzer that is covered by our tests but was hitherto suppressed. - Add TODO to use bug.Errorf after recover to prevent recurrences. We can't do that until the previous panic is fixed. Updates golang/go#54655 Change-Id: I0576d03fcfffe0c8df157cf6c6520c9d402f8803 Reviewed-on: https://go-review.googlesource.com/c/tools/+/425356 Run-TryBot: Alan Donovan <[email protected]> gopls-CI: kokoro <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Suzy Mueller <[email protected]>

adonovan · 2022-08-26T17:03:05Z

Update:

I temporarily disabled two test cases that were failing only on ARM. The fillstruct analyzer's quickfix is inserting zero values (0, "") instead of names of local variables, which means that this call to FindBestMatch returned nil. That could be because its behavior has changed, or because its list of candidates from here is empty. I still have no hypothesis as to why its behavior is CPU-dependent (yet not reproducible on my local ARM machine).
I narrowed the scope of a recover in the analysis driver to include only Analyzer.Run. This revealed panics in the analysisDiagnosticDiagnostics post-processing logic that are routinely produced during tests but were being swept under the rug. I fixed one of these panics (invalid end pos passed to span.Range).
Another panic was caused by a bug.Reportf in span logic that was clearly reachable given valid inputs, so I suppressed it temporarily. We need to decide what the correct assertion is, if any.
The fillreturns analyzer is routinely panicking due to unsafe assumptions about ill-typed code. This is suppressed by the recover. We need to fix it then insert a bug.Reportf after recover so that we always notice panics in our tests.

So, TODO:

diagnose ARM-based problem wrt FindBestMatch and re-enable the two test cases.
fix fillreturns panic.
add bug.Reportf after recover to detect similar regressions.
decide what the invariants should be around spans and line directives.

bcmills · 2022-08-29T13:59:04Z

This might be fixed by CL 425188 once that is merged.

gopherbot · 2022-08-29T16:47:06Z

Change https://go.dev/cl/425188 mentions this issue: cmd/compile/obj/arm64: fix encoding error of FMOVD/FMOVS $0|ZR

gopherbot · 2022-08-29T17:55:22Z

Change https://go.dev/cl/426017 mentions this issue: internal/lsp/tests: re-enable ARM tests

This is a simple revert of CL 425497, which disabled the tests. The problem was due to a misencoded FMOVD instruction causing some 0.0 constants to compile to nonzero; fixed by CL 425188. Updates golang/go#54655 Updates golang/go#425188 Change-Id: I2231b57b7b78ac1ae2e8b60cda62898ea7122cda Reviewed-on: https://go-review.googlesource.com/c/tools/+/426017 gopls-CI: kokoro <[email protected]> Reviewed-by: Robert Findley <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Alan Donovan <[email protected]> Auto-Submit: Alan Donovan <[email protected]>

gopherbot · 2022-08-29T19:08:49Z

Change https://go.dev/cl/426018 mentions this issue: internal/lsp/cache: report analysis panics during testing

gopherbot · 2022-08-29T20:15:01Z

Change https://go.dev/cl/426019 mentions this issue: internal/lsp/analysis/fillreturns: be defensive w.r.t. type errors

In the presence of type errors, TypeOf may return nil, and this appears to have contributed to a crash in the fillreturns analysis. I haven't been able to find or deduce a test case, but this change makes the logic more defensive. Also remove a stale pre-go1.17 test that used to trigger a panic (the one fixed here? unclear) to assert that panics were recoverable. Updates golang/go#54655 Change-Id: Ic9ca9a307eede50a2d4db6424822a155dd43e635 Reviewed-on: https://go-review.googlesource.com/c/tools/+/426019 Auto-Submit: Alan Donovan <[email protected]> Reviewed-by: Robert Findley <[email protected]> TryBot-Result: Gopher Robot <[email protected]> gopls-CI: kokoro <[email protected]> Run-TryBot: Alan Donovan <[email protected]>

adonovan · 2022-09-01T14:42:13Z

Update:

the ARM problems were fixed by a patch to the assembler; the tests were reenabled.
fillreturns was made robust.
bug.Reportf is used after recover (pending, hopefully today).
analyses with RunDespiteErrors=false are no longer run after a type error (pending, today).
analyses are no longer run after failure of a prerequisite. I suspect this was responsible for a number of nondeterminstic failures after context timeouts caused analysis units to fail.
the span invariants question remains as a TODO in the code.

Report a bug when an analysis dependency is found to be neither "vertical" (for facts) or "horizontal" (for results). This has been observed in practise: the lookup from a package ID to a package object is not consistent. The bug report is suppressed for command-line-arguments packages to see whether it occurs on ordinary packages too. Also, add disabled logic to disable recovery and dump all goroutines, for temporary use when debugging. Unrelated things found during debugging: - simplify package key used in analysis cache: the mode is always the same constant. - move TypeErrorAnalyzers logic closer to where needed. Updates golang/go#54655 Updates golang/go#54762 Change-Id: I0c2afd9ddd4fcc21748a03f8fa0769a2de09a56f Reviewed-on: https://go-review.googlesource.com/c/tools/+/426018 Reviewed-by: Robert Findley <[email protected]>

gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Aug 24, 2022

bcmills added Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) and removed gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Aug 24, 2022

gopherbot added this to the Unreleased milestone Aug 24, 2022

bcmills added the arch-arm64 label Aug 24, 2022

bcmills added the release-blocker label Aug 24, 2022

bcmills modified the milestones: Unreleased, Go1.20 Aug 24, 2022

bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 24, 2022

bcmills changed the title ~~x/tools/internal/lsp: tests failing on arm64 builders since 2022-08-23~~ x/tools/internal/lsp: SuggestedFix tests failing on arm64 builders since 2022-08-23 Aug 24, 2022

bcmills changed the title ~~x/tools/internal/lsp: SuggestedFix tests failing on arm64 builders since 2022-08-23~~ x/tools/internal/lsp: SuggestedFix tests failing on arm64 builders between 2022-08-19T17:47:49 and 2022-08-23T21:04:06 Aug 24, 2022

bcmills changed the title ~~x/tools/internal/lsp: SuggestedFix tests failing on arm64 builders between 2022-08-19T17:47:49 and 2022-08-23T21:04:06~~ x/tools/internal/lsp: SuggestedFix tests failing on arm64 builders starting between 2022-08-19T17:47:49 and 2022-08-23T21:04:06 Aug 24, 2022

bcmills mentioned this issue Aug 29, 2022

cmd/dist,cmd/compile: unexpected stale targets on freebsd-arm64-dmgk since 2022-08-23 #54729

Closed

bcmills removed the Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) label Aug 29, 2022

gopherbot closed this as completed in eeb1ba7 Aug 29, 2022

golang locked and limited conversation to collaborators Sep 8, 2023

gopherbot added the FrozenDueToAge label Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/tools/internal/lsp: SuggestedFix tests failing on arm64 builders starting between 2022-08-19T17:47:49 and 2022-08-23T21:04:06 #54655

x/tools/internal/lsp: SuggestedFix tests failing on arm64 builders starting between 2022-08-19T17:47:49 and 2022-08-23T21:04:06 #54655

bcmills commented Aug 24, 2022 •

edited

Loading

bcmills commented Aug 24, 2022

bcmills commented Aug 24, 2022

adonovan commented Aug 24, 2022 •

edited

Loading

bcmills commented Aug 24, 2022

pjweinb commented Aug 24, 2022 via email

adonovan commented Aug 24, 2022 •

edited by dmitshur

Loading

bcmills commented Aug 25, 2022

adonovan commented Aug 26, 2022 •

edited

Loading

bcmills commented Aug 29, 2022

gopherbot commented Aug 29, 2022

gopherbot commented Aug 29, 2022

gopherbot commented Aug 29, 2022

gopherbot commented Aug 29, 2022

adonovan commented Sep 1, 2022

x/tools/internal/lsp: SuggestedFix tests failing on arm64 builders starting between 2022-08-19T17:47:49 and 2022-08-23T21:04:06 #54655

x/tools/internal/lsp: SuggestedFix tests failing on arm64 builders starting between 2022-08-19T17:47:49 and 2022-08-23T21:04:06 #54655

Comments

bcmills commented Aug 24, 2022 • edited Loading

bcmills commented Aug 24, 2022

bcmills commented Aug 24, 2022

adonovan commented Aug 24, 2022 • edited Loading

bcmills commented Aug 24, 2022

pjweinb commented Aug 24, 2022 via email

adonovan commented Aug 24, 2022 • edited by dmitshur Loading

bcmills commented Aug 25, 2022

adonovan commented Aug 26, 2022 • edited Loading

bcmills commented Aug 29, 2022

gopherbot commented Aug 29, 2022

gopherbot commented Aug 29, 2022

gopherbot commented Aug 29, 2022

gopherbot commented Aug 29, 2022

adonovan commented Sep 1, 2022

bcmills commented Aug 24, 2022 •

edited

Loading

adonovan commented Aug 24, 2022 •

edited

Loading

adonovan commented Aug 24, 2022 •

edited by dmitshur

Loading

adonovan commented Aug 26, 2022 •

edited

Loading