Fix a crasher due to an assert when two files have the same UUID but different paths.

[clayborg]

Summary

The PlaceholderObjectFile has an assert in SetLoadAddress that fires if "m_base == value" is not true. To avoid this, we create check that the base address matches, and if it doesn't we clear the module that was found using the UUID so that we create a new PlaceholderObjectFile. Added a test to cover this issue.

[clayborg]

Remove printf that was left in.

[labath]

Looks good, modulo the comment about shadowing.

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

115

This implements something very similar to the GetBaseAddress function on line 81 (it is equivalent to const_cast<PlaceholderObjectFile *>(this)->GetBaseAddress().GetFileAddress()). In fact, the only reason you could define this function is because it is const, while the other method is not. Having two methods with the same name returning different types is very confusing. I think you should just drop this and replace the call on line 417 with objfile->GetBaseAddress().GetFileAddress(). That way, you don't even need to cast anything.

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

def get_minidump_modules(self, yaml_file):
         self.yaml2obj(yaml_file, minidump_path)
         self.target = self.dbg.CreateTarget(None)
         self.process = self.target.LoadCore(minidump_path)
         return self.target.modules
 
     def test_zero_uuid_modules(self):
         """
             Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
-            but contains a PDB70 value whose age is zero and whose UUID values are 
+            but contains a PDB70 value whose age is zero and whose UUID values are
             all zero. Prior to a fix all such modules would be duplicated to the
             first one since the UUIDs claimed to be valid and all zeroes. Now we
             ensure that the UUID is not valid for each module and that we have
             each of the modules in the target after loading the core
         """
         modules = self.get_minidump_modules("linux-arm-zero-uuids.yaml")
         self.assertEqual(2, len(modules))
         self.verify_module(modules[0], "/file/does/not/exist/a", None)
         self.verify_module(modules[1], "/file/does/not/exist/b", None)
 
     def test_uuid_modules_no_age(self):
         """
             Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
-            and contains a PDB70 value whose age is zero and whose UUID values are 
+            and contains a PDB70 value whose age is zero and whose UUID values are
             valid. Ensure we decode the UUID and don't include the age field in the UUID.
         """
         modules = self.get_minidump_modules("linux-arm-uuids-no-age.yaml")
         modules = self.target.modules
         self.assertEqual(2, len(modules))
         self.verify_module(modules[0], "/tmp/a", "01020304-0506-0708-090A-0B0C0D0E0F10")
         self.verify_module(modules[1], "/tmp/b", "0A141E28-323C-4650-5A64-6E78828C96A0")
 
     def test_uuid_modules_no_age_apple(self):
         """
             Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
-            and contains a PDB70 value whose age is zero and whose UUID values are 
+            and contains a PDB70 value whose age is zero and whose UUID values are
             valid. Ensure we decode the UUID and don't include the age field in the UUID.
             Also ensure that the first uint32_t is byte swapped, along with the next
             two uint16_t values. Breakpad incorrectly byte swaps these values when it
             saves Darwin minidump files.
         """
         modules = self.get_minidump_modules("macos-arm-uuids-no-age.yaml")
         modules = self.target.modules
         self.assertEqual(2, len(modules))
         self.verify_module(modules[0], "/tmp/a", "04030201-0605-0807-090A-0B0C0D0E0F10")
         self.verify_module(modules[1], "/tmp/b", "281E140A-3C32-5046-5A64-6E78828C96A0")
 
     def test_uuid_modules_with_age(self):
         """
             Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
-            and contains a PDB70 value whose age is valid and whose UUID values are 
+            and contains a PDB70 value whose age is valid and whose UUID values are
             valid. Ensure we decode the UUID and include the age field in the UUID.
         """
         modules = self.get_minidump_modules("linux-arm-uuids-with-age.yaml")
         self.assertEqual(2, len(modules))
         self.verify_module(modules[0], "/tmp/a", "01020304-0506-0708-090A-0B0C0D0E0F10-10101010")
         self.verify_module(modules[1], "/tmp/b", "0A141E28-323C-4650-5A64-6E78828C96A0-20202020")
 
     def test_uuid_modules_elf_build_id_16(self):
def test_uuid_modules_elf_build_id_zero(self):
             Test multiple modules having a MINIDUMP_MODULE.CvRecord that is valid,
             and contains a ELF build ID whose value is all zero.
         """
         modules = self.get_minidump_modules("linux-arm-uuids-elf-build-id-zero.yaml")
         self.assertEqual(2, len(modules))
         self.verify_module(modules[0], "/not/exist/a", None)
         self.verify_module(modules[1], "/not/exist/b", None)
 
+    def test_uuid_modules_elf_build_id_same(self):
+        """
+            Test multiple modules having a MINIDUMP_MODULE.CvRecord that is
+            valid, and contains a ELF build ID whose value is the same. There
+            is an assert in the PlaceholderObjectFile that was firing when we
+            encountered this which was crashing the process that was checking
+            if PlaceholderObjectFile.m_base was the same as the address this
+            fake module was being loaded at. We need to ensure we don't crash
+            in such cases and that we add both modules even though they have
+            the same UUID.
+        """
+        modules = self.get_minidump_modules("linux-arm-same-uuids.yaml")
+        self.assertEqual(2, len(modules))
+        self.verify_module(modules[0], "/file/does/not/exist/a",
+                           '11223344-1122-3344-1122-334411223344-11223344')
+        self.verify_module(modules[1], "/file/does/not/exist/b",
+                           '11223344-1122-3344-1122-334411223344-11223344')
+
     @expectedFailureAll(oslist=["windows"])
     def test_partial_uuid_match(self):
         """
             Breakpad has been known to create minidump files using CvRecord in each
             module whose signature is set to PDB70 where the UUID only contains the
