[lldb] Assert file cache and live memory are equal on debug builds

[jasonmolenda]

Hm there's no phab action which says "reject approach" :) the perf impact of forcing all reads to be out of memory would be dramatically bad for a remote system debug session, this can't be done. If there is a specific caller that must use live memory even for a section marked "read-only", it needs to use force_live_memory=true in its call to ReadMemory. This change would make app launch time for iOS apps etc probably take 30-60 seconds longer than they do right now.

[jasonmolenda]

I should add — I also don’t think this will fix the bug you’re working on.

[jasonmolenda]

I do like the debug-lldb check that the file cache read matches the live memory part. :)

[augusto2112]

Hi Jason,

I could place this check on the caller, but I think here is the right place for it, maybe I can convince you.

the perf impact of forcing all reads to be out of memory would be dramatically bad for a remote system debug session, this can't be done.

On macOS, this would only affect images that have the LC_SEGMENT_SPLIT_INFO load command. Note that this does not include images that were extracted from the shared cache, as __LINKEDIT is already stripped in that case, so I don't think the performance impact would be that big. For other object files, I could change the default behavior of IsSafeToReadFromFileCache to what we had prior, but, similar to the problem we had on mach-O, there might be situations where the memory diverges that we don't know about.

My reasoning for placing this change here is that I think Target::ReadMemory should be as safe as possible when using the filecache (within reason, if a process changes read-only data mid-execution then that's their responsibility , and they can force a live memory read), this way more callers can benefit from this optimization without having to thoroughly investigate that would be safe for themselves. For example, right now any from a section that contains LC_SEGMENT_SPLIT_INFO could read wrong data, and we would have to reason at every call sites if the default behavior of Target::ReadMemory is a problem or not. Also, many of those call sites are generic, and having to check if this is a problem for a specific object file everywhere would not be very good.

If you're not convinced, I can see a couple of alternatives:

• Instead of passing a boolean determining if we prefer the filecache or memory, pass an enum instead with more options, something like:
  • Default to memory (fallback to filecache).
  • Default to filecache (fallback to memory).
  • Use filecache if deemed safe.
  • Use filecache if read-only.
  • Etc.
• Create another version of "safe" read memory that does what this change does.
• Concentrate this logic of figuring out if the filecache read is safe, so we can reuse it from different call sites.

I should add — I also don’t think this will fix the bug you’re working on.

At the bare minimum it solves rdar://76973844, which I verified. I couldn't verify my bug with a real watch yet but as it stands we're definitely reading wrong memory sometimes.

I do like the debug-lldb check that the file cache read matches the live memory part. :)

At least that we can keep then :) Do you think it's should be a crash, or maybe a log? Wasn't sure what was best.

[vsk]

Hey Augusto, thanks for tackling this, I'm just now slowly paging things in.

Is this a correct statement of the problem: LLDB is failing to disable its file cache optimization when reading writable segments (say, __DATA) from a MachO sourced from the shared cache?

If that's right, then I wonder whether you considered "simply" doing a bounds check on the address? The shared region should be mapped at a fixed virtual range in the debuggee process, and we can determine that range using dyld APIs.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

7040 ↗

