Use the UUID from the minidump's CodeView Record for placeholder modules

[lemo]

Summary

This change adds support for two types of Minidump CodeView records:

1. PDB70 (reference: https://crashpad.chromium.org/doxygen/structcrashpad_1_1CodeViewRecordPDB70.html)

This is by far the most common record type.

2. ELF BuildID (found in Breakpad/Crashpad generated minidumps)

This would set a proper UUID for placeholder modules, in turn enabling an accurate match with local module images.

[amccarth]

The big picture here is fine.

I see a couple opportunities in the details, but I won't block on them.

include/lldb/Core/Module.h

779 ↗

(On Diff #144644)

Was there no SetUUID before because the UUID was considered immutable? I wonder if that's worth keeping. Is the UUID always known at construction time?

source/Core/Module.cpp

341 ↗

(On Diff #144644)

I wonder if the lazy UUID lookup in Module::GetUUID should call this to avoid code duplication. I know it's not a lot of code, but it involves a mutex and an atomic flag, so it might be nice to always have this update in just one place.

source/Plugins/Process/minidump/MinidumpParser.cpp

99 ↗

(On Diff #144644)

I wonder if this should return an Expected<UUID>. The function has multiple ways it can fail, in which case the error info is lost and we silently return an empty UUID.

[labath]

include/lldb/Core/Module.h

779 ↗

(On Diff #144644)

The UUID is not always know at construction time -- sometimes it can be, if you set it on the FileSpec you use to create the module, but you don't have to do that.

However, it is true that the UUID was constant during the lifetime of the Module, and this changes that, which is not ideal IMO.

It seems that in the only place you call this function, you already know the UUID. Would it be possible to do this work in the PlaceholderModule constructor (thereby avoiding any changes to the Module API) ?

source/Plugins/Process/minidump/MinidumpParser.cpp

118–128 ↗

(On Diff #144644)

Looks like the elf part of this is not tested. Do any of our linux minidumps have a build-id we could use?

Since the parser->SB path is already tested by the windows test, this doesn't have to be a full-blown test and a minidump parser unittest (unittests/Process/minidump) should be sufficient.

[lemo]

Thanks for the feedback. I uploaded a new revision (incorporating some of the feedback, including an ELF test case)

include/lldb/Core/Module.h

779 ↗

(On Diff #144644)

Good point: I agree, it's a good idea to preserve the invariant that the module UUID does not change once it's set.

I'd not add more complexity & arguments to the placeholder module constructor though. Moreover, even if it's technically possible to reach into the protected data members from Module I'd like to avoid doing that since Module::m_uuid is closely tied to Module::m_mutex and Module::m_did_parse_uuid (the later now renamed to m_did_set_uuid).

I added a check and assert to prevent Module::SetUUID from overwriting an already set UUID. (Personally I'd would've liked to have the assert be a fastfail in all builds but I remember that is somewhat against the LLDB philosophy)

source/Core/Module.cpp

341 ↗

(On Diff #144644)

Good point, but I'm afraid that would obscure the double-checked-locking in Module::GetUUID() so I'd rather not change it.

source/Plugins/Process/minidump/MinidumpParser.cpp

99 ↗

(On Diff #144644)

The goal is support the common CV records we expect to see from current toolchains and the contract for GetModuleUUID() is to either return a valid UUID or an empty one.

Neither is an error: the CV record is optional and we also can't guarantee to handle every single CV record type.

[labath]

include/lldb/Core/Module.h

779 ↗

(On Diff #144644)

Guarding the invariant with an assert is better than nothing, but even better is arranging stuff so that the invariant cannot be broken in the first place. And this feels like the sort of thing that should be possible to make safe by design.

How about adding a special (protected?) Module constructor, which takes a UUID argument and uses it to initialize the m_uuid field. This way, it is impossible by design to change the UUID mid-flight, and you still don't have to reach into the protected Module fields from the other class (we could even make them private if we want to).

[lemo]

include/lldb/Core/Module.h

779 ↗

(On Diff #144644)

Trying to make sure that the invariant can't be invalidated is a high bar considering that most Module state is protected. But I agree with the sentiment and here's a new iteration:

1. Made Module::SetUUID() protected (note that the m_uuid change is conditional, not just protected by an lldbassert)
2. PlaceholderModule constructor is where SetUUID is called from

I'm hesitant to add another Module constructor: that class already has 2 public + 1 private constructors (even using a delegating constructor, the addition of a new protected constructor questionable). But if you still prefer that option better I'll make the change.

[labath]

I think this is fine. I would have preferred a constructor solution, but the situation there does appear to be a bit messy. I like how you were able to place the SetUUID method next to SetArchitecture. Thanks for your patience.

source/Commands/CommandObjectTarget.cpp

1390 ↗

(On Diff #144790)

strm.Format("No object file for module: {0:F}\n", module->GetFileSpec()) should hopefully fit on a single line

[lemo]

source/Commands/CommandObjectTarget.cpp

1390 ↗

(On Diff #144790)

It does not fit on a single line, but it's cleaner - done, thanks for suggesting it!

lldb/trunk/include/lldb/Core/Module.h

public:
   ///
   /// @return
   ///     A valid type list pointer, or nullptr if there is no valid
   ///     symbol vendor for this module.
   //------------------------------------------------------------------
   TypeList *GetTypeList();
 
   //------------------------------------------------------------------
-  /// Get a pointer to the UUID value contained in this object.
+  /// Get a reference to the UUID value contained in this object.
   ///
   /// If the executable image file doesn't not have a UUID value built into
   /// the file format, an MD5 checksum of the entire file, or slice of the
   /// file for the current architecture should be used.
   ///
   /// @return
   ///     A const pointer to the internal copy of the UUID value in
   ///     this module if this module has a valid UUID value, NULL
PathMappingList m_source_mappings; ///< Module specific source remappings for
                                      ///that doesn't match where the sources
                                      ///currently are
   lldb::SectionListUP m_sections_ap; ///< Unified section list for module that
                                      ///is used by the ObjectFile and and
                                      ///ObjectFile instances for the debug info
 
   std::atomic<bool> m_did_load_objfile{false};
   std::atomic<bool> m_did_load_symbol_vendor{false};
-  std::atomic<bool> m_did_parse_uuid{false};
+  std::atomic<bool> m_did_set_uuid{false};
   mutable bool m_file_has_changed : 1,
       m_first_file_changed_log : 1; /// See if the module was modified after it
                                     /// was initially opened.
 
   //------------------------------------------------------------------
   /// Resolve a file or load virtual address.
   ///
   /// Tries to resolve \a vm_addr as a file address (if \a
uint32_t ResolveSymbolContextForAddress(lldb::addr_t vm_addr,
                                           Address &so_addr, SymbolContext &sc);
 
   void SymbolIndicesToSymbolContextList(Symtab *symtab,
                                         std::vector<uint32_t> &symbol_indexes,
                                         SymbolContextList &sc_list);
 
   bool SetArchitecture(const ArchSpec &new_arch);
 
+  void SetUUID(const lldb_private::UUID &uuid);
+
   SectionList *GetUnifiedSectionList();
 
   friend class ModuleList;
   friend class ObjectFile;
   friend class SymbolFile;
 
 private:
   Module(); // Only used internally by CreateJITModule ()

lldb/trunk/include/lldb/Core/ModuleSpec.h

 
 class ModuleSpec {
 public:
   ModuleSpec()
       : m_file(), m_platform_file(), m_symbol_file(), m_arch(), m_uuid(),
         m_object_name(), m_object_offset(0), m_object_size(0),
         m_source_mappings() {}
 
-  ModuleSpec(const FileSpec &file_spec)
+  ModuleSpec(const FileSpec &file_spec, const UUID& uuid = UUID())
       : m_file(file_spec), m_platform_file(), m_symbol_file(), m_arch(),
-        m_uuid(), m_object_name(), m_object_offset(0),
+        m_uuid(uuid), m_object_name(), m_object_offset(0),
         m_object_size(file_spec.GetByteSize()), m_source_mappings() {}
 
   ModuleSpec(const FileSpec &file_spec, const ArchSpec &arch)
       : m_file(file_spec), m_platform_file(), m_symbol_file(), m_arch(arch),
         m_uuid(), m_object_name(), m_object_offset(0),
         m_object_size(file_spec.GetByteSize()), m_source_mappings() {}
 
   ModuleSpec(const ModuleSpec &rhs)

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

class MiniDumpNewTestCase(TestBase):
 
     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:
+        expected_modules = [
+            {
+                'filename' : 'linux-gate.so',
+                'uuid' : '4EAD28F8-88EF-3520-872B-73C6F2FE7306-C41AF22F',
+            },
+            {
+                'filename' : 'libm-2.19.so',
+                'uuid' : 'D144258E-6149-00B2-55A3-1F3FD2283A87-8670D5BC',
+            },
+            {
+                'filename' : 'libstdc++.so.6.0.19',
+                'uuid' : '76190E92-2AF7-457D-078F-75C9B15FA184-E83EB506',
+            },
+            {
+                'filename' : 'libc-2.19.so',
+                'uuid' : 'CF699A15-CAAE-64F5-0311-FC4655B86DC3-9A479789',
+            },
+            {
+                'filename' : 'linux-x86_64',
+                'uuid' : 'E35C283B-C327-C287-62DB-788BF5A4078B-E2351448',
+            },
+            {
+                'filename' : 'libgcc_s.so.1',
+                'uuid' : '36311B44-5771-0AE5-578C-4BF00791DED7-359DBB92',
+            },
+            {
+                'filename' : 'libpthread-2.19.so',
+                'uuid' : '31E9F21A-E8C1-0396-171F-1E13DA157809-86FA696C',
+            },
+            {
+                'filename' : 'ld-2.19.so',
+                'uuid' : 'D0F53790-4076-D73F-29E4-A37341F8A449-E2EF6CD0',
+            },
+            {
+                'filename' : 'libbreakpad.so',
+                'uuid' : '784FD549-332D-826E-D23F-18C17C6F320A',
+            },
+        ]
+        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.basename, expected['filename'])
+            self.assertEqual(module.GetUUIDString(), expected['uuid'])
 
     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()

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

class MiniDumpTestCase(TestBase):
     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",
+            {
+                'filename' : r"C:\Windows\System32/MSVCP120D.dll",
+                'uuid' : '6E51053C-E757-EB40-8D3F-9722C5BD80DD-01000000',
+            },
+            {
+                'filename' : r"C:\Windows\SysWOW64/kernel32.dll",
+                'uuid' : '1B7ECBE5-5E00-1341-AB98-98D6913B52D8-02000000',
+            },
+            {
+                'filename' : r"C:\Users\amccarth\Documents\Visual Studio 2013\Projects\fizzbuzz\Debug/fizzbuzz.exe",
+                'uuid' : '91B7450F-969A-F946-BF8F-2D6076EA421A-11000000',
+            },
+            {
+                'filename' : r"C:\Windows\System32/MSVCR120D.dll",
+                'uuid' : '86FB8263-C446-4640-AE42-8D97B3F91FF2-01000000',
+            },
+            {
+                'filename' : r"C:\Windows\SysWOW64/KERNELBASE.dll",
+                'uuid' : '4152F90B-0DCB-D44B-AC5D-186A6452E522-01000000',
+            },
+            {
+                'filename' : r"C:\Windows\SysWOW64/ntdll.dll",
+                'uuid' : '6A84B0BB-2C40-5240-A16B-67650BBFE6B0-02000000',
+            },
         ]
         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)
+            self.assertEqual(module.file.fullpath, expected['filename'])
+            self.assertEqual(module.GetUUIDString(), expected['uuid'])
 
     @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")

lldb/trunk/source/Commands/CommandObjectTarget.cpp

for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
       if (module) {
         if (num_dumped++ > 0) {
           strm.EOL();
           strm.EOL();
         }
         ObjectFile *objfile = module->GetObjectFile();
         if (objfile)
           objfile->Dump(&strm);
+        else {
+          strm.Format("No object file for module: {0:F}\n",
+                      module->GetFileSpec());
+        }
       }
     }
     strm.IndentLess();
   }
   return num_dumped;
 }
 
 static void DumpModuleSymtab(CommandInterpreter &interpreter, Stream &strm,

lldb/trunk/source/Core/Module.cpp

 #include "lldb/Symbol/TypeList.h" // for TypeList
 #include "lldb/Symbol/TypeMap.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/Platform.h" // for Platform
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
+#include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Logging.h" // for GetLogIfAn...
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Stream.h" // for Stream
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 
if (m_objfile_sp) {
     } else {
       error.SetErrorString("invalid process");
     }
   }
   return m_objfile_sp.get();
 }
 
 const lldb_private::UUID &Module::GetUUID() {
-  if (!m_did_parse_uuid.load()) {
+  if (!m_did_set_uuid.load()) {
     std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    if (!m_did_parse_uuid.load()) {
+    if (!m_did_set_uuid.load()) {
       ObjectFile *obj_file = GetObjectFile();
 
       if (obj_file != nullptr) {
         obj_file->GetUUID(&m_uuid);
-        m_did_parse_uuid = true;
+        m_did_set_uuid = true;
       }
     }
   }
   return m_uuid;
 }
 
+void Module::SetUUID(const lldb_private::UUID &uuid) {
+  std::lock_guard<std::recursive_mutex> guard(m_mutex);
+  if (!m_did_set_uuid) {
+    m_uuid = uuid;
+    m_did_set_uuid = true;
+  } else {
+    lldbassert(!"Attempting to overwrite the existing module UUID");
+  }
+}
+
 TypeSystem *Module::GetTypeSystemForLanguage(LanguageType language) {
   return m_type_system_map.GetTypeSystemForLanguage(language, this, true);
 }
 
 void Module::ParseAllDebugSymbols() {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   size_t num_comp_units = GetNumCompileUnits();
   if (num_comp_units == 0)

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

 #define liblldb_MinidumpParser_h_
 
 // Project includes
 #include "MinidumpTypes.h"
 
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/Status.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 
 // C includes
 
public:
   Create(const lldb::DataBufferSP &data_buf_sp);
 
   llvm::ArrayRef<uint8_t> GetData();
 
   llvm::ArrayRef<uint8_t> GetStream(MinidumpStreamType stream_type);
 
   llvm::Optional<std::string> GetMinidumpString(uint32_t rva);
 
+  UUID GetModuleUUID(const MinidumpModule* module);
+
   llvm::ArrayRef<MinidumpThread> GetThreads();
 
   llvm::ArrayRef<uint8_t> GetThreadContext(const MinidumpThread &td);
 
   llvm::ArrayRef<uint8_t> GetThreadContextWow64(const MinidumpThread &td);
 
   const MinidumpSystemInfo *GetSystemInfo();
 

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

 llvm::Optional<std::string> MinidumpParser::GetMinidumpString(uint32_t rva) {
   auto arr_ref = m_data_sp->GetData();
   if (rva > arr_ref.size())
     return llvm::None;
   arr_ref = arr_ref.drop_front(rva);
   return parseMinidumpString(arr_ref);
 }
 
+UUID MinidumpParser::GetModuleUUID(const MinidumpModule *module) {
+  auto cv_record =
+      GetData().slice(module->CV_record.rva, module->CV_record.data_size);
+
+  // Read the CV record signature
+  const llvm::support::ulittle32_t *signature = nullptr;
+  Status error = consumeObject(cv_record, signature);
+  if (error.Fail())
+    return UUID();
+
+  const CvSignature cv_signature =
+      static_cast<CvSignature>(static_cast<const uint32_t>(*signature));
+
+  if (cv_signature == CvSignature::Pdb70) {
+    // PDB70 record
+    const CvRecordPdb70 *pdb70_uuid = nullptr;
+    Status error = consumeObject(cv_record, pdb70_uuid);
+    if (!error.Fail())
+      return UUID(pdb70_uuid, sizeof(*pdb70_uuid));
+  } else if (cv_signature == CvSignature::ElfBuildId) {
+    // ELF BuildID (found in Breakpad/Crashpad generated minidumps)
+    //
+    // This is variable-length, but usually 20 bytes
+    // as the binutils ld default is a SHA-1 hash.
+    // (We'll handle only 16 and 20 bytes signatures,
+    // matching LLDB support for UUIDs)
+    //
+    if (cv_record.size() == 16 || cv_record.size() == 20)
+      return UUID(cv_record.data(), cv_record.size());
+  }
+
+  return UUID();
+}
+
 llvm::ArrayRef<MinidumpThread> MinidumpParser::GetThreads() {
   llvm::ArrayRef<uint8_t> data = GetStream(MinidumpStreamType::ThreadList);
 
   if (data.size() == 0)
     return llvm::None;
 
   return MinidumpThread::ParseThreadList(data);
 }

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

 
 enum class MinidumpHeaderConstants : uint32_t {
   Signature = 0x504d444d, // 'PMDM'
   Version = 0x0000a793,   // 42899
   LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue = */ Signature)
 
 };
 
+enum class CvSignature : uint32_t {
+  Pdb70 = 0x53445352, // RSDS
+  ElfBuildId = 0x4270454c, // BpEL (Breakpad/Crashpad minidumps)
+};
+
+// Reference:
+// https://crashpad.chromium.org/doxygen/structcrashpad_1_1CodeViewRecordPDB70.html
+struct CvRecordPdb70 {
+  uint8_t Uuid[16];
+  llvm::support::ulittle32_t Age;
+  // char PDBFileName[];
+};
+static_assert(sizeof(CvRecordPdb70) == 20,
+              "sizeof CvRecordPdb70 is not correct!");
+
 // Reference:
 // https://msdn.microsoft.com/en-us/library/windows/desktop/ms680394.aspx
 enum class MinidumpStreamType : uint32_t {
   Unused = 0,
   Reserved0 = 1,
   Reserved1 = 2,
   ThreadList = 3,
   ModuleList = 4,

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

 /// 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) {}
+  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
for (auto module : filtered_modules) {
     }
 
     // check if the process is wow64 - a 32 bit windows process running on a
     // 64 bit windows
     if (llvm::StringRef(name.getValue()).endswith_lower("wow64.dll")) {
       m_is_wow64 = true;
     }
 
+    const auto uuid = m_minidump_parser.GetModuleUUID(module);
     const auto file_spec =
         FileSpec(name.getValue(), true, GetArchitecture().GetTriple());
-    ModuleSpec module_spec = file_spec;
+    ModuleSpec module_spec(file_spec, uuid);
     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, ...)
       auto placeholder_module =
-          std::make_shared<PlaceholderModule>(file_spec, GetArchitecture());
+          std::make_shared<PlaceholderModule>(module_spec);
       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());