minidump: Add ability to attach (breakpad) symbol files to placeholder modules

[labath]

Summary

The reason this wasn't working was that ProcessMinidump was creating odd
object-file-less modules, and SymbolFileBreakpad required the module to
have an associated object file because it needed to get its base
address.

This fixes that by introducing a PlaceholderObjectFile to serve as a
dummy object file. The general idea for this is taken from D55142, but
I've reworked it a bit to avoid the need for the PlaceholderModule
class. Now that we have an object file, our modules are sufficiently
similar to regular modules that we can use the regular Module class
almost out of the box -- the only thing I needed to tweak was the
Module::CreateModuleFromObjectFile functon to set the module's FileSpec
in addition to it's architecture. This wasn't needed for ObjectFileJIT
(the other user of CreateModuleFromObjectFile), but it shouldn't hurt it
either, and the change seems like a straightforward extension of this
function.

[amccarth]

Nice.

[clayborg]

Check the comment about m_platform_file

include/lldb/Core/Module.h

174–175

Why do we need to update the m_file? m_platform_file is the path of the module as it is known the the remote system, so if m_file differs from the object file's file spec, we might want to shuffle, m_file into m_platform_file first and then overwrite it?

[lemo]

Looks good - a few comments inline.

include/lldb/Core/Module.h

176

nit: return std::move(x) is almost never a good idea. It's not needed, verbose and most importantly it actually inhibits NRVO so it's likely less efficient.

source/Plugins/Process/minidump/ProcessMinidump.cpp

51–83

Can we avoid passing both the module_sp and module_spec? I try to avoid interfaces which allow for inconsistent states, ex: what if module_sp and module_spec describe completely different modules?

At very least I'd add a few asserts, but if we can change the interface to avoid it, even better.

52

Nice - so we can do without this in all scenarios? What about PDBs?

66

should we dump something meaningful? I don't remember the exact command but I think this surfaces in user output

[labath]

• remove std::move
• avoid using the word "update" in CreateModuleFromObjectFile
• implement the Dump method and cover it with a test

include/lldb/Core/Module.h

174–175

I used the word "update", because the previous comment did, but this isn't really "updating" anything. It is just "setting" the FileSpec because it was previously empty (the module we're setting this on is created a couple of lines back (line 163) as an empty module. The entire reason for existence of this function is to perform the articulate dance of creating the objects in the right order (because ObjectFile stores a shared_ptr to the module, it needs to be created after the ModuleSP is fully constructed).

So, there isn't to shuffle here. We just set m_file, to whatever the ObjectFile tells us is the file it was created from.

176

Yep, I should have known that. I'll remove it.

source/Plugins/Process/minidump/ProcessMinidump.cpp

51–83

Yeah, I agree this interface is weird, but this is kind of necessary to make the CreateModuleFromObjectFile flow work. The idea there is to create an empty module, then place an ObjectFile inside of it, and then use the data from the ObjectFile to initialize the module.