(On Diff #360929)

Is LC_SEGMENT_SPLIT_INFO present if and only if the MachO is from the shared cache?

[jasonmolenda]

Sorry for the delay in replying, I needed to think & look at this a bit.

In D106584#2900239, @augusto2112 wrote:

Hi Jason,

I could place this check on the caller, but I think here is the right place for it, maybe I can convince you.

the perf impact of forcing all reads to be out of memory would be dramatically bad for a remote system debug session, this can't be done.

On macOS, this would only affect images that have the LC_SEGMENT_SPLIT_INFO load command.

I tried expanding some shared caches and played around a bit to make sure I understood what we're dealing with. You know all of this, but just to make sure everyone is following along, we have three possible states:

1. Binary on disk, before it gets included in a shared cache. Will have LC_SEGMENT_SPLIT_INFO.
2. Binary embedded in the shared cache binary blog (either on disk or in memory)
3. Binary on disk, extracted from the shared cache. LC_SEGMENT_SPLIT_INFO is absent, cannot be reconstructed from the shared cache binary blob.

With simulator process debugging, the debugger can find the pre-shared-cache distinct binaries on the local system, with the LC_SEGMENT_SPLIT_INFOs.

With iOS debugging, the debugger will have post-shared-cache distinct binaries on the local system, without LC_SEGMENT_SPLIT_INFOs.

I'm not sure what watch debugging looks like -- instead of copying the shared cache off of a watch and expanding it, Xcode downloads a disk image from Apple with the shared cache contents. I don't know if that disk image has pre-shared-cache or post-shared-cache binaries; it's likely that pre-shared-cache binaries are included in the disk image.

In your bug, you've got a table which has relative offsets in it. The offsets are to things in the binary image. They may cross segment boundaries. The binary-on-disk will have TEXT, DATA, and LINKEDIT next to one another. When it's incorporated into the shared cache, the segments will be separated - and these offsets will be updated in the process of creating the shared cache. (are the offsets in the pre-shared-cache binary even valid? Not important) You have a bug where you're reading these offsets and getting them from the pre-shared-cache on-disk-binary, which are incorrect, and want to force reading them from the shared cache memory.

(post-shared-cache on-disk-binaries may have the same offsets as it was in the original shared cache -- but it's actually going to be a MORE subtle bug in that case because people plug in different iPhones and Xcode only extracts the shared cache for one of them if they're all running the same build. But different model iphones will have different shared caches, so using the offsets from the post-shared-cache on-disk-binary-image from one device for another device will be wrong.)

The simulator version of this bug will be fixed, with less perf hit but still probably too much, by ignoring the pre-shared-cache on-disk-binary -- we'll have to read all of the symbol names out of memory via debugserver. If this was over a USB cable, you'd be looking at a minute hit - but for a local debug session, it's probably closer to the order of 5-6 seconds. I'm just guessing, I haven't measured it. But it's a lot of packet traffic.

You'll still have the bug in the scenario outlined above, where I have two different iOS devices running the same iOS build. Xcode will expand the shared cache for the first one I plug in, and lldb will use the same binary images (without LC_SEGMENT_SPLIT_INFOs) for both debug sessions. And it's a questionmark about watch debugging - I suspect those will have LC_SEGMENT_SPLIT_INFOs and so app launch perf would take a gigantic hit to read symbol names all out of memory at debug time.

I think we need to concentrate on special casing the code that reads these offsets, to force it to use live memory every time. I know in theory this problem could happen anywhere -- we could have these cross-segments offsets anywhere in the binary -- but in practice, we know it currently happens in one spot that the debugger looks at.

I admit I'd be a lot happier if we could always use the file cache for read-only Sections and know that the data in the file matches the data in the memory. But if we only have one type of data where this isn't safe (so far?! :), I'm not willing to toss the optimization. At the very least, we'd need to find ways to retain the same perf characteristics as we have right now, and I think that might be tricky.

I do like the debug-lldb check that the file cache read matches the live memory part. :)

At least that we can keep then :) Do you think it's should be a crash, or maybe a log? Wasn't sure what was best.

I think an assert is good, there's no scenario where this should get hit -- if it does, then we really have a bug in using the filecache data and we need to halt immediately in CI or on an lldb developer's desktop. I'm a little less sure about thing this on for DEBUG builds all the time because it does change lldb's perf behavior a lot, specifically adding lots of packet traffic, but I think we should give it a try and see if it turns out to be too much of a change.

[jasonmolenda]

In D106584#2902200, @jasonmolenda wrote:

In D106584#2900239, @augusto2112 wrote:

Hi Jason,

At least that we can keep then :) Do you think it's should be a crash, or maybe a log? Wasn't sure what was best.

I think an assert is good, there's no scenario where this should get hit -- if it does, then we really have a bug in using the filecache data and we need to halt immediately in CI or on an lldb developer's desktop. I'm a little less sure about thing this on for DEBUG builds all the time because it does change lldb's perf behavior a lot, specifically adding lots of packet traffic, but I think we should give it a try and see if it turns out to be too much of a change.

If the perf behavior of lldb changes too much, we could put the "also read from live memory & memcmp against file cache when we use filecache" behind a setting, and set it for testsuite runs, so we get this used in CI and desktop testruns. The extra packet traffic in those cases is going to be less important. Maybe we start with the easy "always enable on DEBUG builds" and fall back to a setting when it turns out to be too much, unless other people have an opinion.

[augusto2112]

In D106584#2901529, @vsk wrote:

Hey Augusto, thanks for tackling this, I'm just now slowly paging things in.

Is this a correct statement of the problem: LLDB is failing to disable its file cache optimization when reading writable segments (say, __DATA) from a MachO sourced from the shared cache?

If that's right, then I wonder whether you considered "simply" doing a bounds check on the address? The shared region should be mapped at a fixed virtual range in the debuggee process, and we can determine that range using dyld APIs.

Hey Vedant! Not exactly. Jason's description is spot on.

@jasonmolenda thanks for putting so much thought into this!

