[lldb] Implement LLDBMemoryReader::resolvePointer #3848

kastiglione · 2022-01-26T21:48:58Z

Override MemoryReader::resolvePointer in LLDBMemoryReader.

This function lets a memory reader implementation optionally convert an address into a symbol. This functionality is used in places where MetadataReader can benefit from knowing the symbol for an address. A Swift symbol can provide extra (but optional) data to MetadataReader. In particular, profiling has shown it to be quite costly for MetadataReader to load context descriptors, build a demangle node tree from those descriptors, and finally mangling it to a symbol. This work to compute the symbol can be avoided by implementing resolvePointer.

adrian-prantl

This awesome! Do you agree that we should cherry-pick this to the swift/release/5.6 branch?

kastiglione · 2022-01-26T22:52:17Z

I have been profiling this in combination with #3800. I will gather some profile data for this patch alone, to help us decide about 5.6. In the mean time, I will open a PR and run the tests.

kastiglione · 2022-01-27T16:18:01Z

test/Shell/Swift/runtime-initialization.test is failing, by not resolving the dynamic type of a variable from the command target var.

kastiglione · 2022-01-27T17:41:45Z

In my current "perf harness", I found that during the first call to GetBitSize, this patch reduces the number of process reads by 55%:

Calls to ReadMemoryFromInferior:
Before: 5166
After: 2346

kastiglione · 2022-01-27T23:57:28Z

The test failure shines a light on something I found confusing: the name resolvePointer. Providing a symbol for an address didn't strike me as necessarily "resolving" the pointer. In fact, the type RemoteAbsolutePointer returned by resolvePointer has a method named isResolved() which only true if there's no symbol.

To put it another way, resolvePointer is given an address of a pointer, and this function can optionally return a symbol instead. But if it does return a symbol, the resulting value has isResolved() == false. This seems like "unresolving" the pointer. Does this mean the function should expect only a symbol to be given, and then the symbol should be resolved?

What does this have to do with this test failure? Well some code calls resolvePointer and then immediately checks isResolved(). For example:

llvm::Optional<StoredPointer>
readResolvedPointerValue(StoredPointer address) {
  if (auto pointer = readPointer(address)) {
    if (!pointer->isResolved())
      return None;
    return (StoredPointer)pointer->getResolvedAddress().getAddressData();
  }
  return None;
}

I wonder if there's a simple fix: instead of returning None if the pointer is not "resolved", return the address we already have. I am checking now to find out.

kastiglione · 2022-01-27T23:58:55Z

The "simple fix" does work, I'll have to get feedback/review and run tests.

adrian-prantl · 2022-01-28T01:04:15Z

I wonder if there's a simple fix: instead of returning None if the pointer is not "resolved", return the address we already have. I am checking now to find out.

In a live process resolvePointer is an identity function, because all pointers are in memory that has been patched by dyld to resolve to the correct address. So if you're patching the if(!resolved) path any breakage would surface either in swift-reflection-dump or lldb.

