-
Notifications
You must be signed in to change notification settings - Fork 10.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DNM][SourceKit] Log attempts to mangle a module instead of llvm_unreachable for SourceKit/LSP #28051
Conversation
Thanks for finding this! It seems like SourceKit should probably catch this before it gets to the mangling logic, but isn't for some reason. Could you point us to an example file + line in one of packages you mention where jump-to-definition is hitting this? |
Thanks for picking this up! It’s difficult to give you a line number and file due to the error arriving on stderr after the and the call has been made but if you want to replicate the problem, clone the project There is a second, far rarer crash which affects the project
|
If you want to stress test the crashes remaining after this fix you can use this script ("timed out" is where there is a
|
A little more information about the second rarer crash I mentioned above. It's probably best to use the project |
Thanks @nathawes for properly resolving the most common crash I mentioned with your commit of Friday. I've been looking at the few remaining probelms and found two "solutions" of a sort. The First Type of crash is something along the lines of:
This is where "siteify" was looking up the identifier
which I "resolved" by commenting out the assertion for now (see last commit on this branch). I lost a bit of time because "siteify", sourcekit-lsp and perhaps all Cursor Info lookups in SourceKit seems to not be working correctly on master since this commit 4023199. If I take a branch before this commit things are working again. If you have the time to look at them this leaves a few crashes that I've not been able to get to the bottom of but you can replicate easily using this more focused script + the siteify project:
I'm seeing:
This is after processing the first 765 swift packages in Dave's list. Thanks very much for your help! |
What is the problem you're seeing? I downloaded the master snapshot from Nov 8 which includes that commit and it appears to be working in general. |
If I build SourceKitService from master and copy it into the toolchain from Nov 1st, sourcekit-lsp requests for definition and references don't return anything and siteify does not link to definition or any USR references. If I build master from before the commit I mention with no other changes and apply nathan's fix things work fine. |
Oh, ok. This could just be because SourceKitService is out of sync with the stdlib, which is not supported. |
Ah, I've download the toolchain for the 8th and am rebuilding master now. IndexDB format change? |
No, but any lock-step changes between the compiler and stdlib can cause trouble. It's not a configuration we can keep working in sourcekit since the compiler doesn't support it either. |
Up and running again. I downloaded the 11-08 toolchain which is compatible with master. From the look of the PR I mentioned it changed the module format which must have been the problem. What I reported above is correct but right now I'm looking at two files which crash the SourceKit tokeniser 😱 (syntax map on file open) - project https://github.com/cats-oss/cujira.git, files Sources/Core/Common/Enumerable.swift and Sources/Core/Common/Extension/Gap.swift, if you use the toolchain in Xcode you can see the crash. |
I've commented out another assertion which "solves" the |
4ea8198
to
f265b69
Compare
I've added a commit commenting out the remaining assertions that where causing SourceKitService to crash. I can't speak to whether these were valid indications of an underlying problem or recent commits looking to catch problems early without realising the impact on SourceKit. In any case I've had a clean run of the 17 projects I mention in the second ruby script above. Specifically (if you want to replicate the crash using
The changes to lib/AST/ASTScopeLookup.cpp fix the problems in https://github.com/AsyncNinja/AsyncNinja.git (and https://github.com/123flo321/pogoprotos-swift.git)
The change to lib/Basic/Decl.cpp fixes a problem in https://github.com/adam-fowler/aws-signer.git
The change to lib/IDE/SyntaxModel.cpp a problem in https://github.com/adam-fowler/xml-encoder.git
|
I think #28226 should have solved the "Extensions must have already been bound" assertion hit and the second in your previous comment. I still need to look into the other two though. |
Thanks @nathawes, I've tried to give you all the information you need to replicate. |
Yeah thanks for that! Having reproducers helps a lot. I'm planning on tackling the mangler/USR generation issue next. |
From memory the demangler in the verify() returns NULL for
|
@johnno1962 This pull request should be broken down into multiple SRs if it isn't already. |
This is not a serious pull request, more documentation of the assertions I had to remove to run through a large corpus of code. |
I'm grateful that you took the time to reduce the problem and provide your analysis - it was super helpful getting these things fixed on our end - but we have a policy of not taking issues, or things that look like issues, on Github and tracking work in Jira tickets. As unpleasant as it is, it is standard operating procedure. Have you tried seeing if a recent toolchain fixes the issues you saw too? I'm wondering if we still need to keep this open after Nathan's patches. |
f265b69
to
ba40a0a
Compare
I've created an SR https://bugs.swift.org/browse/SR-11863, to track these crashes referring to this PR which is the easiest way to track the remaining problems. I've also rebased to master and removed from this PR the crash points that have been resolved. The most common sources of assertion failures have been fixed now (thanks!). The next most common at this stage and perhaps the easiest to fix in a superficial way is the "Attribute's TokenNodes already consumed?" assertion failure which occurs in projects https://github.com/eBardX/XestiMonitors.git at XestiMonitors/Sources/Core/UIKit/Accessibility/AccessibilityStatusMonitor.swift:10:1, https://github.com/adam-fowler/xml-encoder.git at xml-encoder/Sources/xml-encoder/XMLDecoder.swift:49:8, https://github.com/adam-fowler/dictionary-encoder.git at dictionary-encoder/Sources/DictionaryEncoder/DictionaryEncoder.swift:61:6, https://github.com/ashleymills/Reachability.swift.git at Reachability.swift/Sources/Reachability.swift:46:8. After that the crashes are getting pretty rare and it may not be worth pursuing them. Thanks again! |
I’m closing this now to tidy things up. I’ve just done a test run using a toolchain from master and the script above and main problem remaining is the new "Attribute's TokenNodes already consumed?" assertion failure which may be more trouble than it’s worth but it’s not so common. Thanks 👍 |
@johnno1962 I'm actually fixing that one at the moment :-) |
Ha, Terrific! Do you know if it’ll make the Xcode 11.4 release? |
I wasn't planning on cherry-picking it into the release branch because it's purely an assertion failure. When assertions are disabled (like they are in a release build) it doesn't crash and the only difference the fix makes otherwise is that built-in attributes on enum cases are now correctly reported as built-in (instead of non-builtin) in SourceKit's syntax map used for syntax coloring. |
Ah, silly me. Do you know if there are plans to include the sourcekit-lsp binary in Xcode? I was surprised not to see it in the 11.4 beta1 toolchain. |
It should be at
in the 11.4 beta. |
It is indeed there. Great! |
Hi Apple, @benlangmuir
I'm seeing SourceKit crashes using a form of LSP stress tester siteify which tokenises a source file using SourceKit then attempts to find the definition of all identifiers in a source file via LSP. A typical crash I'm seeing is:
Inserting some logging turns out this is where "siteify" tries to look up an identifier for a module reaching the
llvm_unreachable
that I've changed to anfprintf
to standard error. This is the most common SourceKit crash processing the list of packages from https://github.com/daveverwer/SwiftPMLibraryJohn