(post-shared-cache on-disk-binaries may have the same offsets as it was in the original shared cache -- but it's actually going to be a MORE subtle bug in that case because people plug in different iPhones and Xcode only extracts the shared cache for one of them if they're all running the same build. But different model iphones will have different shared caches, so using the offsets from the post-shared-cache on-disk-binary-image from one device for another device will be wrong.)

I finally got a watch to test with, and found that the bug still happens on dylibs extracted from the shared cache, and this might be the problem.

I'm updating the patch and keeping only the assert, and think of another way of solving the problem.

lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

7040 ↗

(On Diff #360929)

No, it means that a dylib is eligible for being incorporated in the shared cache later on.

[augusto2112]

Keep only filecache and live memory equality assertion

[jasonmolenda]

Looks good! Maybe we should add a setting like target.file-cache-memory-reads-verify (a boolean) to enable/disable this check instead of keying off NDEBUG. We can turn it on in the testsuite, and we can optionally have people with release installs of lldb enable it if we ever suspect that it might be an issue. The settings are added with an entry in source/Target/TargetProperties.td and a getter/setter in TargetProperties.

[JDevlieghere]

In D106584#2907898, @jasonmolenda wrote:

Looks good! Maybe we should add a setting like target.file-cache-memory-reads-verify (a boolean) to enable/disable this check instead of keying off NDEBUG. We can turn it on in the testsuite, and we can optionally have people with release installs of lldb enable it if we ever suspect that it might be an issue. The settings are added with an entry in source/Target/TargetProperties.td and a getter/setter in TargetProperties.

If we do that we should make it an lldbassert so it prints something in release builds.

[augusto2112]

Add setting to enable assert

[JDevlieghere]

Should this be enabled in the test suite? (packages/Python/lldbsuite/test/lldbtest.py:783, test/Shell/lit-lldb-init.in)

lldb/source/Target/Target.cpp

1773

Can we use a unique_ptr here (std::make_unique<uint8_t[]>)?

[augusto2112]

Run file-cache verification on tests, change raw buffer to unique pointer

[jasonmolenda]

Looks great! Thanks for doing this.

[JDevlieghere]

Seems like this is still missing form lit-lldb-init.in which means the shell tests (and by extension the Swift REPL tests downstream) will run with the setting off. Other than that this LGTM.

[augusto2112]

Add test to test/Shell/lit-lldb-init.in

[augusto2112]

Commit 77e9d10f0fbfe04a14e6ce61753376dd78e0c2f0

[jankratochvil]

It broke some Linux buildbots and it also broke testsuite on my Fedora machine:
functionalities/gdb_remote_client/TestTargetXMLArch.py

• lldb-x86_64-fedora: https://lab.llvm.org/staging/#/builders/16/builds/8988
• lldb-aarch64-ubuntu: https://lab.llvm.org/buildbot/#/builders/96/builds/10245
• lldb-arm-ubuntu: https://lab.llvm.org/buildbot/#/builders/17/builds/9542

Some buildbots do not use -DLLVM_ENABLE_ASSERTIONS=ON which may be the reason why they PASS.

python3.9: /home/jkratoch/redhat/llvm-monorepo/lldb/source/Target/Target.cpp:1781: size_t lldb_private::Target::ReadMemory(const lldb_private::Address &, void *, size_t, lldb_private::Status &, bool, lldb::addr_t *): Assertion `memcmp(live_buf.get(), dst, dst_len) == 0 && "File cache and live memory diverge!"' failed.
(lldb) p dst_len
(size_t) $1 = 1
(lldb) p/x *live_buf.get()
(unsigned char) $2 = 0x00
(lldb) p/x *(uint8_t *)dst
(uint8_t) $3 = 0xc3

  * frame #4: 0x00007fffe119d36a _lldb.so`lldb_private::Target::ReadMemory(this=0x00005555559585e0, addr=0x00005555559ff9a8, dst=0x0000555555922560, dst_len=1, error=0x00007fffffff9088, force_live_memory=false, load_addr_ptr=0x0000000000000000) at Target.cpp:1780:15
    frame #5: 0x00007fffe17d0932 _lldb.so`UnwindAssembly_x86::GetNonCallSiteUnwindPlanFromAssembly(this=0x00005555559117b0, func=0x00005555559ff9a8, thread=0x00005555559eb7a0, unwind_plan=0x00005555559224d0) at UnwindAssembly-x86.cpp:56:31
    frame #6: 0x00007fffe10d42df _lldb.so`lldb_private::FuncUnwinders::GetAssemblyUnwindPlan(this=0x00005555559ff9a0, target=0x00005555559585e0, thread=0x00005555559eb7a0) at FuncUnwinders.cpp:341:32

lldb/include/lldb/Core/Section.h

 //===-- Section.h -----------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_CORE_SECTION_H
 #define LLDB_CORE_SECTION_H
 
 #include "lldb/Core/ModuleChild.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Flags.h"
 #include "lldb/Utility/UserID.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 
public:
 
   // Get the number of host bytes required to hold a target byte
   uint32_t GetTargetByteSize() const { return m_target_byte_size; }
 
   bool IsRelocated() const { return m_relocated; }
 
   void SetIsRelocated(bool b) { m_relocated = b; }
 
+  bool IsReadOnly();
+
 protected:
   ObjectFile *m_obj_file;   // The object file that data for this section should
                             // be read from
   lldb::SectionType m_type; // The type of this section
   lldb::SectionWP m_parent_wp; // Weak pointer to parent section
   ConstString m_name;          // Name of this section
   lldb::addr_t m_file_addr; // The absolute file virtual address range of this
                             // section if m_parent == NULL,

lldb/include/lldb/Target/Target.h

 #define LLDB_TARGET_TARGET_H
 
 #include <list>
 #include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "lldb/Breakpoint/BreakpointList.h"
 #include "lldb/Breakpoint/BreakpointName.h"
 #include "lldb/Breakpoint/WatchpointList.h"
 #include "lldb/Core/Architecture.h"
 #include "lldb/Core/Disassembler.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/UserSettingsController.h"
 #include "lldb/Expression/Expression.h"
 #include "lldb/Host/ProcessLaunchInfo.h"
public:
   bool GetAutoInstallMainExecutable() const;
 
   void UpdateLaunchInfoFromProperties();
 
   void SetDebugUtilityExpression(bool debug);
 
   bool GetDebugUtilityExpression() const;
 
+  void SetVerifyFileCacheMemoryReads(bool debug);
+
+  bool GetVerifyFileCacheMemoryReads() const;
+
 private:
   // Callbacks for m_launch_info.
   void Arg0ValueChangedCallback();
   void RunArgsValueChangedCallback();
   void EnvVarsValueChangedCallback();
   void InputPathValueChangedCallback();
   void OutputPathValueChangedCallback();
   void ErrorPathValueChangedCallback();

lldb/packages/Python/lldbsuite/test/lldbtest.py

def setUpCommands(cls):
             "settings set target.auto-apply-fixits false",
 
             # Testsuite runs in parallel and the host can have also other load.
             "settings set plugin.process.gdb-remote.packet-timeout 60",
 
             'settings set symbols.clang-modules-cache-path "{}"'.format(
                 configuration.lldb_module_cache_dir),
             "settings set use-color false",
+            
+            # Verify that file cache and live memory always match.
+            "settings set target.verify-file-cache-memory-reads true",
         ]
 
         # Set any user-overridden settings.
         for setting, value in configuration.settings:
             commands.append('setting set %s %s'%(setting, value))
 
         # Make sure that a sanitizer LLDB's environment doesn't get passed on.
         if cls.platformContext and cls.platformContext.shlib_environment_var in os.environ:

lldb/source/Core/Section.cpp

 //===-- Section.cpp -------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Core/Section.h"
 #include "lldb/Core/Address.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/VMRange.h"
 
size_t SectionList::Slide(addr_t slide_amount, bool slide_children) {
   size_t count = 0;
   const_iterator pos, end = m_sections.end();
   for (pos = m_sections.begin(); pos != end; ++pos) {
     if ((*pos)->Slide(slide_amount, slide_children))
       ++count;
   }
   return count;
 }
+
+bool Section::IsReadOnly() {
+  auto permissions = Flags(GetPermissions());
+  return !permissions.Test(ePermissionsWritable) &&
+         permissions.Test(ePermissionsReadable);
+}

lldb/source/Target/Target.cpp

 //===-- Target.cpp --------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Target/Target.h"
 #include "lldb/Breakpoint/BreakpointIDList.h"
 #include "lldb/Breakpoint/BreakpointPrecondition.h"
 #include "lldb/Breakpoint/BreakpointResolver.h"
 #include "lldb/Breakpoint/BreakpointResolverAddress.h"
 #include "lldb/Breakpoint/BreakpointResolverFileLine.h"
 #include "lldb/Breakpoint/BreakpointResolverFileRegex.h"
 #include "lldb/Breakpoint/BreakpointResolverName.h"
 #include "lldb/Breakpoint/BreakpointResolverScripted.h"
