Skip to content

Implement projectExistentialAndUnwrapClass #37673

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 10, 2021
Merged

Implement projectExistentialAndUnwrapClass #37673

merged 1 commit into from
Jun 10, 2021

Conversation

augusto2112
Copy link
Contributor

@augusto2112 augusto2112 commented May 27, 2021 •
edited
Loading

  • Explanation: Adds a new function, projectExistentialAndUnwrapClass, which behaves like projectExistential but is tailored for LLDB, so LLDB can find the dynamic type of objects using reflection.

  • Scope: The new function behaves like projectExistential , except for the following differences:

    1 - When it comes to existentials, LLDB stores the address of the error
    pointer, which must be dereferenced before further processing.

    2 - When the existential wraps a class type, LLDB expects the address
    returned is the class instance itself and not the address of the
    reference.

  • Risk: Low, this change is purely additive.

  • Testing: Modified /validation-test/Reflection/existentials.swift to test the new functionality.

  • Issue: rdar://55412978

  • Reviewer: Adrian Prantl (@adrian-prantl)

@augusto2112 augusto2112 requested a review from a team as a code owner May 27, 2021 18:14
adrian-prantl
adrian-prantl reviewed May 27, 2021
View reviewed changes
adrian-prantl
adrian-prantl reviewed May 27, 2021
View reviewed changes
adrian-prantl
adrian-prantl reviewed May 27, 2021
View reviewed changes
adrian-prantl
adrian-prantl reviewed May 27, 2021
View reviewed changes
adrian-prantl
adrian-prantl reviewed May 27, 2021
View reviewed changes
adrian-prantl
adrian-prantl reviewed May 27, 2021
View reviewed changes
adrian-prantl
adrian-prantl reviewed May 27, 2021
View reviewed changes
adrian-prantl
adrian-prantl reviewed May 27, 2021
View reviewed changes
@augusto2112 augusto2112 force-pushed the project-existential-lldb branch 2 times, most recently from 15eff2b to 9d9b4c2 Compare May 28, 2021 20:18
@augusto2112 augusto2112 requested a review from adrian-prantl May 28, 2021 20:23
@adrian-prantl
Copy link
Contributor

This looks good, it would be great if we could also add a test similar to the other reflection tests in the validation test suite.

@augusto2112
Copy link
Contributor Author

@swift-ci test

@adrian-prantl
Copy link
Contributor

Please make sure to also cherry-pick this to the main branch.

@swift-ci
Copy link
Contributor

swift-ci commented Jun 4, 2021

Build failed
Swift Test OS X Platform
Git Sha - 123b97c56b4cad3fb45684c0fef86a73119dbe75

@augusto2112
Copy link
Contributor Author

Seems like macOS testing hung after running all tests?

15:17:03 PASS: Swift(macosx-x86_64) :: stdlib/StringNormalization.swift (14615 of 14618)
15:17:09 PASS: Swift(macosx-x86_64) :: stdlib/UnicodeTrieGenerator.gyb (14616 of 14618)
15:17:47 PASS: Swift(macosx-x86_64) :: stdlib/StringMemoryTest.swift (14617 of 14618)
16:41:07 
Build timed out (after 83 minutes). Marking the build as failed.
16:41:08 Build was aborted
16:41:08 Archiving artifacts
16:41:08 ninja: build stopped: interrupted by user.
16:41:12 Recording test results
16:41:14 ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?

Doesn't seems to be related to the test I changed:

15:04:54 PASS: Swift(macosx-x86_64) :: Reflection/existentials.swift (3675 of 14618)

@augusto2112
Copy link
Contributor Author

@swift-ci test

@swift-ci
Copy link
Contributor

swift-ci commented Jun 4, 2021

Build failed
Swift Test OS X Platform
Git Sha - 123b97c56b4cad3fb45684c0fef86a73119dbe75

@adrian-prantl
Copy link
Contributor

Looks like there's one real failure on 32-bit watchsimulator:

https://ci.swift.org/job/swift-PR-macos/27591/testReport/junit/Swift(watchsimulator-i386)/Reflection/existentials_swift/

@augusto2112 augusto2112 force-pushed the project-existential-lldb branch from 123b97c to f732eee Compare June 5, 2021 13:16
@augusto2112
Copy link
Contributor Author

@swift-ci test

@swift-ci
Copy link
Contributor

swift-ci commented Jun 5, 2021

Build failed
Swift Test OS X Platform
Git Sha - f732eeeca528352feba535a2a678e709eb14c304

