Skip to content

[lldb] Replace usage of remote AST with reflection on GetDynamicTypeAndAddress_Protocol. #3024

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 11, 2021
Merged

[lldb] Replace usage of remote AST with reflection on GetDynamicTypeAndAddress_Protocol. #3024

merged 1 commit into from
Jun 11, 2021

Conversation

augusto2112
Copy link

rdar://55412978

@augusto2112
Copy link
Author

@swift-ci please test with swiftlang/swift#37673

@augusto2112
Copy link
Author

test with swiftlang/swift#37673
@swift-ci please test

@augusto2112 augusto2112 force-pushed the remove-remoteast-from-GetDynamicTypeAndAddress_Protocol branch from 2732fca to 173c118 Compare June 7, 2021 19:28
@augusto2112
Copy link
Author

Not sure what this error means:

--- bootstrap: error: Command '['env', 'SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk', 'SWIFTCI_USE_LOCAL_DEPS=1', 'SWIFTPM_MACOS_DEPLOYMENT_TARGET=10.15', u'DYLD_LIBRARY_PATH=/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/bootstrap/lib:/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/tsc/lib:/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/llbuild-macosx-x86_64/lib:/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/yams/lib:/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/swift-argument-parser/lib:/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/swift-driver/lib:/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/swift-crypto/lib:/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/toolchain-macosx-x86_64/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/swift/macosx:/usr/lib/swift', u'/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/swift-test', '--enable-test-discovery', '--parallel', '--build-path', '/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/swiftpm-macosx-x86_64', '--configuration', 'release', '-Xlinker', '-rpath', '-Xlinker', u'@executable_path/../lib/swift/macosx', '-Xswiftc', '-module-cache-path', '-Xswiftc', '/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/swiftpm-macosx-x86_64/module-cache', '-Xmanifest', '-module-cache-path', '-Xmanifest', '/Users/buildnode/jenkins/workspace/apple-llvm-project-pr-macos/buildbot_incremental/swiftpm-macosx-x86_64/module-cache']' returned non-zero exit status 1

I'll re-run the tests.

@augusto2112
Copy link
Author

test with swiftlang/swift#37673
@swift-ci please test

@augusto2112 augusto2112 force-pushed the remove-remoteast-from-GetDynamicTypeAndAddress_Protocol branch from 173c118 to d2445eb Compare June 8, 2021 13:09
@augusto2112
Copy link
Author

test with swiftlang/swift#37673
@swift-ci please test

@augusto2112 augusto2112 force-pushed the remove-remoteast-from-GetDynamicTypeAndAddress_Protocol branch from d2445eb to 804d5a9 Compare June 9, 2021 19:40
@augusto2112
Copy link
Author

@kastiglione reflection currently doesn't parse metadata info generated by the JIT (when declaring things in lldb expressions/REPL). I updated the code to check if the protocol type belong to the __ldb_expr module and if so, use the remote AST implementation instead of reflection. This should be a temporary fix until we start parsing metadata from the modules generated by the JIT.

@augusto2112 augusto2112 force-pushed the remove-remoteast-from-GetDynamicTypeAndAddress_Protocol branch from 804d5a9 to dfd1828 Compare June 9, 2021 19:51
@adrian-prantl
Copy link

This is awesome, I just left a few smaller comments inline.

@adrian-prantl adrian-prantl self-requested a review June 9, 2021 21:03
adrian-prantl
adrian-prantl requested changes Jun 9, 2021
View reviewed changes
Copy link

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as "request changes" so I don't forget to re-review this after the next refactoring. The direction is good.

@augusto2112 augusto2112 force-pushed the remove-remoteast-from-GetDynamicTypeAndAddress_Protocol branch 2 times, most recently from 3dfde3f to 462b6bb Compare June 10, 2021 16:23
@augusto2112
Copy link
Author

test with swiftlang/swift#37673
@swift-ci please test

@adrian-prantl
Copy link

Almost there!

@augusto2112 augusto2112 force-pushed the remove-remoteast-from-GetDynamicTypeAndAddress_Protocol branch from 462b6bb to 72f5bcd Compare June 11, 2021 14:27
@augusto2112 augusto2112 force-pushed the remove-remoteast-from-GetDynamicTypeAndAddress_Protocol branch from 72f5bcd to 245972a Compare June 11, 2021 14:29
@augusto2112
Copy link
Author

@swift-ci please test

adrian-prantl
adrian-prantl reviewed Jun 11, 2021
View reviewed changes
lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntimeDynamicTypeResolution.cpp

swift::Demangle::Demangler dem;

NodePointer type = protocol_typeref->getDemangling(dem);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a followup-commit: We should factor this out into a static helper and unit-test it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the whole method?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remainder of the method that operates on the demangle tree. It doesn't need a live process or reflection metadata and can be easily unit-tested, so we should do that :-)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aah ok!

@augusto2112 augusto2112 merged commit 22609b3 into swiftlang:swift/release/5.5 Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrian-prantl adrian-prantl adrian-prantl approved these changes

@kastiglione kastiglione Awaiting requested review from kastiglione

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@augusto2112 @adrian-prantl @kastiglione