🍒 [lldb][HostInfoMacOSX] Try to use DW_AT_LLVM_sysroot instead of xcrun when looking up SDK #10157
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ping checks This assertion was added to check that `RegisterXcodeSDK` will correctly update the source mappings of the module. However, the source mapping will only get updated if the command-line invocation of `xcrun` succeeded. Even if `xcrun` failed to find an SDK, the source mappings would get an entry (see https://github.com/llvm/llvm-project/blob/f6212c1cd3d8b827c7d7e2f6cf54b135c27eacc6/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm#L424-L444). But if the shell invocation itself failed, then the mappings are not updated. This means depending on how slow `xcrun` is on a given host, this test may fail. On my machine this happens consistently in debug and release builds. This patch removes this flakey assertion. We unfortunately lost some test coverage here but I'm not sure there's great alternatives unless we either: 1. Mock the `xcrun` call somehow (we could maybe pass a callable around which defaults to `xcrun` in non-test code?) 2. Make a `xcrun` time-out not an error either? (cherry picked from commit 338c9a4)
…of xcrun when looking up SDK" (llvm#129621)" This reverts commit 6041c74. Relands the original patch with the test-case data fixed. Weirldy the PR CI didn't seem to run the unit-tests? In any case, the problem was an incorrect expectation in the test-case data. Since we have both public and internal SDK in that test-case, we should `expect_mismatch` to be `true`. (cherry picked from commit 77a8770)
|
@swift-ci test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GetSDKRootusesxcrunto find an SDK root path for a given SDK version string. But if the SDK doesn't exist in the Xcode installations, but instead lives in theCommandLineTools,xcrunwill fail to find it. Negative searches for an SDK path cost a lot (a few seconds) each timexcrunis invoked. We do cache negative results infind_cached_pathinside LLDB, but we would still pay the price on every new debug session the first time we evaluate an expression. This doesn't only cause a noticable delay in running the expression, but also generates following error:In this patch we avoid these possibly expensive calls to
xcrunby checking theDW_AT_LLVM_sysroot, and if it exists, using that as the SDK path. We need an explicit check for theCommandLineToolspath before we callRegisterXcodeSDK, because that will try to callxcrun. This won't prevent other uses ofGetSDKRootpopping up that cause us to make expensivexcruncalls, but for now this addresses the regression in the expression evaluator. We also had to adjust theXcodeSDK::Mergelogic to update the sysroot. There is one case for which this wouldn't make sense: if a CU was compiled withCommandLineToolsand a different one with an older internal SDK, in that case we would update theCommandLineToolssysroot with a.Internal.sdkprefix, which won't possibly exist forCommandLineTools. I added a unit-test for this. Not sure if we want to explicitly detect and disallow this, given it's quite a niche scenario.rdar://113619904
rdar://113619723
(cherry picked from commit 77a8770)