The function is used only(?) here:

  llvm::Optional<std::pair<const TypeRef *, RemoteAddress>>
  getDynamicTypeAndAddressClassExistential(RemoteAddress ExistentialAddress) {
    auto PointerValue =
        readResolvedPointerValue(ExistentialAddress.getAddressData());

I originally thought an ExistentialAddress would never be a symbol, because it points to the address of a live object in heap or stack. But the failing testcase is a global variable, and that could actually have a symbol which LLDB finds, but which we would not want to return in LLDBMemoryReader. LLDBMemoryReader reads live process memory and reflection metdata and we only want symbols in the latter case.

adrian-prantl · 2022-01-28T01:06:06Z

So an alternative fix in LLDB would be to check if the symbol is in a reflection section or in __DATA,const and explicitly not in the rest of __DATA.

kastiglione · 2022-01-28T01:20:36Z

Yes, with a bit of thinking through this, I now understand this better. Your comment helps clarify. I have added an isEffectivelyReadOnly function and only return a symbol if that's true.

adrian-prantl · 2022-01-28T01:44:54Z

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

+    if (section->GetName() == "__const")
+      return true;
+    if (auto segment = section->GetParent())
+      if (segment->GetName() == "__DATA_CONST")


We shouldn't be hardcoding implementation details of the MachO format here, but I'm sure we can sink this into Target or somewhere more appropriate

thanks I am on the same page, I put this change together quickly primarily to test asynchronously.

augusto2112

🎉🎉🎉

kastiglione · 2022-01-29T04:16:47Z

@shafik I have added a virtual function (bool CanContainSwiftReflectionData(const Section &)). I don't know why the base class implementation is being called instead of the expected overrides. Have I made a bonehead mistake?

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

shafik · 2022-01-30T01:56:19Z

lldb/source/Core/Section.cpp

@@ -390,6 +390,10 @@ void Section::SetPermissions(uint32_t permissions) {
  m_executable = (permissions & ePermissionsExecutable) != 0;
 }

+bool Section::CanContainSwiftReflectionData() const {


Maybe GetCanContainSwiftReflectionData having this function be the same name feels confusing especially in this PR. e.g. we can see below GetSectionData calls m_obj_file->ReadSectionData.

Don't suppose you have alternative naming suggestions? I would say that Can is like Is or Has, which are not conventionally combined with Get. Using same name was by design, the idea is to connect them by name. If that's confusing, then I am open to other names, but I don't think GetCan… should be it.

shafik · 2022-01-30T02:03:31Z

lldb/include/lldb/Symbol/ObjectFile.h

@@ -689,6 +690,8 @@ class ObjectFile : public std::enable_shared_from_this<ObjectFile>,
  virtual llvm::StringRef
  GetReflectionSectionIdentifier(swift::ReflectionSectionKind section);

+  virtual bool CanContainSwiftReflectionData(const Section &section);


I see we assert false in the implementation why don't we make it pure virtual using =0 instead? I see we use a mix in this class. Detecting we have not implemented an override at runtime seems less ideal.

I gave it a default behavior of return false, instead of making it pure virtual.

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

kastiglione · 2022-01-31T23:14:40Z

nice, passing!

kastiglione · 2022-02-01T19:11:55Z

@swift-ci test

…esolvePointer [lldb] Implement LLDBMemoryReader::resolvePointer (cherry picked from commit bb7505d)

adrian-prantl · 2022-02-09T22:22:11Z

Looking at the list of individual commits, this PR would have been a great candidate for squashing into a single commit. I'm all for always writing the smallest possible patch, but only if each patch can stand for itself and has a meaningful commit message.

kastiglione · 2022-02-10T00:44:30Z

@JDevlieghere also brought this up. I will squash in the future.

…esolvePointer [lldb] Implement LLDBMemoryReader::resolvePointer (cherry picked from commit bb7505d) (cherry picked from commit 201ddc5)

kastiglione requested review from adrian-prantl and augusto2112 January 26, 2022 21:48

adrian-prantl approved these changes Jan 26, 2022

View reviewed changes

adrian-prantl reviewed Jan 28, 2022

View reviewed changes

augusto2112 approved these changes Jan 28, 2022

View reviewed changes

kastiglione requested a review from shafik January 29, 2022 04:14

augusto2112 requested changes Jan 29, 2022

View reviewed changes

lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp Outdated Show resolved Hide resolved

shafik reviewed Jan 30, 2022

View reviewed changes

kastiglione requested a review from augusto2112 January 31, 2022 18:16

augusto2112 approved these changes Jan 31, 2022

View reviewed changes

kastiglione added 9 commits February 1, 2022 07:19

[lldb] Implement LLDBMemoryReader::resolvePointer

c8b9012

only return symbols for read-only sections

51415bc

get bit logic right

c101dcd

second try

df3589c

add and use CanContainSwiftReflectionData

eabe333

s/COFF/ELF/

3a4e37a

make CanContainSwiftReflectionData conditional on LLDB_ENABLE_SWIFT

59d88f0

tidying

1060cf3

add swift-use-reflection-symbols experimental setting (default true)

9f230a1

kastiglione force-pushed the lldb-Implement-LLDBMemoryReader-resolvePointer branch from 148d7c7 to 9f230a1 Compare February 1, 2022 15:21

kastiglione added 2 commits February 1, 2022 07:35

incorporate resolveRemoteAddress

23b2a50

separately handle tagged and real addresses

845de1c

kastiglione merged commit bb7505d into stable/20211026 Feb 2, 2022

kastiglione deleted the lldb-Implement-LLDBMemoryReader-resolvePointer branch February 2, 2022 02:04

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

Merge pull request #3848 from apple/lldb-Implement-LLDBMemoryReader-r…

201ddc5

…esolvePointer [lldb] Implement LLDBMemoryReader::resolvePointer (cherry picked from commit bb7505d)

[lldb] Implement LLDBMemoryReader::resolvePointer #3848

[lldb] Implement LLDBMemoryReader::resolvePointer #3848

Uh oh!

Conversation

kastiglione commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

Uh oh!

kastiglione commented Jan 26, 2022

Uh oh!

kastiglione commented Jan 27, 2022

Uh oh!

kastiglione commented Jan 27, 2022

Uh oh!

kastiglione commented Jan 27, 2022

Uh oh!

kastiglione commented Jan 27, 2022

Uh oh!

adrian-prantl commented Jan 28, 2022

Uh oh!

adrian-prantl commented Jan 28, 2022

Uh oh!

kastiglione commented Jan 28, 2022

Uh oh!

adrian-prantl Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

kastiglione Jan 28, 2022 • 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!

augusto2112 left a comment

Choose a reason for hiding this comment

Uh oh!

kastiglione commented Jan 29, 2022

Uh oh!

Uh oh!

shafik Jan 30, 2022

Choose a reason for hiding this comment

Uh oh!

kastiglione Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

shafik Jan 30, 2022

Choose a reason for hiding this comment

Uh oh!

kastiglione Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kastiglione commented Jan 31, 2022

Uh oh!

kastiglione commented Feb 1, 2022

Uh oh!

adrian-prantl commented Feb 9, 2022

Uh oh!

kastiglione commented Feb 10, 2022

Uh oh!

Uh oh!

kastiglione commented Jan 26, 2022 •

edited

Loading

kastiglione Jan 28, 2022 •

edited

Loading