Minidump plugin: Fix flaky test

[dvlahovski]

Summary

One of the tests was flaky, because similarly to
https://reviews.llvm.org/D18697 (rL265391) - if there is a process running
which is with the same PID as in the core file, the minidump
core file debugging will fail, because we get some information from the
running process.
The fix is routing the ProcessInfo requests through the Process class
and overriding it in ProcessMinidump to return correct data.

[labath]

lgtm

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

 """
 Test basics of Minidump debugging.
 """
 
 from __future__ import print_function
 from six import iteritems
 
+import shutil
 
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
 class MiniDumpNewTestCase(TestBase):
 
     mydir = TestBase.compute_mydir(__file__)
 
     NO_DEBUG_INFO_TESTCASE = True
 
+    _linux_x86_64_pid = 29917
+    _linux_x86_64_not_crashed_pid = 29939
+    _linux_x86_64_not_crashed_pid_offset = 0xD967
+
     def test_process_info_in_minidump(self):
         """Test that lldb can read the process 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.assertTrue(self.process, PROCESS_IS_VALID)
         self.assertEqual(self.process.GetNumThreads(), 1)
-        self.assertEqual(self.process.GetProcessID(), 29917)
+        self.assertEqual(self.process.GetProcessID(), self._linux_x86_64_pid)
 
     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")
         # This process crashed due to a segmentation fault in its
         # one and only thread.
         self.assertEqual(self.process.GetNumThreads(), 1)
         thread = self.process.GetThreadAtIndex(0)
         self.assertEqual(thread.GetStopReason(), lldb.eStopReasonSignal)
         stop_description = thread.GetStopDescription(256)
         self.assertTrue("SIGSEGV" in stop_description)
 
     def test_stack_info_in_minidump(self):
         """Test that we can see a trivial stack in a breakpad-generated Minidump."""
         # target create linux-x86_64 -c linux-x86_64.dmp
         self.dbg.CreateTarget("linux-x86_64")
         self.target = self.dbg.GetSelectedTarget()
         self.process = self.target.LoadCore("linux-x86_64.dmp")
         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())
         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."""
+        """Test that if we load a snapshot minidump file (meaning the process
+        did not crash) there is no stop reason."""
         # target create -c linux-x86_64_not_crashed.dmp
         self.dbg.CreateTarget(None)
         self.target = self.dbg.GetSelectedTarget()
         self.process = self.target.LoadCore("linux-x86_64_not_crashed.dmp")
         self.assertEqual(self.process.GetNumThreads(), 1)
         thread = self.process.GetThreadAtIndex(0)
         self.assertEqual(thread.GetStopReason(), lldb.eStopReasonNone)
         stop_description = thread.GetStopDescription(256)
         self.assertEqual(stop_description, None)
 
-    def test_deeper_stack_in_minidump(self):
-        """Test that we can examine a more interesting stack in a Minidump."""
-        # Launch with the Minidump, and inspect the stack.
-        # target create linux-x86_64_not_crashed -c linux-x86_64_not_crashed.dmp
-        target = self.dbg.CreateTarget("linux-x86_64_not_crashed")
-        process = target.LoadCore("linux-x86_64_not_crashed.dmp")
+    def do_test_deeper_stack(self, binary, core, pid):
+        target = self.dbg.CreateTarget(binary)
+        process = target.LoadCore(core)
         thread = process.GetThreadAtIndex(0)
 
+        self.assertEqual(process.GetProcessID(), pid)
+
         expected_stack = {1: 'bar', 2: 'foo', 3: '_start'}
         self.assertGreaterEqual(thread.GetNumFrames(), len(expected_stack))
         for index, name in iteritems(expected_stack):
             frame = thread.GetFrameAtIndex(index)
             self.assertTrue(frame.IsValid())
             function_name = frame.GetFunctionName()
             self.assertTrue(name in function_name)
 
