Improve LLDB's handling of non-local minidumps

[lemo]

Summary

Normally, LLDB is creating a high-fidelity representation of a live
process, including a list of modules and sections, with the
associated memory address ranges. In order to build the module and
section map LLDB tries to locate the local module image (object file)
and will parse it.

This does not work for postmortem debugging scenarios where the crash
dump (minidump in this case) was captured on a different machine.

Fortunately the minidump format encodes enough information about
each module's memory range to allow us to create placeholder modules.
This enables most LLDB functionality involving address-to-module
translations.

Also, we may want to completly disable the search for matching
local object files if we load minidumps unless we can prove that the
local image matches the one from the crash origin.
(not part of this change, see: llvm.org/pr35193)

Example: Identify the module from a stack frame PC:

Before:
thread #1, stop reason = Exception 0xc0000005 encountered at address 0x164d14

frame #0: 0x00164d14
frame #1: 0x00167c79
frame #2: 0x00167e6d
frame #3: 0x7510336a
frame #4: 0x77759882
frame #5: 0x77759855

After:
thread #1, stop reason = Exception 0xc0000005 encountered at address 0x164d14

frame #0: 0x00164d14 C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
frame #1: 0x00167c79 C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
frame #2: 0x00167e6d C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
frame #3: 0x7510336a C:\Windows\SysWOW64\kernel32.dll
frame #4: 0x77759882 C:\Windows\SysWOW64\ntdll.dll
frame #5: 0x77759855 C:\Windows\SysWOW64\ntdll.dll

Example: target modules list

Before:
error: the target has no associated executable images

After:
[ 0] C:\Windows\System32\MSVCP120D.dll
[ 1] C:\Windows\SysWOW64\kernel32.dll
[ 2] C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug\fizzbuzz.exe
[ 3] C:\Windows\System32\MSVCR120D.dll
[ 4] C:\Windows\SysWOW64\KERNELBASE.dll
[ 5] C:\Windows\SysWOW64\ntdll.dll

NOTE: the minidump format also includes the debug info GUID, so we can fill-in the module UUID from it, but this part was excluded from this change to keep the changes simple (the LLDB UUID is hardcoded to be either 16 or 20 bytes, while the CodeView GUIDs are normally 24 bytes)

[amccarth]

Nice!

source/Plugins/Process/minidump/ProcessMinidump.cpp

53 ↗