-            first 16 bytes of a 20 byte ELF build ID. Code was added to 
-            ProcessMinidump.cpp to deal with this and allows partial UUID matching. 
+            first 16 bytes of a 20 byte ELF build ID. Code was added to
+            ProcessMinidump.cpp to deal with this and allows partial UUID matching.
 
             This test verifies that if we have a minidump with a 16 byte UUID, that
             we are able to associate a symbol file with a 20 byte UUID only if the
             first 16 bytes match. In this case we will see the path from the file
             we found in the test directory and the 20 byte UUID from the actual
             file, not the 16 byte shortened UUID from the minidump.
         """
         so_path = self.getBuildArtifact("libuuidmatch.so")
         self.yaml2obj("libuuidmatch.yaml", so_path)
         cmd = 'settings set target.exec-search-paths "%s"' % (os.path.dirname(so_path))
         self.dbg.HandleCommand(cmd)
         modules = self.get_minidump_modules("linux-arm-partial-uuids-match.yaml")
         self.assertEqual(1, len(modules))
-        self.verify_module(modules[0], so_path, 
+        self.verify_module(modules[0], so_path,
                            "7295E17C-6668-9E05-CBB5-DEE5003865D5-5267C116")
 
     def test_partial_uuid_mismatch(self):
         """
             Breakpad has been known to create minidump files using CvRecord in each
             module whose signature is set to PDB70 where the UUID only contains the
