[lldb] Register reflection images with LLDBMemoryReader #3800

augusto2112 · 2022-01-14T19:20:03Z

No description provided.

augusto2112 · 2022-01-14T19:21:56Z

adrian-prantl

This looks really promising! For some reason the patch is quite large, I wonder if we could frontload a few NFC refactors and get the distracting changes out of the way first.

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

adrian-prantl · 2022-01-14T21:24:25Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

+      [&](swift::ReflectionSectionKind section_kind)
+          -> std::pair<swift::remote::RemoteRef<void>, uint64_t> {
+        auto section_name =
+            ConstString(obj_file_format->getSectionName(section_kind));


why is the ConstString necessary here?

Because there's no ConstString overload that compares a ConstString to a StringRef.

It would be more efficient to use if (section->GetName().AsStringRef() == section_name) than looking up and potentially allocating a new ConstString.

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntimeDynamicTypeResolution.cpp

adrian-prantl · 2022-01-14T21:51:52Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntimeDynamicTypeResolution.cpp

+        // The start of the masked address is right after the previous
+        // registered address,
+        // so subtract it and 1 to get to the file address.
+        file_address = address - (pair_iterator - 1)->first - 1;


Does this check that the address actually falls inside of the image that lower_bound() returned?

Yes, by checking the iterator in the beginning: pair_iterator != m_address_map.end

augusto2112 · 2022-01-17T15:03:02Z

@swift-ci test

augusto2112 · 2022-01-26T16:22:01Z

Please test with following pull request:
swiftlang/swift#41021

preset=lldb-pull-request
@swift-ci Please test with preset macOS Platform

augusto2112 · 2022-01-26T16:22:27Z

Please test with following pull request:
swiftlang/swift#41021

preset=lldb-pull-request
@swift-ci Please test with preset Linux Platform

adrian-prantl · 2022-01-26T19:17:35Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

@@ -134,15 +134,21 @@ bool LLDBMemoryReader::readBytes(swift::remote::RemoteAddress address,
  LLDB_LOGV(log, "[MemoryReader] asked to read {0} bytes at address {1:x}",
            size, address.getAddressData());

+  Address addr = Address(address.getAddressData());
+  if (auto resolved_addr = tryToConvertFromMappedAddressIntoRealAddress(


Are there more specific terms that we could use instead of Mapped and Real? Is Real maybe a FileAddress (which would be a term of art in LLDB that is well understood?

Real would be an instance of Address which Target knows how to read (in this case, we convert it to the section + offset form of an Address). Not sure what the best name would be, I've been using "artificial" on the documentation to describe these fake addresses, so maybe tryToConvertFromArtificialToLLDBAddress?

augusto2112 · 2022-01-26T19:27:52Z

Please test with following pull request:
swiftlang/swift#41021

@swift-ci Please test

adrian-prantl · 2022-01-26T19:21:50Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+
+  for (auto swift_mask : used_swift_masks) {
+    assert((next_available_address & swift_mask) != swift_mask &&
+           "LLDB artificial address clashes with a swift mask!");


neat!
If we use static_assert, maybe this could even be a constexpr and be checked even if assertions are disabled.

Unfortunately this can't can't be be a const_expr since next_available_address depends on the size of the images at runtime.

IIUC, what we want to check is that the 0x2000000000000000 constant will never appear in a pointer that would appear in the process. For that we just need to check that it's contained in SWIFT_ABI_ARM64_SWIFT_SPARE_BITS_MASK.
So a static_assert(0x2000000000000000 & SWIFT_ABI_ARM64_SWIFT_SPARE_BITS_MASK == 0x2000000000000000) should be sufficient for that purpose. (Repeat with all other platforms).

adrian-prantl · 2022-01-26T19:24:52Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+    // bit set.
+    next_available_address = m_address_map.back().first;
+    // Add 8 to it to keep it pointer aligned. This assumes the previous address
+    // was also pointer aligned.


As it's written, this comment doesn't really make sense, because this only works if the address was already aligned and then adding 8 doesn't change anything in terms of aligned-ness. SO either the comment is wrong or this should be a call to llvm::alignTo() from MathExtras.h.

It should work as the size of the previous image is aligned, I wasn't aware of llvm::alignTo() though, I'll use that instead.

I changed the bit manipulation further down to use llvm::alignTo(), but decided to keep the plus 8 here, as it's actually clearer to understand, but changed the code and comments to clarify that we add 8 to the end of the previous image's address (which is already pointer aligned).

adrian-prantl · 2022-01-26T19:26:13Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.h

 private:
  Process &m_process;
  size_t m_max_read_amount;

  llvm::Optional<uint64_t> m_local_buffer;
  uint64_t m_local_buffer_size = 0;
+  // A map from "artificial" addresses that we hand out to modules that
+  // LLDBMemoryReader will read from.
+  std::vector<std::pair<uint64_t, lldb::ModuleSP>> m_address_map;


Since this is a vector, should this be m_address_list, or even better m_section_ranges? Can you document that this is guaranteed to be sorted?

Even though it's a vector, we use it as a map from ranges -> modules, so what about m_range_module_map?

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

adrian-prantl · 2022-01-26T19:30:01Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+}
+
+llvm::Optional<Address>
+LLDBMemoryReader::tryToConvertFromMappedAddressIntoRealAddress(


It seems like every call site just unwraps the optional and assigns it to the original value. Maybe this function should return the original address if it failed?

Maybe call it resolve..?

I could do that, but I personally like to make it explicit that the function may fail...

I'll change the name though.

adrian-prantl · 2022-01-26T19:30:25Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+    return {};
+
+  // If the address contains our mask, this is an image we registered.
+  if (address & 0x2000000000000000) {


the 0x20000 should be a constant declared only once

adrian-prantl · 2022-01-26T19:31:16Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+    auto comparison_pair = std::make_pair(address, ModuleSP());
+    auto pair_iterator = std::lower_bound(m_address_map.begin(),
+                                          m_address_map.end(), comparison_pair);
+    if (pair_iterator != m_address_map.end()) {


std::lowerbound could return an entry that doesn't contain the searched address, since it returns the insert point for that value. Can you add error handling for that?

Sorry, I don't understand, in what situation can that happen?

augusto2112 · 2022-01-26T21:14:46Z

swiftlang/swift#41021

@swift-ci test

adrian-prantl

I think this is in a shape where we can land this once all outstanding comments are addressed. Thanks!

adrian-prantl · 2022-01-26T21:18:47Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.h

+  llvm::Optional<std::pair<uint64_t, uint64_t>> addModuleToAddressMap(lldb::ModuleSP module);
+
+private:
+  /// Given a possibly artificial address, try to map it back into an Address.


Naming suggestion: replace artificial with tagged?

adrian-prantl · 2022-01-26T21:23:57Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+      // registered address,
+      // subtract it and 8 (pointer alignment) to get to the virtual file
+      // address.
+      file_address = address - (pair_iterator - 1)->first - 8;


personally I find std::prev(pair_iterator) more readable than pair_iterator-1. Up to you.

adrian-prantl · 2022-01-26T21:33:03Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

@@ -134,15 +136,21 @@ bool LLDBMemoryReader::readBytes(swift::remote::RemoteAddress address,
  LLDB_LOGV(log, "[MemoryReader] asked to read {0} bytes at address {1:x}",
            size, address.getAddressData());

+  Address addr = Address(address.getAddressData());


Suggestion:
we could change resolveFromArtificialToLLDBAddress to

lldb::Address LLDBMemoryReader::resolveRemoteAddress(uint64_t address) const { }

and have it return lldb::Address(address) in the error path. Then this whole block would become

lldb::Address addr = resolveRemoteAddress(address.getAddressData());

adrian-prantl · 2022-01-26T21:41:25Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+    if (pair_iterator == m_range_module_map.begin())
+      // Since this is the first registered module,
+      // just unsetting the mask will give the virtual file address.
+      file_address = address & ~ARTIFICIAL_ADDRESS_MASK;


There needs to be a check:

if (file_address < _pair_iterator.first) { LLDB_LOG(log, "[MemoryReader] Address is tagged, but doesn't exist in m_range_module_map"); return {} }

Similarly we could also store the last valid virtual address and have a sanity upper bounds check as well

In the map, we store the end address, not start address, of each image, std::lower_bound: "Returns an iterator pointing to the first element in the range [first, last) that is not less than (i.e. greater or equal to) value, or last if no such element is found."

Let's say that the fist image goes from 0x200...00 to 0x200..10, and we're looking for a tagged address 0x200..05. We'll end in this first case, and because we know the address is tagged, we know for sure it's in range.

For the upper bound, we already check pair_iterator != m_range_module_map.end(), m_range_module_map.end() is what will be return if the address we're looking for is larger than anything in the map.

adrian-prantl · 2022-01-26T21:48:03Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

+      [&](swift::ReflectionSectionKind section_kind)
+          -> std::pair<swift::remote::RemoteRef<void>, uint64_t> {
+        auto section_name =
+            ConstString(obj_file_format->getSectionName(section_kind));


It would be more efficient to use if (section->GetName().AsStringRef() == section_name) than looking up and potentially allocating a new ConstString.

adrian-prantl · 2022-01-26T21:51:22Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+    // set.
+    // The start address of the next image is the next available pointer aligned
+    // address (this works because all the end addresses are also pointer
+    // aligned).


Adding +8 does not change the pointer-sized-alignment of an address. If the address was 8-byte aligned before, it's still after, and if it wasn't it won't be 8-byte-aligned afterwards either.
It's also suspicious that the resolve method then subtracts 8 again. Is this necessary at all?

It's necessary because what we're adding 8 to is the end of the previous image. Think about it this way, the first image is mapped in the range [0x2...00 - 0x2...16], we want to map the second image the next pointer aligned address, and we know that the end of the previous image is pointer aligned, so we add 8 to it and get 0x2...24.

Similarly, when we resolve this address back to its virtual address value, we'd like to undo this operation, so we subtract the end of the previous address (0x2...16) and 8, to get back what the original virtual address is without the mask.

adrian-prantl · 2022-01-26T21:52:09Z

lldb/source/Target/TargetProperties.td

@@ -7,6 +7,9 @@ let Definition = "target_experimental" in {
  def SwiftCreateModuleContextsInParallel : Property<"swift-create-module-contexts-in-parallel", "Boolean">,
    DefaultTrue,
    Desc<"Create the per-module Swift AST contexts in parallel.">;
+  def SwiftReadMetadataFromFileCache: Property<"swift-read-metadata-from-file-cache", "Boolean">,
+    DefaultTrue,
+    Desc<"Read metadata from the file cache instead of the process when possible">;


Read Swift reflection metadata ...

augusto2112 · 2022-01-27T15:20:58Z

preset=lldb-pull-request
@swift-ci Please test

augusto2112 · 2022-01-27T19:11:43Z

preset=lldb-pull-request
@swift-ci Please test with preset

augusto2112 · 2022-01-27T19:25:24Z

preset=lldb-pull-request
@swift-ci Please test

adrian-prantl · 2022-01-27T19:26:54Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+              address.getAddressData());
+    return false;
+  }
+  auto addr = *maybeAddr;


This looks good!

adrian-prantl · 2022-01-27T19:31:21Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+      SWIFT_ABI_X86_64_SWIFT_SPARE_BITS_MASK,
+      SWIFT_ABI_X86_64_OBJC_RESERVED_BITS_MASK,
+      SWIFT_ABI_X86_64_SIMULATOR_OBJC_RESERVED_BITS_MASK,
+      SWIFT_ABI_X86_64_IS_OBJC_BIT,


Is this SWIFT_ABI_X86_64_IS_OBJC_BIT included in SWIFT_ABI_X86_64_SWIFT_SPARE_BITS_MASK?

Yes, should we remove it?

I think we only need a static_assert that our tag is inside of SWIFT_ABI_*_SWIFT_SPARE_BITS_MASK, to check that our tag will not collide with real addresses in the process. And then a second check to make sure that our tag does not collide with the OBJC bit to make sure our addresses don't look like OBjC tagged pointers to ReflectionContext.

adrian-prantl · 2022-01-27T19:32:07Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+
+  for (auto swift_mask : used_swift_masks) {
+    assert((module_start_address & swift_mask) != swift_mask &&
+           "LLDB tagged address clashes with a swift mask!");


This is now checking that the start address doesn't use any masked bits. That's good I think we also should check that our own tag bit don't conflict with any runtime bits.

What do you mean by our own tag bit? If TAGGED_ADDRESS_MASK itself conflicts with runtime bits?

adrian-prantl · 2022-01-27T19:33:49Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+    // just unsetting the mask will give the virtual file address.
+    file_address = address & ~TAGGED_ADDRESS_MASK;
+  else
+    // The previous pair's address is the start of the current image.


How about:
// The end of the previous section is the start of the current one.

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.h

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

augusto2112 · 2022-01-27T21:49:45Z

@swift-ci test

adrian-prantl · 2022-01-27T22:06:48Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.h

+  // we can tell what the upper bound of all mapped images is.
+  // Addresses are always inserted in ascending order, so the data structure is
+  // guaranteed to be sorted.
+  std::vector<std::pair<uint64_t, lldb::ModuleSP>> m_range_module_map;


What do you think about?

// LLDBMemoryReader prefers to read reflection metadata from the // binary on disk, which is faster than reading it out of process // memory, especially when debugging remotely. To achieve this LLDB // registers virtual addresses starting at (0x0 & // LLDB_VIRTUAL_ADDRESS_BIT) with ReflectionContext. Sorted by // virtual address, m_lldb_virtual_address_map stores each // lldb::Module and the first virtual address after the end of that // module's virtual address space. std::vector<std::pair<uint64_t, lldb::ModuleSP>> m_lldb_virtual_address_map;

adrian-prantl · 2022-01-27T22:07:35Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.h

 private:
  Process &m_process;
  size_t m_max_read_amount;

  llvm::Optional<uint64_t> m_local_buffer;
  uint64_t m_local_buffer_size = 0;
+
+  // A map from tagged addresses that we hand out to modules that


adrian-prantl · 2022-01-27T22:08:36Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.h

+  // guaranteed to be sorted.
+  std::vector<std::pair<uint64_t, lldb::ModuleSP>> m_range_module_map;
+
+  // Tagged addresses we hand out will contain this mask to indicate that


/// The bit used to tag LLDB's virtual addresses as such. See \c m_lldb_virtual_address_map.

augusto2112 · 2022-01-28T18:39:01Z

@swift-ci test

adrian-prantl · 2022-01-31T17:55:31Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+      m_range_module_map.begin(), m_range_module_map.end(), comparison_pair,
+      [](auto &a, auto &b) { return a.first < b.first; });
+
+  // If the address is larger than anything we have mapped.


This isn't a full sentence :-)

adrian-prantl · 2022-01-31T17:55:58Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+
+  // If the address is larger than anything we have mapped.
+  if (pair_iterator == m_range_module_map.end())
+    return {};


Should this be logged (with LOGV)?

adrian-prantl · 2022-01-31T17:56:51Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+
+  return resolved;
+
+  LLDB_LOGV(log,


This is dead code.

adrian-prantl · 2022-01-31T17:57:33Z

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

+  uint64_t file_address;
+  if (pair_iterator == m_range_module_map.begin())
+    // Since this is the first registered module,
+    // just unsetting the mask will give the virtual file address.


just unsetting the mask will -> clearing the tag bit

adrian-prantl

Two more tiny inline comments, but otherwise this is good!

augusto2112 · 2022-01-31T18:57:50Z

@swift-ci test

augusto2112 · 2022-01-31T20:52:16Z

@swift-ci test

augusto2112 requested review from adrian-prantl and kastiglione January 14, 2022 19:22

adrian-prantl reviewed Jan 14, 2022

View reviewed changes

augusto2112 force-pushed the enable-filecache-again branch from c08ad1e to 188ed2d Compare January 17, 2022 15:02

augusto2112 force-pushed the enable-filecache-again branch 3 times, most recently from fd71269 to 6b3121c Compare January 26, 2022 16:05

augusto2112 changed the base branch from stable/20210726 to stable/20211026 January 26, 2022 16:05

augusto2112 requested a review from jasonmolenda January 26, 2022 16:22

adrian-prantl reviewed Jan 26, 2022

View reviewed changes

augusto2112 force-pushed the enable-filecache-again branch 2 times, most recently from 8d55ee8 to ab7b54a Compare January 26, 2022 21:13

augusto2112 requested a review from adrian-prantl January 26, 2022 21:15

adrian-prantl reviewed Jan 26, 2022

View reviewed changes

kastiglione mentioned this pull request Jan 26, 2022

[lldb] Implement LLDBMemoryReader::resolvePointer #3848

Merged

augusto2112 force-pushed the enable-filecache-again branch from ab7b54a to c151c78 Compare January 27, 2022 15:20

augusto2112 marked this pull request as ready for review January 27, 2022 15:20

augusto2112 force-pushed the enable-filecache-again branch from c151c78 to a665192 Compare January 27, 2022 19:10

adrian-prantl reviewed Jan 27, 2022

View reviewed changes

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.h Outdated Show resolved Hide resolved

adrian-prantl reviewed Jan 27, 2022

View reviewed changes

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp Outdated Show resolved Hide resolved

augusto2112 force-pushed the enable-filecache-again branch from a665192 to c3c8472 Compare January 27, 2022 21:45

adrian-prantl approved these changes Jan 27, 2022

View reviewed changes

augusto2112 force-pushed the enable-filecache-again branch from c3c8472 to 2239b23 Compare January 28, 2022 18:36

adrian-prantl reviewed Jan 31, 2022

View reviewed changes

lldb/source/Plugins/LanguageRuntime/Swift/LLDBMemoryReader.cpp

return resolved;

LLDB_LOGV(log,

Copy link

adrian-prantl Jan 31, 2022

Choose a reason for hiding this comment

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

This is dead code.

adrian-prantl reviewed Jan 31, 2022

View reviewed changes

adrian-prantl approved these changes Jan 31, 2022

View reviewed changes

augusto2112 force-pushed the enable-filecache-again branch from 2239b23 to 91f7f83 Compare January 31, 2022 18:57

[lldb] Register reflection images with LLDBMemoryReader

b5cf6ba

augusto2112 force-pushed the enable-filecache-again branch from 91f7f83 to b5cf6ba Compare January 31, 2022 20:48

augusto2112 merged commit 1e62148 into swiftlang:stable/20211026 Feb 1, 2022

kastiglione added a commit that referenced this pull request Feb 4, 2022

Remove dependency on #3800

417ba5a

[lldb] Register reflection images with LLDBMemoryReader #3800

[lldb] Register reflection images with LLDBMemoryReader #3800

Uh oh!

Conversation

augusto2112 commented Jan 14, 2022

Uh oh!

augusto2112 commented Jan 14, 2022

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

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!

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!

augusto2112 commented Jan 17, 2022

Uh oh!

augusto2112 commented Jan 26, 2022

Uh oh!

augusto2112 commented Jan 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

augusto2112 commented Jan 26, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

augusto2112 commented Jan 26, 2022

Uh oh!

adrian-prantl 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!

Choose a reason for hiding this comment

Uh oh!