Implemement TypeSystemSwiftTypeRef::GetBitSize() (NFC) #1636

adrian-prantl · 2020-08-12T00:45:58Z

In order to implement GetBitSize, this patch also adds an initial
TypeRef-based implementation of
SwiftLanguageRuntimeImpl::DoArchetypeBinding(). To make
SwiftLanguageRuntimeImpl::GetTypeInfo() work with TypeRefs of
ClangImported non-Objective-C types, a new callback is added to
swift::reflection::MemoryReader, which allos LLDB to provide the
swift::reflection::TypeInfo for a Clang type based on debug
information.

rdar://problem/55412920

Needs swiftlang/swift#33417

adrian-prantl · 2020-08-12T00:49:53Z

This is not yet ready, a few tests are still failing. Just trying to get early feedback for swiftlang/swift#33417

adrian-prantl · 2020-08-25T21:16:53Z

@kastiglione This is my work-in-progress for the GetBitSize() implementation. The patch is rather larger because it depends on an implementation of DoArchetypBinding(), which makes up the bulk the wor.

adrian-prantl · 2020-08-28T16:40:39Z

This is good to go now.

adrian-prantl · 2020-08-28T16:40:47Z

@swift-ci test

adrian-prantl · 2020-08-28T21:42:01Z

test with swiftlang/swift#33417
@swift-ci test

adrian-prantl · 2020-08-28T21:42:35Z

test with swiftlang/swift#33417
@swift-ci test

kastiglione

This is a first pass of reading and questions, there's a lot here (for me anyway), I'll do a follow up read. Sorry for some minor comments.

kastiglione · 2020-08-28T22:34:26Z

lldb/source/Target/SwiftLanguageRuntimeImpl.h

@@ -114,6 +134,21 @@ class SwiftLanguageRuntimeImpl {
  bool IsABIStable();

 protected:
+  /// Use the reflection context to build a TypeRef object.
+  ///
+  ///\param module can be used to specify a module to look up DWARF


is this correct formatting, or will it cause rendering issues?

That's a typo!

lldb/source/Target/SwiftLanguageRuntimeImpl.h

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

kastiglione · 2020-08-28T23:47:10Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

+    return nullptr;
+  swift::Demangle::NodePointer node =
+      TypeSystemSwiftTypeRef::GetCanonicalDemangleTree(
+          module ? module : ts->GetModule(), Dem, mangled_name.GetStringRef());


Suggested change

module ? module : ts->GetModule(), Dem, mangled_name.GetStringRef());

module ?: ts->GetModule(), Dem, mangled_name.GetStringRef());

kastiglione · 2020-08-28T23:50:04Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

+      // but all the other functions currently get this wrong, too!
+      m_process.GetTarget().CalculateExecutionContext(exe_ctx);


am I understanding right that this isn't using frame, to match other places calling it wrong?

Yes, I made a note in rdar://problem/67587895 Locking the wrong scratch context in most places in SwiftLanguageRuntimeDynamicTypeResolution.cpp to fix this in a separate commit.

adrian-prantl · 2020-08-29T00:34:29Z

lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp

+
+std::string SwiftASTContext::ImportName(const clang::NamedDecl *clang_decl) {
+  if (auto clang_importer = GetClangImporter()) {
+    // FIMXE: It's not clear why this special case is necessary.


Note to self: I forgot about this. I meant to check this before uploading.

It's no longer necessary.

adrian-prantl · 2020-09-01T21:11:44Z

test with swiftlang/swift#33417
@swift-ci test

In order to implement GetBitSize, this patch also adds an initial TypeRef-based implementation of SwiftLanguageRuntimeImpl::DoArchetypeBinding(). To make SwiftLanguageRuntimeImpl::GetTypeInfo() work with TypeRefs of ClangImported non-Objective-C types, a new callback is added to swift::reflection::MemoryReader, which allos LLDB to provide the swift::reflection::TypeInfo for a Clang type based on debug information. <rdar://problem/55412920>

The SwiftOptionSet data formatter still reconstructs types after ths patch because there is leftover work in ClangImporter to separate out name importing. <rdar://problem/67709905>

adrian-prantl · 2020-09-04T19:18:59Z

test with swiftlang/swift#33417
@swift-ci smoke test

adrian-prantl · 2020-09-04T20:15:39Z

test with swiftlang/swift#33417
@swift-ci smoke test

adrian-prantl · 2020-09-04T20:15:52Z

test with swiftlang/swift#33417
@swift-ci test

adrian-prantl · 2020-09-04T20:16:27Z

test with swiftlang/swift#33417
@swift-ci test

module that does not have two different implementations that vary based off an opaque #define.

adrian-prantl · 2020-09-04T23:01:54Z

test with swiftlang/swift#33417
@swift-ci test

adrian-prantl · 2020-09-05T01:31:34Z

test with swiftlang/swift#33417
@swift-ci test

adrian-prantl · 2020-09-06T16:41:17Z

test with swiftlang/swift#33417
@swift-ci test

kastiglione · 2020-09-10T19:40:48Z

Did this not get picked into master-next?

adrian-prantl · 2020-09-10T19:55:36Z

Yes, at dab2fc6 <dab2fc6> Sorry I forgot to put a note into the PR.

…

-- adrian

On Sep 10, 2020, at 12:41 PM, Dave Lee ***@***.***> wrote: Did this not get picked into master-next? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1636 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARFXBOGDBALRC5T2QBUG33SFETVBANCNFSM4P3Z67ZQ>.

kastiglione · 2020-09-10T20:03:09Z

Do we prefer to push directly instead of using a PR?

kastiglione · 2020-09-10T20:03:56Z

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

@@ -1587,7 +1643,64 @@ TypeSystemSwiftTypeRef::GetPointerType(opaque_compiler_type_t type) {
 llvm::Optional<uint64_t>
 TypeSystemSwiftTypeRef::GetBitSize(opaque_compiler_type_t type,
                                   ExecutionContextScope *exe_scope) {
-  return m_swift_ast_context->GetBitSize(ReconstructType(type), exe_scope);
+  auto impl = [&]() -> llvm::Optional<uint64_t> {
+    // Bug-for-bug compatibility. See comment in SwiftASTContext::GetBitSize().


How come this implementation is so much different from master-next? dab2fc6#diff-4f052b540e66838ef123c3a839bacf1cR1617-R1629

For example GetAsClangTypeOrNull exists in this PR but not in the master-next version?

Thank you for catching this! I mush have cherry-picked an older version of the commit. Wow.

Hmm.. no. git diff origin/swift/master doesn't show a difference. Is it possible that this is the state of one of the earlier commits in the PR and not the final one?

the reason I noticed this is because when I cherry picked #1779, there were changes to GetAsClangTypeOrNull that did not exist at all on swift/master-next.

I am confused 🤔

It seems I have done something slightly wrong in my cherry-pick in #1779.

adrian-prantl · 2020-09-10T22:03:16Z

Do we prefer to push directly instead of using a PR?

Either way fine — I usually create two PRs so I can merge them at the same time once the testing for the master one has succeeded. This was an exception.

adrian-prantl requested review from vedantk and dcci August 12, 2020 00:48

adrian-prantl changed the title ~~Implemement TypeSystemSwiftTypeRef::GetBitSize() (NFC)~~ [WIP] Implemement TypeSystemSwiftTypeRef::GetBitSize() (NFC) Aug 12, 2020

adrian-prantl force-pushed the 55412920 branch 3 times, most recently from 9996dd0 to 34ec1a3 Compare August 14, 2020 21:34

adrian-prantl force-pushed the 55412920 branch 2 times, most recently from ac8941d to f26b688 Compare August 22, 2020 00:56

adrian-prantl force-pushed the 55412920 branch 3 times, most recently from c37068b to 6814c3f Compare August 28, 2020 00:59

adrian-prantl changed the title ~~[WIP] Implemement TypeSystemSwiftTypeRef::GetBitSize() (NFC)~~ Implemement TypeSystemSwiftTypeRef::GetBitSize() (NFC) Aug 28, 2020

kastiglione reviewed Aug 29, 2020

View reviewed changes

adrian-prantl commented Aug 29, 2020

View reviewed changes

adrian-prantl force-pushed the 55412920 branch 6 times, most recently from e589cb0 to 4f33869 Compare September 1, 2020 21:11

adrian-prantl changed the base branch from swift/master-rebranch to swift/master September 3, 2020 19:55

adrian-prantl added 2 commits September 4, 2020 12:17

Factor out TypeSystemSwift::GetSupportedLanguagesForTypes() (NFC)

412199a

adrian-prantl added 2 commits September 4, 2020 12:17

Implement TypeSystemSwiftTypeRef::IsImportedType() (NFC)

79ebfdf

The SwiftOptionSet data formatter still reconstructs types after ths patch because there is leftover work in ClangImporter to separate out name importing. <rdar://problem/67709905>

Adapt to upstream changes introducing type lookup errors.

16bcb30

adrian-prantl force-pushed the 55412920 branch from 4f33869 to 16bcb30 Compare September 4, 2020 19:18

adrian-prantl mentioned this pull request Sep 4, 2020

Add a callback to swift::reflection::MemoryReader that allows LLDB to… swiftlang/swift#33417

Merged

TestSwiftSystemFrameworks: Pick another random datatype from the

0aa734e

module that does not have two different implementations that vary based off an opaque #define.

qualify dyn_cast (NFC)

1a7506d

Disable reproducer test on Linux

d7723d7

adrian-prantl merged commit b5bedc4 into swiftlang:swift/master Sep 8, 2020

kastiglione reviewed Sep 10, 2020

View reviewed changes

	module ? module : ts->GetModule(), Dem, mangled_name.GetStringRef());
	module ?: ts->GetModule(), Dem, mangled_name.GetStringRef());

		// but all the other functions currently get this wrong, too!
		m_process.GetTarget().CalculateExecutionContext(exe_ctx);

Implemement TypeSystemSwiftTypeRef::GetBitSize() (NFC) #1636

Implemement TypeSystemSwiftTypeRef::GetBitSize() (NFC) #1636

Uh oh!

Conversation

adrian-prantl commented Aug 12, 2020

Uh oh!

adrian-prantl commented Aug 12, 2020

Uh oh!

adrian-prantl commented Aug 25, 2020

Uh oh!

adrian-prantl commented Aug 28, 2020

Uh oh!

adrian-prantl commented Aug 28, 2020

Uh oh!

adrian-prantl commented Aug 28, 2020

Uh oh!

adrian-prantl commented Aug 28, 2020

Uh oh!

kastiglione left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrian-prantl commented Sep 1, 2020

Uh oh!

adrian-prantl commented Sep 4, 2020

Uh oh!

adrian-prantl commented Sep 4, 2020

Uh oh!

adrian-prantl commented Sep 4, 2020

Uh oh!

adrian-prantl commented Sep 4, 2020

Uh oh!

adrian-prantl commented Sep 4, 2020

Uh oh!

adrian-prantl commented Sep 5, 2020

Uh oh!

adrian-prantl commented Sep 6, 2020

Uh oh!

kastiglione commented Sep 10, 2020

Uh oh!

adrian-prantl commented Sep 10, 2020 via email

Uh oh!

kastiglione commented Sep 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kastiglione Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kastiglione Sep 10, 2020 •

edited

Loading