+    def test_deeper_stack_in_minidump(self):
+        """Test that we can examine a more interesting stack in a Minidump."""
+        # Launch with the Minidump, and inspect the stack.
+        # target create linux-x86_64_not_crashed -c linux-x86_64_not_crashed.dmp
+        self.do_test_deeper_stack("linux-x86_64_not_crashed",
+                                  "linux-x86_64_not_crashed.dmp",
+                                  self._linux_x86_64_not_crashed_pid)
+
+    def do_change_pid_in_minidump(self, core, newcore, offset, oldpid, newpid):
+        """ This assumes that the minidump is breakpad generated on Linux -
+        meaning that the PID in the file will be an ascii string part of
+        /proc/PID/status which is written in the file
+        """
+        shutil.copyfile(core, newcore)
+        with open(newcore, "r+") as f:
+            f.seek(offset)
+            self.assertEqual(f.read(5), oldpid)
+
+            f.seek(offset)
+            if len(newpid) < len(oldpid):
+                newpid += " " * (len(oldpid) - len(newpid))
+            newpid += "\n"
+            f.write(newpid)
+
+    def test_deeper_stack_in_minidump_with_same_pid_running(self):
+        """Test that we read the information from the core correctly even if we
+        have a running process with the same PID"""
+        try:
+            self.do_change_pid_in_minidump("linux-x86_64_not_crashed.dmp",
+                                           "linux-x86_64_not_crashed-pid.dmp",
+                                           self._linux_x86_64_not_crashed_pid_offset,
+                                           str(self._linux_x86_64_not_crashed_pid),
+                                           str(os.getpid()))
+            self.do_test_deeper_stack("linux-x86_64_not_crashed",
+                                      "linux-x86_64_not_crashed-pid.dmp",
+                                      os.getpid())
+        finally:
+            self.RemoveTempFile("linux-x86_64_not_crashed-pid.dmp")
+
+    def test_two_cores_same_pid(self):
+        """Test that we handle the situation if we have two core files with the same PID """
+        try:
+            self.do_change_pid_in_minidump("linux-x86_64_not_crashed.dmp",
+                                           "linux-x86_64_not_crashed-pid.dmp",
+                                           self._linux_x86_64_not_crashed_pid_offset,
+                                           str(self._linux_x86_64_not_crashed_pid),
+                                           str(self._linux_x86_64_pid))
+            self.do_test_deeper_stack("linux-x86_64_not_crashed",
+                                      "linux-x86_64_not_crashed-pid.dmp",
+                                      self._linux_x86_64_pid)
+            self.test_stack_info_in_minidump()
+        finally:
+            self.RemoveTempFile("linux-x86_64_not_crashed-pid.dmp")
+
     def test_local_variables_in_minidump(self):
         """Test that we can examine local variables in a Minidump."""
         # Launch with the Minidump, and inspect a local variable.
         # target create linux-x86_64_not_crashed -c linux-x86_64_not_crashed.dmp
         target = self.dbg.CreateTarget("linux-x86_64_not_crashed")
         process = target.LoadCore("linux-x86_64_not_crashed.dmp")
         thread = process.GetThreadAtIndex(0)
         frame = thread.GetFrameAtIndex(1)
         value = frame.EvaluateExpression('x')
         self.assertEqual(value.GetValueAsSigned(), 3)

source/Plugins/Process/minidump/ProcessMinidump.h

public:
 
   static void Terminate();
 
   static ConstString GetPluginNameStatic();
 
   static const char *GetPluginDescriptionStatic();
 
   ProcessMinidump(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
-                  const lldb_private::FileSpec &core_file,
-                  MinidumpParser minidump_parser);
+                  const FileSpec &core_file, MinidumpParser minidump_parser);
 
   ~ProcessMinidump() override;
 
   bool CanDebug(lldb::TargetSP target_sp,
                 bool plugin_specified_by_name) override;
 
   Error DoLoadCore() override;
 
public:
   size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
                       Error &error) override;
 
   ArchSpec GetArchitecture();
 
   Error GetMemoryRegionInfo(lldb::addr_t load_addr,
                             MemoryRegionInfo &range_info) override;
 
