[lldb] Add enum support to TypeSystemSwiftTypeRef::DumpTypeValue #2330

kastiglione · 2021-01-14T00:15:55Z

Complete the implementation of TypeSystemSwiftTypeRef::DumpTypeValue by adding enum support.

rdar://68171421

kastiglione · 2021-01-14T00:20:34Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

+GetMultiPayloadEnumCaseName(const swift::reflection::EnumTypeInfo *eti,
+                            const DataExtractor &data) {
+  using namespace swift::reflection;
+  assert(eti->getEnumKind() == EnumKind::MultiPayloadEnum);
+  const auto &cases = eti->getCases();
+  auto it = std::max_element(cases.begin(), cases.end(),
+                             [](const auto &a, const auto &b) {
+                               return a.TI.getSize() < b.TI.getSize();
+                             });
+  if (it == cases.end())
+    return {};
+
+  auto payload_capacity = it->TI.getSize();
+  if (data.GetByteSize() == payload_capacity + 1) {
+    auto tag = data.GetDataStart()[payload_capacity];
+    if (tag >= 0 && tag < cases.size())
+      return cases[tag].Name;
+  }
+
+  return {};
+}


@adrian-prantl @tbkka this is the manual implementation of projecting multipayload enums, where just the case name is taken.

This looks like the right idea. As long as you use this sparingly (ideally, only for certain well-known types), I would call it a reasonable workaround until the better solution I've been chipping away at is complete.

In particular, as you and I have discussed, the compiler uses a variety of layout strategies; this only supports the simplest one. The "better solution" I've been working on is taking a while precisely because I need to know which strategy the compiler used and accurately support each one.

kastiglione · 2021-01-14T00:33:50Z

This is draft because there are still issues to be resolved.

kastiglione · 2021-01-14T04:18:05Z

@swift-ci test

adrian-prantl · 2021-01-14T18:24:17Z

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