-            first 16 bytes of a 20 byte ELF build ID. Code was added to 
-            ProcessMinidump.cpp to deal with this and allows partial UUID matching. 
-            
+            first 16 bytes of a 20 byte ELF build ID. Code was added to
+            ProcessMinidump.cpp to deal with this and allows partial UUID matching.
             This test verifies that if we have a minidump with a 16 byte UUID, that
             we are not able to associate a symbol file with a 20 byte UUID only if
             any of the first 16 bytes do not match. In this case we will see the UUID
             from the minidump file and the path from the minidump file.
         """
         so_path = self.getBuildArtifact("libuuidmismatch.so")
         self.yaml2obj("libuuidmismatch.yaml", so_path)
         cmd = 'settings set target.exec-search-paths "%s"' % (os.path.dirname(so_path))
         self.dbg.HandleCommand(cmd)
         modules = self.get_minidump_modules("linux-arm-partial-uuids-mismatch.yaml")
         self.assertEqual(1, len(modules))
         self.verify_module(modules[0],
-                           "/invalid/path/on/current/system/libuuidmismatch.so", 
+                           "/invalid/path/on/current/system/libuuidmismatch.so",
                            "7295E17C-6668-9E05-CBB5-DEE5003865D5")
 
     def test_relative_module_name(self):
         old_cwd = os.getcwd()
         self.addTearDownHook(lambda: os.chdir(old_cwd))
         os.chdir(self.getBuildDir())
         name = "file-with-a-name-unlikely-to-exist-in-the-current-directory.so"
         open(name, "a").close()
         modules = self.get_minidump_modules(
                 self.getSourcePath("relative_module_name.yaml"))
         self.assertEqual(1, len(modules))
         self.verify_module(modules[0], name, None)

lldb/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-arm-same-uuids.yaml

+--- !minidump
+Streams:
+  - Type:            SystemInfo
+    Processor Arch:  AMD64
+    Platform ID:     Linux
+    CSD Version:     '15E216'
+    CPU:
+      Vendor ID:       GenuineIntel
+      Version Info:    0x00000000
+      Feature Info:    0x00000000
+  - Type:            ModuleList
+    Modules:
+      - Base of Image:   0x0000000000001000
+        Size of Image:   0x00001000
+        Module Name:     '/file/does/not/exist/a'
+        CodeView Record: '52534453112233441122334411223344112233441122334411'
+      - Base of Image:   0x0000000000003000
+        Size of Image:   0x00001000
+        Module Name:     '/file/does/not/exist/b'
+        CodeView Record: '52534453112233441122334411223344112233441122334411'
+...

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

 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:
   PlaceholderObjectFile(const lldb::ModuleSP &module_sp,
-                        const ModuleSpec &module_spec, lldb::offset_t base,
-                        lldb::offset_t size)
+                        const ModuleSpec &module_spec, lldb::addr_t base,
+                        lldb::addr_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_up = std::make_unique<Symtab>(this);
   }
 
-  ConstString GetPluginName() override { return ConstString("placeholder"); }
+  static ConstString GetStaticPluginName() {
+    return ConstString("placeholder");
+  }
+  ConstString GetPluginName() override { return GetStaticPluginName(); }
   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; }
   UUID GetUUID() override { return m_uuid; }
bool SetLoadAddress(Target &target, addr_t value,
     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);
   }
 
+  lldb::addr_t GetBaseImageAddress() const { return m_base; }
 private:
   ArchSpec m_arch;
   UUID m_uuid;
-  lldb::offset_t m_base;
-  lldb::offset_t m_size;
+  lldb::addr_t m_base;
+  lldb::addr_t m_size;
 };
 } // namespace
 
 ConstString ProcessMinidump::GetPluginNameStatic() {
   static ConstString g_name("minidump");
   return g_name;
 }
 
bool ProcessMinidump::UpdateThreadList(ThreadList &old_thread_list,
   }
   return new_thread_list.GetSize(false) > 0;
 }
 
 void ProcessMinidump::ReadModuleList() {
   std::vector<const minidump::Module *> filtered_modules =
       m_minidump_parser->GetFilteredModuleList();
 
-  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES));
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
 
   for (auto module : filtered_modules) {
     std::string name = cantFail(m_minidump_parser->GetMinidumpFile().getString(
         module->ModuleNameRVA));
+    const uint64_t load_addr = module->BaseOfImage;
+    const uint64_t load_size = module->SizeOfImage;
     LLDB_LOG(log, "found module: name: {0} {1:x10}-{2:x10} size: {3}", name,
-             module->BaseOfImage, module->BaseOfImage + module->SizeOfImage,
-             module->SizeOfImage);
+             load_addr, load_addr + load_size, load_size);
 
     // check if the process is wow64 - a 32 bit windows process running on a
     // 64 bit windows
     if (llvm::StringRef(name).endswith_lower("wow64.dll")) {
       m_is_wow64 = true;
     }
 
     const auto uuid = m_minidump_parser->GetModuleUUID(module);
     auto file_spec = FileSpec(name, GetArchitecture().GetTriple());
     ModuleSpec module_spec(file_spec, uuid);
     module_spec.GetArchitecture() = GetArchitecture();
     Status error;
     // Try and find a module with a full UUID that matches. This function will
     // add the module to the target if it finds one.
-    lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec, 
+    lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec,
                                                      true /* notify */, &error);
     if (!module_sp) {
       // Try and find a module without specifying the UUID and only looking for
       // the file given a basename. We then will look for a partial UUID match
       // if we find any matches. This function will add the module to the
       // target if it finds one, so we need to remove the module from the target
       // if the UUID doesn't match during our manual UUID verification. This
       // allows the "target.exec-search-paths" setting to specify one or more
       // directories that contain executables that can be searched for matches.
       ModuleSpec basename_module_spec(module_spec);
       basename_module_spec.GetUUID().Clear();
       basename_module_spec.GetFileSpec().GetDirectory().Clear();
-      module_sp = GetTarget().GetOrCreateModule(basename_module_spec, 
-                                                     true /* notify */, &error);
+      module_sp = GetTarget().GetOrCreateModule(basename_module_spec,
       if (module_sp) {
         // We consider the module to be a match if the minidump UUID is a
         // prefix of the actual UUID, or if either of the UUIDs are empty.
         const auto dmp_bytes = uuid.GetBytes();
         const auto mod_bytes = module_sp->GetUUID().GetBytes();
         const bool match = dmp_bytes.empty() || mod_bytes.empty() ||
             mod_bytes.take_front(dmp_bytes.size()) == dmp_bytes;
         if (!match) {
             GetTarget().GetImages().Remove(module_sp);
             module_sp.reset();
         }
       }
     }
+    if (module_sp) {
+      // Watch out for place holder modules that have different paths, but the
+      // same UUID. If the base address is different, create a new module. If
+      // we don't then we will end up setting the load address of a different
+      // PlaceholderObjectFile and an assertion will fire.
+      auto *objfile = module_sp->GetObjectFile();
+      if (objfile && objfile->GetPluginName() ==
+          PlaceholderObjectFile::GetStaticPluginName()) {
+        if (((PlaceholderObjectFile *)objfile)->GetBaseImageAddress() !=
+            load_addr)
+          module_sp.reset();
+      }
+    }
     if (!module_sp) {
       // 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, ...)
       LLDB_LOG(log,
                "Unable to locate the matching object file, creating a "
                "placeholder module for: {0}",
                name);
 
       module_sp = Module::CreateModuleFromObjectFile<PlaceholderObjectFile>(
-          module_spec, module->BaseOfImage, module->SizeOfImage);
+          module_spec, load_addr, load_size);
       GetTarget().GetImages().Append(module_sp, true /* notify */);
     }
 
     bool load_addr_changed = false;
-    module_sp->SetLoadAddress(GetTarget(), module->BaseOfImage, false,
+    module_sp->SetLoadAddress(GetTarget(), load_addr, false,
                               load_addr_changed);
   }
 }
 
 bool ProcessMinidump::GetProcessInfo(ProcessInstanceInfo &info) {
   info.Clear();
   info.SetProcessID(GetID());
   info.SetArchitecture(GetArchitecture());