+  bool GetProcessInfo(ProcessInstanceInfo &info) override;
+
   MinidumpParser m_minidump_parser;
 
 protected:
   void Clear();
 
   bool UpdateThreadList(ThreadList &old_thread_list,
                         ThreadList &new_thread_list) override;
 

source/Plugins/Process/minidump/ProcessMinidump.cpp

void ProcessMinidump::RefreshStateAfterStop() {
   stop_thread->SetStopInfo(stop_info);
 }
 
 bool ProcessMinidump::IsAlive() { return true; }
 
 bool ProcessMinidump::WarnBeforeDetach() const { return false; }
 
 size_t ProcessMinidump::ReadMemory(lldb::addr_t addr, void *buf, size_t size,
-                                   lldb_private::Error &error) {
+                                   Error &error) {
   // Don't allow the caching that lldb_private::Process::ReadMemory does
   // since we have it all cached in our dump file anyway.
   return DoReadMemory(addr, buf, size, error);
 }
 
 size_t ProcessMinidump::DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
-                                     lldb_private::Error &error) {
+                                     Error &error) {
 
   llvm::ArrayRef<uint8_t> mem = m_minidump_parser.GetMemory(addr, size);
   if (mem.empty()) {
     error.SetErrorString("could not parse memory info");
     return 0;
   }
 
   std::memcpy(buf, mem.data(), mem.size());
   return mem.size();
 }
 
 ArchSpec ProcessMinidump::GetArchitecture() {
   if (!m_is_wow64) {
     return m_minidump_parser.GetArchitecture();
   }
 
   llvm::Triple triple;
   triple.setVendor(llvm::Triple::VendorType::UnknownVendor);
   triple.setArch(llvm::Triple::ArchType::x86);
   triple.setOS(llvm::Triple::OSType::Win32);
   return ArchSpec(triple);
 }
 
-Error ProcessMinidump::GetMemoryRegionInfo(
-    lldb::addr_t load_addr, lldb_private::MemoryRegionInfo &range_info) {
+Error ProcessMinidump::GetMemoryRegionInfo(lldb::addr_t load_addr,
+                                           MemoryRegionInfo &range_info) {
   Error error;
   auto info = m_minidump_parser.GetMemoryRegionInfo(load_addr);
   if (!info) {
     error.SetErrorString("No valid MemoryRegionInfo found!");
     return error;
   }
   range_info = info.getValue();
   return error;
 }
 
 void ProcessMinidump::Clear() { Process::m_thread_list.Clear(); }
 
-bool ProcessMinidump::UpdateThreadList(
-    lldb_private::ThreadList &old_thread_list,
-    lldb_private::ThreadList &new_thread_list) {
+bool ProcessMinidump::UpdateThreadList(ThreadList &old_thread_list,
+                                       ThreadList &new_thread_list) {
   uint32_t num_threads = 0;
   if (m_thread_list.size() > 0)
     num_threads = m_thread_list.size();
 
   for (lldb::tid_t tid = 0; tid < num_threads; ++tid) {
     llvm::ArrayRef<uint8_t> context;
     if (!m_is_wow64)
       context = m_minidump_parser.GetThreadContext(m_thread_list[tid]);
if (log) {
                   name.getValue().c_str());
     }
 
     bool load_addr_changed = false;
     module_sp->SetLoadAddress(GetTarget(), module->base_of_image, false,
                               load_addr_changed);
   }
 }
+
+bool ProcessMinidump::GetProcessInfo(ProcessInstanceInfo &info) {
+  info.Clear();
+  info.SetProcessID(GetID());
+  info.SetArchitecture(GetArchitecture());
+  lldb::ModuleSP module_sp = GetTarget().GetExecutableModule();
+  if (module_sp) {
+    const bool add_exe_file_as_first_arg = false;
+    info.SetExecutableFile(GetTarget().GetExecutableModule()->GetFileSpec(),
+                           add_exe_file_as_first_arg);
+  }
+  return true;
+}