So, at the point when this function is called, the module_sp object will be empty (so there isn't anything to assert). It's only present here to satisfy the ObjectFile contract and set up the weak_ptr backlink. The module_spec contains the actual data, which is used to initialize the PlaceholderObjectFile (and which will then in turn initialize the Module object).

52

I haven't tried anything special besides running existing tests, but I don't think this should hamper anything. The modules I create here will be fairly realistic, they will have UUIDs, ObjectFiles and everything. So if the PDBs were able to attach to the PlaceholderModules, they should certainly be able to do the same with regular modules + PlaceholderObjectFiles.

66

I guess you meant "image dump objfile". It's kind of weird because that command prefixes the output with "outputting headers for <N> modules", and I have no idea what would be the "headers" for this object file. However I suppose a short one line summary of the file is better than just printing nothing.

[labath]

Hi Greg, what do you think of my replies to your CreateModuleFromObjectFile comment?

[clayborg]

In D57751#1390345, @labath wrote:

Hi Greg, what do you think of my replies to your CreateModuleFromObjectFile comment?

Your explanation makes sense. I missed the fact that this function was creating a module from an object file!

include/lldb/Core/Module.h

public:
   static lldb::ModuleSP CreateModuleFromObjectFile(Args &&... args) {
     // Must create a module and place it into a shared pointer before we can
     // create an object file since it has a std::weak_ptr back to the module,
     // so we need to control the creation carefully in this static function
     lldb::ModuleSP module_sp(new Module());
     module_sp->m_objfile_sp =
         std::make_shared<ObjFilePlugin>(module_sp, std::forward<Args>(args)...);
 
-    // Once we get the object file, update our module with the object file's
-    // architecture since it might differ in vendor/os if some parts were
-    // unknown.
-    if (ArchSpec arch = module_sp->m_objfile_sp->GetArchitecture()) {
-      module_sp->m_arch = arch;
-      return module_sp;
-    }
-    return nullptr;
-  }
+    // Once we get the object file, set module ArchSpec to the one we get from
+    // the object file. If the object file does not have an architecture, we
+    // consider the creation a failure.
+    ArchSpec arch = module_sp->m_objfile_sp->GetArchitecture();
+    if (!arch)
+      return nullptr;
+
+    // Also copy the object file's FileSpec.
+    module_sp->m_file = module_sp->m_objfile_sp->GetFileSpec();
 
   //------------------------------------------------------------------
   /// Destructor.
   //------------------------------------------------------------------
   ~Module() override;
 
   bool MatchesModuleSpec(const ModuleSpec &module_ref);
 

lit/Minidump/Inputs/linux-x86_64.syms

+MODULE Linux x86_64 3B285CE327C387C262DB788BF5A4078B0 linux-x86_64
+INFO CODE_ID E35C283BC327C28762DB788BF5A4078BE2351448
+FUNC 3d0 18 0 crash()
+FUNC 3f0 10 0 _start

lit/Minidump/breakpad-symbols.test

+# Test that we can attach breakpad symbols to the "placeholder" modules created
+# for minidump files.
+#
+# The minidump input for this test taken from packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new
+
+# RUN: %lldb -c %S/Inputs/linux-x86_64.dmp \
+# RUN:   -o "target symbols add -s /tmp/test/linux-x86_64 %S/Inputs/linux-x86_64.syms" \
+# RUN:   -s %s -o exit | FileCheck %s
+
+image list
+# CHECK-LABEL: image list
+# CHECK: E35C283B-C327-C287-62DB-788BF5A4078B-E2351448 0x0000000000400000 /tmp/test/linux-x86_64
+
+image dump symtab /tmp/test/linux-x86_64
+# CHECK-LABEL: image dump symtab /tmp/test/linux-x86_64
+# CHECK: Symtab, file = /tmp/test/linux-x86_64, num_symbols = 2:
+# CHECK: [    0]      0   X Code            0x00000000004003d0 0x00000000004003d0 0x0000000000000018 0x00000000 crash()
+# CHECK: [    1]      0   X Code            0x00000000004003f0 0x00000000004003f0 0x0000000000000010 0x00000000 _start
+
+image lookup -a 0x4003f3
+# CHECK-LABEL: image lookup -a 0x4003f3
+# CHECK: Summary: linux-x86_64`_start + 3
+
+image dump objfile /tmp/test/linux-x86_64
+# CHECK-LABEL: image dump objfile /tmp/test/linux-x86_64
+# CHECK: Placeholder object file for /tmp/test/linux-x86_64 loaded at [0x400000-0x401000)

source/Plugins/Process/minidump/ProcessMinidump.cpp

 
 // 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 {
+namespace {
+
+/// A minimal ObjectFile implementation providing a dummy object file for the
+/// cases when the real module binary is not available. This allows the module
+/// to show up in "image list" and symbols to be added to it.
+class PlaceholderObjectFile : public ObjectFile {
 public:
-  PlaceholderModule(const ModuleSpec &module_spec) :
-    Module(module_spec.GetFileSpec(), module_spec.GetArchitecture()) {
-    if (module_spec.GetUUID().IsValid())
-      SetUUID(module_spec.GetUUID());
-  }
-
-  // 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);
+  PlaceholderObjectFile(const lldb::ModuleSP &module_sp,
+                        const ModuleSpec &module_spec, lldb::offset_t base,
+                        lldb::offset_t size)
+      : ObjectFile(module_sp, &module_spec.GetFileSpec(), /*file_offset*/ 0,
+                   /*length*/ 0, /*data_sp*/ nullptr, /*data_offset*/ 0),
+        m_arch(module_spec.GetArchitecture()), m_uuid(module_spec.GetUUID()),
+        m_base(base), m_size(size) {
+    m_symtab_ap = llvm::make_unique<Symtab>(this);
+  }
+
+  ConstString GetPluginName() override { return ConstString("placeholder"); }
+  uint32_t GetPluginVersion() override { return 1; }
+  bool ParseHeader() override { return true; }
+  Type CalculateType() override { return eTypeUnknown; }
+  Strata CalculateStrata() override { return eStrataUnknown; }
+  uint32_t GetDependentModules(FileSpecList &file_list) override { return 0; }
+  bool IsExecutable() const override { return false; }
+  ArchSpec GetArchitecture() override { return m_arch; }
+  Symtab *GetSymtab() override { return m_symtab_ap.get(); }
+  bool IsStripped() override { return true; }
+  ByteOrder GetByteOrder() const override { return m_arch.GetByteOrder(); }
+
+  uint32_t GetAddressByteSize() const override {
+    return m_arch.GetAddressByteSize();
+  }
+
+  bool GetUUID(UUID *uuid) override {
+    *uuid = m_uuid;
+    return true;
+  }
+
+  Address GetBaseAddress() override {
+    return Address(m_sections_ap->GetSectionAtIndex(0), 0);
   }
 
-ObjectFile *GetObjectFile() override { return nullptr; }
+  void CreateSections(SectionList &unified_section_list) override {
+    m_sections_ap = llvm::make_unique<SectionList>();
+    auto section_sp = std::make_shared<Section>(
+        GetModule(), this, /*sect_id*/ 0, ConstString(".module_image"),
+        eSectionTypeOther, m_base, m_size, /*file_offset*/ 0, /*file_size*/ 0,
+        /*log2align*/ 0, /*flags*/ 0);
+    m_sections_ap->AddSection(section_sp);
+    unified_section_list.AddSection(std::move(section_sp));
+  }
+
+  bool SetLoadAddress(Target &target, addr_t value,
+                      bool value_is_offset) override {
+    assert(!value_is_offset);
+    assert(value == m_base);
+
+    // Create sections if they haven't been created already.
+    GetModule()->GetSectionList();
+    assert(m_sections_ap->GetNumSections(0) == 1);
 
-  SectionList *GetSectionList() override {
-    return Module::GetUnifiedSectionList();
+    target.GetSectionLoadList().SetSectionLoadAddress(
+        m_sections_ap->GetSectionAtIndex(0), m_base);
+    return true;
   }
+
+  void Dump(Stream *s) override {
+    s->Format("Placeholder object file for {0} loaded at [{1:x}-{2:x})\n",
+              GetFileSpec(), m_base, m_base + m_size);
+  }
+
+private:
+  ArchSpec m_arch;
+  UUID m_uuid;
+  lldb::offset_t m_base;
+  lldb::offset_t m_size;
 };
+} // namespace
 
 ConstString ProcessMinidump::GetPluginNameStatic() {
   static ConstString g_name("minidump");
   return g_name;
 }
 
 const char *ProcessMinidump::GetPluginDescriptionStatic() {
   return "Minidump plug-in.";
for (auto module : filtered_modules) {
     if (llvm::StringRef(name.getValue()).endswith_lower("wow64.dll")) {
       m_is_wow64 = true;
     }
 
     const auto uuid = m_minidump_parser.GetModuleUUID(module);
     auto file_spec = FileSpec(name.getValue(), GetArchitecture().GetTriple());
     FileSystem::Instance().Resolve(file_spec);
     ModuleSpec module_spec(file_spec, uuid);
+    module_spec.GetArchitecture() = GetArchitecture();
     Status error;
     lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, &error);
     if (!module_sp || error.Fail()) {
       // 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, ...)
       if (log) {
         log->Printf("Unable to locate the matching object file, creating a "
                     "placeholder module for: %s",
                     name.getValue().c_str());
       }
 
-      auto placeholder_module =
-          std::make_shared<PlaceholderModule>(module_spec);
-      placeholder_module->CreateImageSection(module, GetTarget());
-      module_sp = placeholder_module;
+      module_sp = Module::CreateModuleFromObjectFile<PlaceholderObjectFile>(
+          module_spec, module->base_of_image, module->size_of_image);
       GetTarget().GetImages().Append(module_sp);
     }
 
     if (log) {
       log->Printf("ProcessMinidump::%s load module: name: %s", __FUNCTION__,
                   name.getValue().c_str());
     }