@@ -2226,9 +2226,6 @@ CompilerType TypeSystemSwiftTypeRef::GetChildCompilerTypeAtIndex(
          if (llvm::StringRef(AsMangledName(type))
                  .endswith("sSo18NSNotificationNameaD"))
            return GetTypeFromMangledTypename(ConstString("$sSo8NSStringCD"));
-          // FIXME: Private discriminators come out in a different format.


How was this resolved in this patch?

It's not entirely resolved, more changes required, one at least is to support validation. But it's removal is required to support Foundation.IndexPath.Storage whose type would be ignored by this condition.

See here: #2330 (comment)

adrian-prantl

One question inline but otherwise this looks good!

kastiglione · 2021-01-14T21:10:34Z

This depends on swiftlang/swift#35433

kastiglione · 2021-01-15T00:07:20Z

TODO

Commit [Reflection] Support OpaqueExistential in RecordTypeInfo::readExtraInhabitantIndex swift#35433
Figure out how to handle clang enums represented as swift structs
Fix TestEnumVariables
Limit GetMultiPayloadEnumCaseName to known types
Determine if there are any additional Foundation (or SDK) enums that need to be special cased

kastiglione · 2021-01-15T16:39:53Z

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

+StripPrivateIDs(swift::Demangle::Demangler &dem,
+                swift::Demangle::NodePointer node) {
+  using namespace swift::Demangle;
+  return TypeSystemSwiftTypeRef::Transform(dem, node, [&](NodePointer node) {
+    if (node->getKind() != Node::Kind::PrivateDeclName ||
+        node->getNumChildren() != 2)
+      return node;
+
+    assert(node->getFirstChild()->getKind() == Node::Kind::Identifier);
+    assert(node->getLastChild()->getKind() == Node::Kind::Identifier);
+    auto *new_node = dem.createNode(Node::Kind::PrivateDeclName);
+    auto *ident = dem.createNodeWithAllocatedText(
+        Node::Kind::Identifier, node->getLastChild()->getText());
+    new_node->addChild(ident, dem);
+    return new_node;


This function strips the private discriminators (identifiers) from a demangled symbol. This function is used when comparing symbols in Equivalent, so that two symbols that differ only by the unique identifier in PrivateDeclName, are considered equal.

Would this hide bugs where we accidentally mix up these two types named Foo?

a.swift
fileprivate struct Foo {}
b.swift
fileprivate struct Foo { let i = "i am different" }

What I want to say is: we need to make sure to not forget to investigate why private discriminators reported by remote mirrors look different than the ones the Swift compiler emits in the debug info.
But that's not something blocking this patch. (But maybe there should be a bug FIXME in the code).

good catch. I'll have to constrain this more.

I ended up asking about this, and the explanation is:

the private identifier is not ABI and doesn't show up in any runtime data structures. the $ is the address of the anonymized context descriptor that represents the private scope. there are no stable string identifiers for private or local types

For the purpose of verification, it's okay to compare tham in a relaxed manner. But for correctness, this is a potential problem with our incremental adoption of TypeSystemSwiftTypeRef. Since we mix&match between type systems, this makes it impossible to convert between AST <-> TypeRef types with private discriminators unless we have a ValueObject with an AST type to bind the two together.

We do store the private discriminator of each source file in DWARF (as a DW_TAG_namespace). If there is a way to tie a typeref type to a DWARF compile unit, we could do the conversion from runtime -> ast discriminator. But I don't think this is going to be possible in all cases: For example when using remote mirrors to resolve the dynamic type of an existential, how would we know where that type was defined?

Is my understanding here correct?

This is a problem for private types which lldb has the AST for. For the SDK, we'll never have private types, so the issue of AST <-> TypeRef conversion can't occur for SDK types.

For project code, there is code that's built using full swiftmodules, and code that's builds using library evolution and swiftinterfaces. For builds using full fidelity swiftmodules, can or should lldb use the AST only? If AST were strictly used when it's available, and TypeRefs used only when the AST is not available, then would we never need to worry about conversion? However, would we be able to recognize the difference between a full swiftmodule, and a swiftmodule generated from a swiftinterface?

I'm not sure whether the TypeRef can be cross referenced to a compile unit. I'll have to look into that.

I wonder if we can cross reference through uniqueness? If, ignoring the private identifier, there's only a single type by that name (unlike say your fileprivate struct Foo example), then if we enumerated all types we would see that there's just the one. However that sounds heavy handed and poor for performance/maintenance.

Since we mix&match between type systems

do we need to do this, and if so why? To put it another way, if we get a private TypeRef, when might lldb try to use that type in an AST context, vs always using reflection for such types? In other words, once you go TypeRef, do you ever need to go back to AST?

Is my understanding here correct?

This is a problem for private types which lldb has the AST for. For the SDK, we'll never have private types, so the issue of AST <-> TypeRef conversion can't occur for SDK types.

No Swift ASTs are shipped for the SDK overlays. Therefore, we may get private types from Remote Mirrors, but never in the ASTs. This same is not true for development builds where we build the older snapshot of the Swift overlays that is inside the apple/swift repository from source.

The situation for SDK types could arise in the testsuite but not in production.

For project code, there is code that's built using full swiftmodules, and code that's builds using library evolution and swiftinterfaces. For builds using full fidelity swiftmodules, can or should lldb use the AST only? If AST were strictly used when it's available, and TypeRefs used only when the AST is not available, then would we never need to worry about conversion? However, would we be able to recognize the difference between a full swiftmodule, and a swiftmodule generated from a swiftinterface?

Here is gets much more complicated. In the expression evaluator all type lookup requests from the Swift compiler get sent back to LLDB, and it's LLDB's job to resolve type names. LLDB might perform dynamic type resolution (using Remote Mirrors) here and then could try to reconstruct a type that does not exist in the AST world. We also can run into the exact same issue with Clang types. Dynamic type resolution can return a private implementation subclass (e.g., NSString has multiple internal implementations) that the AST cannot know about because its declaration is not part of the SDK at all.
In the case of private subclasses, we could try reconstructing more general types (superclasses) until we find one that can be reconstructed. But this does not work for all private types in general. LLDB would probably need to make up a declaration for these types and hand it to the AST.

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

kastiglione · 2021-01-15T16:45:06Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

@@ -292,7 +292,7 @@ class LLDBMemoryReader : public swift::remote::MemoryReader {
      } else {
        *result = 0;
      }
-      break;
+      return true;


Swift reflection code expects this function to return true when a valid value is assigned to outBuffer.

kastiglione · 2021-01-15T16:45:51Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

+    case DLQ_GetLeastValidPointerValue: {
+      auto *result = (uint64_t *)outBuffer;
+      auto &triple = m_process.GetTarget().GetArchitecture().GetTriple();
+      if (triple.isOSDarwin() && triple.isArch64Bit())
+        *result = 0x100000000;
+      else
+        *result = 0x1000;
+      return true;


This was previously unimplemented, and so would return false for DLQ_GetLeastValidPointerValue queries. This caused projectEnumValue in Swift reflection to fail.

See also rdar://70729149

kastiglione · 2021-01-15T16:49:50Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

    m_local_buffer_size = 0;
  }

 private:
  Process &m_process;
  size_t m_max_read_amount;

-  uint64_t m_local_buffer = 0;
+  llvm::Optional<uint64_t> m_local_buffer;


Some enum values are zero sized. Because of the asserts in this function, PushLocalBuffer() previously should be only called if the buffer was non-zero. This means you'd have to write code like:

if (buffer) pushLocalBuffer(...); DO STUFF if (buffer) popLocalBuffer();

By using optional, the calling code can be made unconditional:

pushLocalBuffer(...); DO STUFF popLocalBuffer();

This works better with make_scope_exit too.

kastiglione · 2021-01-15T16:51:58Z

lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp

+  // Temporary workaround.
+  if (eti->getEnumKind() == EnumKind::MultiPayloadEnum &&
+      type.GetMangledTypeName().GetStringRef().startswith(
+          "$s10Foundation9IndexPathV7Storage10"))
+    return GetMultiPayloadEnumCaseName(eti, data);


The work around is currently limited to Foundation.IndexPath.Storage, because that's the only case surfaced by the lldb test suite. I'd like this to be exhaustive, but I'll have to do some work to identify other any other multipayload enums in Foundation that we need to test and hardcode a special case for.

kastiglione · 2021-01-15T16:55:34Z

@adrian-prantl This has changed a fair bit since your original accept, would you mind another review?

In particular I've left a bunch of PR comments to draw attention new changes that you may bring up ideas/opinions/problems.

lldb/include/lldb/Target/SwiftLanguageRuntime.h

adrian-prantl · 2021-01-15T17:30:16Z

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

    case Node::Kind::BoundGenericStructure:
      return false;
+    case Node::Kind::Structure: {
+      // In some instances, a swift `structure` wraps an objc enum. The enum


do we know what "some instances" are?

There are many instances. I'm not currently sure which objc enums are mangled as structs, and which aren't, nor why. It's something I could look into.

Let's save that investigation for another day.

kastiglione · 2021-01-16T01:20:03Z

@swift-ci test

kastiglione · 2021-01-16T18:06:28Z

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

-          if (result.GetMangledTypeName().GetStringRef().count('$') > 1)
-            return fallback();


Here's a thought. In this case, we could have two levels of "fallback" logic:

if TypeRef type contains '$' if AST has type return AST type else return TypeRef type

I've had some second thoughts about this fallback logic. A possible deal breaker is that the AST could contain fewer children, and that would make the index invalid. In other words, a runtime index could return some other child in the AST. To proceed, we'd have to compare the AST child to see that it matches the runtime child, modulo the private identifier, but that style of comparison is in-exact which could cause rare bugs.

Could we add a if GetNumChildren(typeref) == GetNumChildren(ast) condition to the fallback and error out otherwise?

I added the fallback with a child count check in 3dad37a.

kastiglione · 2021-01-21T02:03:51Z

@swift-ci test

kastiglione · 2021-01-21T18:51:23Z

@swift-ci test

kastiglione · 2021-01-21T23:29:06Z

@swift-ci test

kastiglione marked this pull request as draft January 14, 2021 00:16

kastiglione requested review from tbkka and adrian-prantl January 14, 2021 00:18

kastiglione commented Jan 14, 2021

View reviewed changes

tbkka approved these changes Jan 14, 2021

View reviewed changes

adrian-prantl reviewed Jan 14, 2021

View reviewed changes

adrian-prantl approved these changes Jan 14, 2021

View reviewed changes

kastiglione commented Jan 15, 2021

View reviewed changes

kastiglione requested a review from adrian-prantl January 15, 2021 16:53

kastiglione marked this pull request as ready for review January 15, 2021 16:57

adrian-prantl reviewed Jan 15, 2021

View reviewed changes

lldb/include/lldb/Target/SwiftLanguageRuntime.h Show resolved Hide resolved

adrian-prantl reviewed Jan 15, 2021

View reviewed changes

kastiglione added 12 commits January 15, 2021 17:12

[lldb] Add enum support to TypeSystemSwiftTypeRef::DumpTypeValue

ccfcdc6

fix issues in LLDBMemoryReader::queryDataLayout

c4fbbcc

make m_local_buffer Optional, to allow for 0-sized buffers

0d1bead

strip PrivateDeclNames before validating swift symbols

dae5d8a

fallback to SwiftASTContext for unsupported cases

48fd6e6

improve handling of clang imported enums

a976288

strip only first PrivateDeclName child

a9ded5d

remove TypeInfoKind::Enum assert

df36fa0

limit multipayload workaround to Foundation.IndexPath

0e28747

rearrange assert condition

32e0a23

reenable a test expect

2ce9bbd

add GetEnumCaseName docstring

3e6a52c

kastiglione force-pushed the lldb-Add-enum-support-to-TypeSystemSwiftTypeRef-DumpTypeValue branch from 037c790 to 3e6a52c Compare January 16, 2021 01:12

simplify validation

83ef63f

kastiglione commented Jan 16, 2021

View reviewed changes

skip IndexPath test on linux

511d858

improve fallback logic for private child types

3dad37a

This was referenced Jan 22, 2021

[lldb] Add enum support to TypeSystemSwiftTypeRef::DumpTypeValue #2361

Merged

[lldb] Add enum support to TypeSystemSwiftTypeRef::DumpTypeValue #2362

Merged

kastiglione merged commit b428d0d into swift/release/5.4 Jan 22, 2021

kastiglione deleted the lldb-Add-enum-support-to-TypeSystemSwiftTypeRef-DumpTypeValue branch January 22, 2021 06:10

		if (result.GetMangledTypeName().GetStringRef().count('$') > 1)
		return fallback();

[lldb] Add enum support to TypeSystemSwiftTypeRef::DumpTypeValue #2330

[lldb] Add enum support to TypeSystemSwiftTypeRef::DumpTypeValue #2330

Conversation

kastiglione commented Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kastiglione commented Jan 14, 2021

kastiglione commented Jan 14, 2021

Choose a reason for hiding this comment

kastiglione Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrian-prantl left a comment

Choose a reason for hiding this comment

kastiglione commented Jan 14, 2021

kastiglione commented Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kastiglione commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kastiglione commented Jan 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kastiglione commented Jan 21, 2021

kastiglione commented Jan 21, 2021

kastiglione commented Jan 21, 2021

kastiglione commented Jan 14, 2021 •

edited

Loading

kastiglione Jan 14, 2021 •

edited

Loading

kastiglione commented Jan 15, 2021 •

edited

Loading