size_t Target::ReadMemory(const Address &addr, void *dst, size_t dst_len,
   // we keep the result around in this buffer, in case this result is the
   // best we can do.
   std::unique_ptr<uint8_t[]> file_cache_read_buffer;
   size_t file_cache_bytes_read = 0;
 
   // Read from file cache if read-only section.
   if (!force_live_memory && resolved_addr.IsSectionOffset()) {
     SectionSP section_sp(resolved_addr.GetSection());
-    if (section_sp) {
-      auto permissions = Flags(section_sp->GetPermissions());
-      bool is_readonly = !permissions.Test(ePermissionsWritable) &&
-                         permissions.Test(ePermissionsReadable);
-      if (is_readonly) {
-        file_cache_bytes_read =
-            ReadMemoryFromFileCache(resolved_addr, dst, dst_len, error);
-        if (file_cache_bytes_read == dst_len)
-          return file_cache_bytes_read;
-        else if (file_cache_bytes_read > 0) {
-          file_cache_read_buffer =
-              std::make_unique<uint8_t[]>(file_cache_bytes_read);
-          std::memcpy(file_cache_read_buffer.get(), dst, file_cache_bytes_read);
-        }
-      }
-    }
-  }
+    if (section_sp && section_sp->IsReadOnly()) {
+
+      if (GetVerifyFileCacheMemoryReads()) {
+        if (ProcessIsValid() && file_cache_bytes_read == dst_len) {
+          if (load_addr == LLDB_INVALID_ADDRESS)
+            load_addr = resolved_addr.GetLoadAddress(this);
+          if (load_addr != LLDB_INVALID_ADDRESS) {
+            std::unique_ptr<uint8_t[]> live_buf =
+                std::make_unique<uint8_t[]>(dst_len);
+            bytes_read = m_process_sp->ReadMemory(load_addr, live_buf.get(),
+                                                  dst_len, error);
+            if (bytes_read == dst_len) {
+              lldbassert(memcmp(live_buf.get(), dst, dst_len) == 0 &&
+                         "File cache and live memory diverge!");
+            }
+          }
+        }
+      }
+
+      if (file_cache_bytes_read > 0) {
 
   if (ProcessIsValid()) {
     if (load_addr == LLDB_INVALID_ADDRESS)
       load_addr = resolved_addr.GetLoadAddress(this);
 
     if (load_addr == LLDB_INVALID_ADDRESS) {
       ModuleSP addr_module_sp(resolved_addr.GetModule());
       if (addr_module_sp && addr_module_sp->GetFileSpec())
return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_target_properties[idx].default_uint_value != 0);
 }
 
 void TargetProperties::SetDebugUtilityExpression(bool debug) {
   const uint32_t idx = ePropertyDebugUtilityExpression;
   m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, debug);
 }
 
+bool TargetProperties::GetVerifyFileCacheMemoryReads() const {
+  const uint32_t idx = ePropertyVerifyFileCacheMemoryReads;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+      nullptr, idx, g_target_properties[idx].default_uint_value != 0);
+}
+
+void TargetProperties::SetVerifyFileCacheMemoryReads(bool verify) {
+  const uint32_t idx = ePropertyVerifyFileCacheMemoryReads;
+  m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, verify);
+}
+
 // Target::TargetEventData
 
 Target::TargetEventData::TargetEventData(const lldb::TargetSP &target_sp)
     : EventData(), m_target_sp(target_sp), m_module_list() {}
 
 Target::TargetEventData::TargetEventData(const lldb::TargetSP &target_sp,
                                          const ModuleList &module_list)
     : EventData(), m_target_sp(target_sp), m_module_list(module_list) {}

lldb/source/Target/TargetProperties.td

def RequireHardwareBreakpoints: Property<"require-hardware-breakpoint", "Boolean">,
     DefaultFalse,
     Desc<"Require all breakpoints to be hardware breakpoints.">;
   def AutoInstallMainExecutable: Property<"auto-install-main-executable", "Boolean">,
     DefaultTrue,
     Desc<"Always install the main executable when connected to a remote platform.">;
   def DebugUtilityExpression: Property<"debug-utility-expression", "Boolean">,
     DefaultFalse,
     Desc<"Enable debugging of LLDB-internal utility expressions.">;
+  def VerifyFileCacheMemoryReads: Property<"verify-file-cache-memory-reads", "Boolean">,
+    DefaultFalse,
+    Desc<"Verify that memory read from the file-cache is identical to the memory read from the process.">;
 }
 
 let Definition = "process_experimental" in {
   def OSPluginReportsAllThreads: Property<"os-plugin-reports-all-threads", "Boolean">,
     Global,
     DefaultTrue,
     Desc<"Set to False if your OS Plugins doesn't report all threads on each stop.">;
 }