@augusto2112 augusto2112 force-pushed the project-existential-lldb branch from f732eee to 45dd83b Compare June 5, 2021 19:24
@augusto2112
Copy link
Contributor Author

@swift-ci test

@augusto2112 augusto2112 mentioned this pull request Jun 7, 2021
Merged
@adrian-prantl
Copy link
Contributor

adrian-prantl commented Jun 7, 2021 •
edited
Loading

We are going to need to swift-ci nominate this. Can you edit the PR description to include a block with more context (for example like #37493) and call out that this only adds a new API for LLDB and has no efrect on the compiler?

@augusto2112
Copy link
Contributor Author

Since I refactored projectExistential, this change also affects remote mirrors as well (it returns the dynamic type for class existentials, which wasn't happening before). I could leave projectExistential untouched here and open a second pr to swift/main with the refactor after this patch is merged.

@adrian-prantl
Copy link
Contributor

Since I refactored projectExistential, this change also affects remote mirrors as well (it returns the dynamic type for class existentials, which wasn't happening before). I could leave projectExistential untouched here and open a second pr to swift/main with the refactor after this patch is merged.

It is probably a good idea to move out the NFC refactoring into a separate patch anyway, so why not do that? It makes the decision to take the remaining patch easier when it has a smaller surface area.

@augusto2112 augusto2112 force-pushed the project-existential-lldb branch 2 times, most recently from 8e772c1 to d85ae9b Compare June 8, 2021 16:37
@augusto2112
Copy link
Contributor Author

@swift-ci test

@augusto2112
Copy link
Contributor Author

@swift-ci please nominate

@swift-ci
Copy link
Contributor

swift-ci commented Jun 8, 2021

Build failed
Swift Test OS X Platform
Git Sha - d85ae9b70a27caf14b2a67a73eb28fa4da33d774

@augusto2112 augusto2112 force-pushed the project-existential-lldb branch from d85ae9b to 21863e3 Compare June 8, 2021 18:40
@augusto2112
Copy link
Contributor Author

augusto2112 commented Jun 8, 2021 •
edited
Loading

Looks like I accidentally reversed one of the fixes I made to the existentials.swift test on 32 bits.

@augusto2112
Copy link
Contributor Author

@swift-ci test

mikeash
mikeash approved these changes Jun 8, 2021
View reviewed changes
Copy link
Contributor

@mikeash mikeash left a comment

Choose a reason for hiding this comment

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

LGTM. DereferenceAndSet confused me momentarily, but I think I like it now.

@kastiglione kastiglione changed the title Implement projectExistentialForLLDB Implement projectExistentialAndUnwrapClass Jun 8, 2021
kastiglione
kastiglione reviewed Jun 9, 2021
View reviewed changes
include/swift/SwiftRemoteMirror/SwiftRemoteMirror.h
/// may derefence and set OutStartOfInstanceData if OutInstanceTypeRef is a
/// class TypeRef.
SWIFT_REMOTE_MIRROR_LINKAGE
int swift_reflection_projectExistentialAndUnwrapClass(
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used in swift-reflection-test.c so we can access it from the existentials.swift test.

kastiglione
kastiglione reviewed Jun 9, 2021
View reviewed changes
@augusto2112
Implement projectExistentialAndUnwrapClass
e36e8ee 
Implement a version of projectExistential tailored for LLDB. There are 2
differences when projecting existentials for LLDB:

1 - When it comes to existentials, LLDB stores the address of the error
    pointer, which must be dereferenced.
2 - When the existential wraps a class type, LLDB expects the address
    returned is the class instance itself and not the address of the
    reference.

This patch also adapts the swift reflection test machinery to test
projectExistentialAndUnwrapClass as well. This is done by exposing
the new functionality from swift reflection test.  It is tested in
existentials.swift, and ensures that the typeref information is
exactly the same as what is expected from projectExistential,
except the out address.
@augusto2112 augusto2112 force-pushed the project-existential-lldb branch from 21863e3 to e36e8ee Compare June 9, 2021 20:04
@augusto2112
Copy link
Contributor Author

@swift-ci test

@adrian-prantl adrian-prantl merged commit 3a09d63 into swiftlang:release/5.5 Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kastiglione kastiglione kastiglione left review comments

@mikeash mikeash mikeash approved these changes

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

@fredriss fredriss fredriss approved these changes

@slavapestov slavapestov Awaiting requested review from slavapestov

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

Successfully merging this pull request may close these issues.
6 participants
@augusto2112 @adrian-prantl @swift-ci @kastiglione @mikeash @fredriss