(On Diff #142694)

I wonder if this should just be part of the constructor. I don't see a scenario where you'd create a PlaceholderModule and not want to create exactly one fake image section. I know the style guide is against doing lots of work in a constructor, but that's mostly because it's hard to report failures, which you're not doing here anyway.

329 ↗

(On Diff #142694)

nit: s/Fortunatelly/Fortunately/

339 ↗

(On Diff #142694)

If CreateImageSection bit were done by the PlaceholderModule's constructor, then there'd be no need for the artificial my_module temporary, and nobody would ever make a PlaceholderModule without an image section.

[lemo]

source/Plugins/Process/minidump/ProcessMinidump.cpp

53 ↗

(On Diff #142694)

Thanks for the suggestion. I agree, this would look a bit cleaner to fold everything in the constructor (except the extra arguments to the constructor, but it will probably still be a net win)

The reason for a separate method is the little "shared_from_this()". It can't be done from the constructor since the object is not yet managed by a shared_ptr, so we need to do it post-construction.

[clayborg]

Looks good. I questions if we want PlaceholderModule to be available for all symbolicators/core dump plugins. See inlined comments.

source/Plugins/Process/minidump/ProcessMinidump.cpp

47 ↗

(On Diff #142706)

I would be worth putting this class maybe in the same folder as lldb_private::Module and possibly renaming it. I can see this kind of thing being useful for symbolication in general and it won't be limited to use in minidumps. It should have enough accessors that allows an external client to modify everything.

79 ↗

(On Diff #142706)

Just use lldb_private::Module::m_sections_ap and put the sections in there? Any reason not to?

[labath]

The part I am not sure about here is that you are creating a Module which has no associated object file, but it still has some sections. That's not how any of our current modules/object files work, and I worry that this may cause problems down the line (and plainly put, having sections without an object file feels weird). I am wondering whether it wouldn't be better to go all the way and create a "PlaceholderObjectFile" as well (or instead of PlaceholderModule).

I don't know what the right answer here is, but it is something to think about...

packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py

53 ↗

(On Diff #142706)

Could we strengthen these assertions a bit. Given that this is static data you are loading, I don't see why you couldn't hard-code the names of the modules you should expect.

source/Plugins/Process/minidump/ProcessMinidump.cpp

47 ↗

(On Diff #142706)

I concur. Besides postmortem, we can run into the situation where we cannot access the loaded modules for live debugging as well.

53 ↗

(On Diff #142694)

This can be solved by hiding the constructor and having a static factory function which returns a shared_ptr.

[lemo]

Thanks for the feedback.

PS. The sample output in the description if from LLDB running on Linux, reading minidumps captured on Windows.

[lemo]

packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py

53 ↗

(On Diff #142706)

Done.

Just curious, do we have any support for golden output files? (it would be great for tests like this)

source/Plugins/Process/minidump/ProcessMinidump.cpp

47 ↗

(On Diff #142706)

Thanks, I agree. I think this needs a bit more baking first though, I say let's put it through some use first (and maybe identify a 2nd use case).
(in particular we'd also need a more generic interface and I'd like to avoid creating an overly complex solution which may not even fit future use cases)

79 ↗

(On Diff #142706)

Thanks, done. (no good reason, I was just extra cautious not to introduce any unexpected side-effects)

53 ↗

(On Diff #142694)

The factory is a great idea, done. (one downside with a private constructor is that we lose the benefits of std::make_shared though)

[labath]

packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py

53 ↗

(On Diff #142706)

If you're feeling adventurous, you can try rewriting this as a lit test. You should be able to just do a lldb -c my-core -o "image list" and FileCheck the output.

[lemo]

packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py

53 ↗

(On Diff #142706)

Thanks for the tip, I'll keep this in mind for the future.

[amccarth]

LGTM, but consider highlighting the side effect to target when the factory makes a Placeholder module.

In the future, we need to refactor the minidump tests to get rid of the redundancy.

source/Plugins/Process/minidump/ProcessMinidump.cpp

70 ↗

(On Diff #142799)

I didn't notice before that target is a non-const ref. Maybe the comment should explain why target is modified (and/or maybe in PlaceholderModule::Create).

[lemo]

In D45700#1070491, @amccarth wrote:

LGTM, but consider highlighting the side effect to target when the factory makes a Placeholder module.

Good observation: taking a step back, the factory introduces too much coupling, especially if we want to extend this placeholder module approach to other uses. Because of this, I reverted back to the standalone PlaceholderModule::CreateImageSection() approach. Thanks Adrian!

source/Plugins/Process/minidump/ProcessMinidump.cpp

70 ↗

(On Diff #142799)

Updated the function comment. This is similar to how other places set the section load address (ex. ObjectFileELF::SetLoadAddress)

[lemo]

Greg/Pavel, does the latest revision look good to you? Thanks!

[amccarth]

I actually preferred the factory solution.

What I intended to express is that the modification of the target doesn't seems like it should be inside the PlaceholderModule class, so whether you use a factory or not doesn't really address that aspect of the coupling. In my head, modification of the target should be done by the client that instantiates the PlaceholderModule (whether it does that via constructor or factory).

But this isn't a new problem, so I'm happy to consider the coupling problem outside the scope of this patch.

[clayborg]

LGTM

[labath]

It looks like nobody except me is worried about the module-without-an-object-file situation, so I guess we can try this out and see how it goes.

The test you've added here has been failing on windows though. I've tried to fix this in r330314, but it meant modifying the module.file.fullpath output expectations. I'm not sure where you're going with the minidump support, but if you are bothered by module.file.fullpath containing a forward slash, you may want to look into fixing the SBFileSpec behavior.

[lemo]

It looks like nobody except me is worried about the
module-without-an-object-file situation, so I guess we can try this out and
see how it goes.

Sorry Pavel, I meant to respond to this: most of the code seems to
explicitly handle this case (module-without-object-file), I just had to fix
a couple of cases. It's possible that more fixes will be required, but the
intention seems to be to accommodate for missing object files so at least
in this area I didn't have to break new ground.

I also considered creating placeholder object files, but it proved a bit
more intrusive since there are numerous places where it's assumed that
object files map to a real file which can be read and written to. Maybe at
some point we'll need to reconsider this (placeholder object files) but for
the initial iteration the placeholder modules seems to be sufficient. The
only downside I noticed is mostly cosmetic, for example things like "target
modules dump objfile" may print empty lines for the missing object files.

The test you've added here has been failing on windows though. I've tried

to fix this in r330314, but it meant modifying the module.file.fullpath
output expectations. I'm not sure where you're going with the minidump
support, but if you are bothered by module.file.fullpath containing a
forward slash, you may want to look into fixing the SBFileSpec behavior.

Thanks!

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py

def test_process_info_in_minidump(self):
         self.dbg.CreateTarget(None)
         self.target = self.dbg.GetSelectedTarget()
         self.process = self.target.LoadCore("linux-x86_64.dmp")
         self.assertTrue(self.process, PROCESS_IS_VALID)
         self.assertEqual(self.process.GetNumThreads(), 1)
         self.assertEqual(self.process.GetProcessID(), self._linux_x86_64_pid)
         self.check_state()
 
+    def test_modules_in_mini_dump(self):
+        """Test that lldb can read the list of modules from the minidump."""
+        # target create -c linux-x86_64.dmp
+        self.dbg.CreateTarget(None)
+        self.target = self.dbg.GetSelectedTarget()
+        self.process = self.target.LoadCore("linux-x86_64.dmp")
+        self.assertTrue(self.process, PROCESS_IS_VALID)
+        self.assertEqual(self.target.GetNumModules(), 9)
+        for module in self.target.modules:
+            self.assertTrue(module.IsValid())
+
     def test_thread_info_in_minidump(self):
         """Test that lldb can read the thread information from the Minidump."""
         # target create -c linux-x86_64.dmp
         self.dbg.CreateTarget(None)
         self.target = self.dbg.GetSelectedTarget()
         self.process = self.target.LoadCore("linux-x86_64.dmp")
         self.check_state()
         # This process crashed due to a segmentation fault in its
def test_stack_info_in_minidump(self):
         self.assertEqual(self.process.GetNumThreads(), 1)
         self.assertEqual(self.process.GetProcessID(), self._linux_x86_64_pid)
         thread = self.process.GetThreadAtIndex(0)
         # frame #0: linux-x86_64`crash()
         # frame #1: linux-x86_64`_start
         self.assertEqual(thread.GetNumFrames(), 2)
         frame = thread.GetFrameAtIndex(0)
         self.assertTrue(frame.IsValid())
+        self.assertTrue(frame.GetModule().IsValid())
         pc = frame.GetPC()
         eip = frame.FindRegister("pc")
         self.assertTrue(eip.IsValid())
         self.assertEqual(pc, eip.GetValueAsUnsigned())
 
     def test_snapshot_minidump(self):
         """Test that if we load a snapshot minidump file (meaning the process
         did not crash) there is no stop reason."""

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py

def test_thread_info_in_mini_dump(self):
         # This process crashed due to an access violation (0xc0000005) in its
         # one and only thread.
         self.assertEqual(self.process.GetNumThreads(), 1)
         thread = self.process.GetThreadAtIndex(0)
         self.assertEqual(thread.GetStopReason(), lldb.eStopReasonException)
         stop_description = thread.GetStopDescription(256)
         self.assertTrue("0xc0000005" in stop_description)
 
+    def test_modules_in_mini_dump(self):
+        """Test that lldb can read the list of modules from the minidump."""
+        # target create -c fizzbuzz_no_heap.dmp
+        self.dbg.CreateTarget("")
+        self.target = self.dbg.GetSelectedTarget()
+        self.process = self.target.LoadCore("fizzbuzz_no_heap.dmp")
+        self.assertTrue(self.process, PROCESS_IS_VALID)
+        expected_modules = [
+            r"C:\Windows\System32\MSVCP120D.dll",
+            r"C:\Windows\SysWOW64\kernel32.dll",
+            r"C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug\fizzbuzz.exe",
+            r"C:\Windows\System32\MSVCR120D.dll",
+            r"C:\Windows\SysWOW64\KERNELBASE.dll",
+            r"C:\Windows\SysWOW64\ntdll.dll",
+        ]
+        self.assertEqual(self.target.GetNumModules(), len(expected_modules))
+        for module, expected in zip(self.target.modules, expected_modules):
+            self.assertTrue(module.IsValid())
+            self.assertEqual(module.file.fullpath, expected)
+
     @expectedFailureAll(bugnumber="llvm.org/pr35193", hostoslist=["windows"])
     def test_stack_info_in_mini_dump(self):
         """Test that we can see a trivial stack in a VS-generate mini dump."""
         # target create -c fizzbuzz_no_heap.dmp
         self.dbg.CreateTarget("")
         self.target = self.dbg.GetSelectedTarget()
         self.process = self.target.LoadCore("fizzbuzz_no_heap.dmp")
         self.assertEqual(self.process.GetNumThreads(), 1)
         thread = self.process.GetThreadAtIndex(0)
 
         pc_list = [ 0x00164d14, 0x00167c79, 0x00167e6d, 0x7510336a, 0x77759882, 0x77759855]
 
         self.assertEqual(thread.GetNumFrames(), len(pc_list))
         for i in range(len(pc_list)):
             frame = thread.GetFrameAtIndex(i)
             self.assertTrue(frame.IsValid())
             self.assertEqual(frame.GetPC(), pc_list[i])
+            self.assertTrue(frame.GetModule().IsValid())
 
     @skipUnlessWindows # Minidump saving works only on windows
     def test_deeper_stack_in_mini_dump(self):
         """Test that we can examine a more interesting stack in a mini dump."""
         self.build()
         exe = self.getBuildArtifact("a.out")
         core = self.getBuildArtifact("core.dmp")
         try:

lldb/trunk/source/Commands/CommandObjectTarget.cpp

if (num_modules > 0) {
     for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
       Module *module = module_list.GetModulePointerAtIndexUnlocked(image_idx);
       if (module) {
         if (num_dumped++ > 0) {
           strm.EOL();
           strm.EOL();
         }
         ObjectFile *objfile = module->GetObjectFile();
+        if (objfile)
           objfile->Dump(&strm);
       }
     }
     strm.IndentLess();
   }
   return num_dumped;
 }
 
 static void DumpModuleSymtab(CommandInterpreter &interpreter, Stream &strm,

lldb/trunk/source/Core/Module.cpp

if (!m_did_load_objfile.load()) {
         }
       }
     }
   }
   return m_objfile_sp.get();
 }
 
 SectionList *Module::GetSectionList() {
-  // Populate m_unified_sections_ap with sections from objfile.
+  // Populate m_sections_ap with sections from objfile.
   if (!m_sections_ap) {
     ObjectFile *obj_file = GetObjectFile();
     if (obj_file != nullptr)
       obj_file->CreateSections(*GetUnifiedSectionList());
   }
   return m_sections_ap.get();
 }
 
 void Module::SectionFileAddressesChanged() {
   ObjectFile *obj_file = GetObjectFile();
   if (obj_file)
     obj_file->SectionFileAddressesChanged();
   SymbolVendor *sym_vendor = GetSymbolVendor();
   if (sym_vendor != nullptr)
     sym_vendor->SectionFileAddressesChanged();
 }
 
 SectionList *Module::GetUnifiedSectionList() {
-  // Populate m_unified_sections_ap with sections from objfile.
   if (!m_sections_ap)
     m_sections_ap = llvm::make_unique<SectionList>();
   return m_sections_ap.get();
 }
 
 const Symbol *Module::FindFirstSymbolWithNameAndType(const ConstString &name,
                                                      SymbolType symbol_type) {
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);

lldb/trunk/source/Core/Section.cpp

void Section::DumpName(Stream *s) const {
   SectionSP parent_sp(GetParent());
   if (parent_sp) {
     parent_sp->DumpName(s);
     s->PutChar('.');
   } else {
     // The top most section prints the module basename
     const char *name = NULL;
     ModuleSP module_sp(GetModule());
-    const FileSpec &file_spec = m_obj_file->GetFileSpec();
 
-    if (m_obj_file)
+    if (m_obj_file) {
+      const FileSpec &file_spec = m_obj_file->GetFileSpec();
       name = file_spec.GetFilename().AsCString();
+    }
     if ((!name || !name[0]) && module_sp)
       name = module_sp->GetFileSpec().GetFilename().AsCString();
     if (name && name[0])
       s->Printf("%s.", name);
   }
   m_name.Dump(s);
 }
 

lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.h

public:
   Status DoLoadCore() override;
 
   DynamicLoader *GetDynamicLoader() override;
 
   ConstString GetPluginName() override;
 
   uint32_t GetPluginVersion() override;
 
+  SystemRuntime *GetSystemRuntime() override { return nullptr; }
+
   Status DoDestroy() override;
 
   void RefreshStateAfterStop() override;
 
   bool IsAlive() override;
 
   bool WarnBeforeDetach() const override;
 

lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp

 // Other libraries and framework includes
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/State.h"
 #include "lldb/Target/DynamicLoader.h"
 #include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/DataBufferLLVM.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 // C includes
 // C++ includes
 
+using namespace lldb;
 using namespace lldb_private;
 using namespace minidump;
 
+//------------------------------------------------------------------
+/// A placeholder module used for minidumps, where the original
+/// object files may not be available (so we can't parse the object
+/// files to extract the set of sections/segments)
+///
+/// This placeholder module has a single synthetic section (.module_image)
+/// which represents the module memory range covering the whole module.
+//------------------------------------------------------------------
+class PlaceholderModule : public Module {
+public:
+  PlaceholderModule(const FileSpec &file_spec, const ArchSpec &arch) :
+    Module(file_spec, arch) {}
+
+  // Creates a synthetic module section covering the whole module image
+  // (and sets the section load address as well)
+  void CreateImageSection(const MinidumpModule *module, Target& target) {
+    const ConstString section_name(".module_image");
+    lldb::SectionSP section_sp(new Section(
+        shared_from_this(),     // Module to which this section belongs.
+        nullptr,                // ObjectFile
+        0,                      // Section ID.
+        section_name,           // Section name.
+        eSectionTypeContainer,  // Section type.
+        module->base_of_image,  // VM address.
+        module->size_of_image,  // VM size in bytes of this section.
+        0,                      // Offset of this section in the file.
+        module->size_of_image,  // Size of the section as found in the file.
+        12,                     // Alignment of the section (log2)
+        0,                      // Flags for this section.
+        1));                    // Number of host bytes per target byte
+    section_sp->SetPermissions(ePermissionsExecutable | ePermissionsReadable);
+    GetSectionList()->AddSection(section_sp);
+    target.GetSectionLoadList().SetSectionLoadAddress(
+        section_sp, module->base_of_image);
+  }
+
+  ObjectFile *GetObjectFile() override { return nullptr; }
+
+  SectionList *GetSectionList() override {
+    return Module::GetUnifiedSectionList();
+  }
+};
+
 ConstString ProcessMinidump::GetPluginNameStatic() {
   static ConstString g_name("minidump");
   return g_name;
 }
 
 const char *ProcessMinidump::GetPluginDescriptionStatic() {
   return "Minidump plug-in.";
 }
if (llvm::StringRef(name.getValue()).endswith_lower("wow64.dll")) {
       m_is_wow64 = true;
     }
 
     const auto file_spec = FileSpec(name.getValue(), true);
     ModuleSpec module_spec = file_spec;
     Status error;
     lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, &error);
     if (!module_sp || error.Fail()) {
-      continue;
+      // We failed to locate a matching local object file. Fortunately,
+      // the minidump format encodes enough information about each module's
+      // memory range to allow us to create placeholder modules.
+      //
+      // This enables most LLDB functionality involving address-to-module
+      // translations (ex. identifing the module for a stack frame PC) and
+      // modules/sections commands (ex. target modules list, ...)
+      auto placeholder_module =
+          std::make_shared<PlaceholderModule>(file_spec, GetArchitecture());
+      placeholder_module->CreateImageSection(module, GetTarget());
+      module_sp = placeholder_module;
+      GetTarget().GetImages().Append(module_sp);
     }
 
     if (log) {
       log->Printf("ProcessMinidump::%s load module: name: %s", __FUNCTION__,
                   name.getValue().c_str());
     }
 
     bool load_addr_changed = false;