Fix a bug in linux core file handling

[labath]

Summary

There was a bug in linux core file handling, where if there was a running process with the same
process id as the id in the core file, the core file debugging would fail, as we would pull some
pieces of information (ProcessInfo structure) from the running process instead of the core file.
I fix this by routing the ProcessInfo requests through the Process class and overriding it in
ProcessElfCore to return correct data.

A (slightly convoluted) test is included.

[zturner]

What if you are simultaneously debugging 2 processes with the same pid?
Does this fail?

[clayborg]

See inlined comments.

source/Plugins/Process/elf-core/ProcessElfCore.cpp

644–647 ↗

(On Diff #52379)

Can the ELF core really not figure out what its executable is? Shouldn't it be filling in info.m_executable by calling info.SetExecutableFile(...)?

source/Target/Process.cpp

6487 ↗

(On Diff #52379)

We should probably be extra safe here and do:

PlatformSP platform_sp = GetTarget().GetPlatform();
if (platform_sp)
    return platform_sp->GetProcessInfo(GetID(), info);
else
    return false;

[labath]

Address review comments.

[labath]

I have also added a test which loads two core files with the same pid. (Based on very little experimentation) everything seems to be working ok for that case.

source/Plugins/Process/elf-core/ProcessElfCore.cpp

644–647 ↗

(On Diff #52572)

It get's parsed and set as the main target module in DoLoadCore, so I load the info from there.

lldb/trunk/include/lldb/Target/Process.h

#endif
     ///
     /// @param [in] sc
     ///     A SymbolContext with eSymbolContextFunction and eSymbolContextModule
     ///     pre-computed.
     //------------------------------------------------------------------
     void
     PrintWarningOptimization (const SymbolContext &sc);
 
+    virtual bool
+    GetProcessInfo(ProcessInstanceInfo &info);
+
 public:
     //------------------------------------------------------------------
     /// Get the exit status for a process.
     ///
     /// @return
     ///     The process's return code, or -1 if the current process
     ///     state is not eStateExited.
     //------------------------------------------------------------------

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/linux-core/TestLinuxCore.py

 """
 Test basics of linux core file debugging.
 """
 
 from __future__ import print_function
 
+import shutil
+import struct
+
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 class LinuxCoreTestCase(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
 
     mydir = TestBase.compute_mydir(__file__)
 
+    _i386_pid   = 32306
+    _x86_64_pid = 32259
+
     @skipIf(bugnumber="llvm.org/pr26947")
-    @no_debug_info_test
     def test_i386(self):
         """Test that lldb can read the process information from an i386 linux core file."""
-        self.do_test("i386", 32306)
+        self.do_test("i386", self._i386_pid)
 
-    @no_debug_info_test
     def test_x86_64(self):
         """Test that lldb can read the process information from an x86_64 linux core file."""
-        self.do_test("x86_64", 32259)
+        self.do_test("x86_64", self._x86_64_pid)
 
-    def do_test(self, arch, pid):
-        target = self.dbg.CreateTarget(arch + ".out")
-        process = target.LoadCore(arch + ".core")
+    def test_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 around"""
+        try:
+            shutil.copyfile("x86_64.out",  "x86_64-pid.out")
+            shutil.copyfile("x86_64.core", "x86_64-pid.core")
+            with open("x86_64-pid.core", "r+b") as f:
+                # These are offsets into the NT_PRSTATUS and NT_PRPSINFO structures in the note
+                # segment of the core file. If you update the file, these offsets may need updating
+                # as well. (Notes can be viewed with readelf --notes.)
+                for pid_offset in [0x1c4, 0x320]:
+                    f.seek(pid_offset)
+                    self.assertEqual(struct.unpack("<I", f.read(4))[0], self._x86_64_pid)
+
+                    # We insert our own pid, and make sure the test still works.
+                    f.seek(pid_offset)
+                    f.write(struct.pack("<I", os.getpid()))
+            self.do_test("x86_64-pid", os.getpid())
+        finally:
+            self.RemoveTempFile("x86_64-pid.out")
+            self.RemoveTempFile("x86_64-pid.core")
+
+    def test_two_cores_same_pid(self):
+        """Test that we handle the situation if we have two core files with the same PID
+        around"""
+        alttarget = self.dbg.CreateTarget("altmain.out")
+        altprocess = alttarget.LoadCore("altmain.core")
+        self.assertTrue(altprocess, PROCESS_IS_VALID)
+        self.assertEqual(altprocess.GetNumThreads(), 1)
+        self.assertEqual(altprocess.GetProcessID(), self._x86_64_pid)
+
+        altframe = altprocess.GetSelectedThread().GetFrameAtIndex(0)
+        self.assertEqual(altframe.GetFunctionName(), "_start")
+        self.assertEqual(altframe.GetLineEntry().GetLine(), line_number("altmain.c", "Frame _start"))
+
+        error = lldb.SBError()
+        F = altprocess.ReadCStringFromMemory(altframe.FindVariable("F").GetValueAsUnsigned(), 256, error)
+        self.assertTrue(error.Success())
+        self.assertEqual(F, "_start")
+
+        # without destroying this process, run the test which opens another core file with the
+        # same pid
+        self.do_test("x86_64", self._x86_64_pid)
+
+    def do_test(self, filename, pid):
+        target = self.dbg.CreateTarget(filename + ".out")
+        process = target.LoadCore(filename + ".core")
         self.assertTrue(process, PROCESS_IS_VALID)
         self.assertEqual(process.GetNumThreads(), 1)
         self.assertEqual(process.GetProcessID(), pid)
 
         thread = process.GetSelectedThread()
         self.assertTrue(thread)
         self.assertEqual(thread.GetThreadID(), pid)
         backtrace = ["bar", "foo", "_start"]
         self.assertEqual(thread.GetNumFrames(), len(backtrace))
         for i in range(len(backtrace)):
             frame = thread.GetFrameAtIndex(i)
             self.assertTrue(frame)
             self.assertEqual(frame.GetFunctionName(), backtrace[i])
             self.assertEqual(frame.GetLineEntry().GetLine(),
                     line_number("main.c", "Frame " + backtrace[i]))
             self.assertEqual(frame.FindVariable("F").GetValueAsUnsigned(), ord(backtrace[i][0]))

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/linux-core/altmain.c

+void _start(void)
+{
+    const char *F = "_start";
+    char *boom = (char *)0;
+    *boom = 47; // Frame _start
+}

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/linux-core/make-core.sh

 #! /bin/bash
 
 set -e -x
 
+file=$1
+if [ -z "$file" ]; then
+    cat <<EOF
+Please supply the main source file as the first argument.
+EOF
+    exit 1
+fi
+
 if grep -q '^|' </proc/sys/kernel/core_pattern; then
     cat <<EOF
 Your system uses a crash report tool ($(cat /proc/sys/kernel/core_pattern)). Core files
 will not be generated.  Please reset /proc/sys/kernel/core_pattern (requires root
 privileges) to enable core generation.
 EOF
     exit 1
 fi
 
 ulimit -c 1000
 real_limit=$(ulimit -c)
 if [ $real_limit -lt 100 ]; then
     cat <<EOF
 Unable to increase the core file limit. Core file may be truncated!
 To fix this, increase HARD core file limit (ulimit -H -c 1000). This may require root
 privileges.
 EOF
 fi
 
-${CC:-cc} -nostdlib -static -g $CFLAGS main.c -o a.out
+${CC:-cc} -nostdlib -static -g $CFLAGS "$file" -o a.out
 
 cat <<EOF
 Executable file is in a.out.
 Core file will be saved according to pattern $(cat /proc/sys/kernel/core_pattern).
 EOF
 
 ulimit -s 8 # Decrease stack size to 8k => smaller core files.
 exec ./a.out

lldb/trunk/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp

DynamicLoaderPOSIXDYLD::ResolveExecutableModule (lldb::ModuleSP &module_sp)
 
     if (m_process == nullptr)
         return;
 
     auto &target = m_process->GetTarget ();
     const auto platform_sp = target.GetPlatform ();
 
     ProcessInstanceInfo process_info;
-    if (!platform_sp->GetProcessInfo (m_process->GetID (), process_info))
+    if (!m_process->GetProcessInfo(process_info))
     {
         if (log)
             log->Printf ("DynamicLoaderPOSIXDYLD::%s - failed to get process info for pid %" PRIu64,
                          __FUNCTION__, m_process->GetID ());
         return;
     }
 
     if (log)

lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.h

public:
 
     lldb_private::ArchSpec
     GetArchitecture();
 
     // Returns AUXV structure found in the core file
     const lldb::DataBufferSP
     GetAuxvData() override;
 
+    bool
+    GetProcessInfo(lldb_private::ProcessInstanceInfo &info) override;
+
 protected:
     void
     Clear ( );
 
     bool UpdateThreadList(lldb_private::ThreadList &old_thread_list,
                           lldb_private::ThreadList &new_thread_list) override;
 
 private:

lldb/trunk/source/Plugins/Process/elf-core/ProcessElfCore.cpp

 const lldb::DataBufferSP
 ProcessElfCore::GetAuxvData()
 {
     const uint8_t *start = m_auxv.GetDataStart();
     size_t len = m_auxv.GetByteSize();
     lldb::DataBufferSP buffer(new lldb_private::DataBufferHeap(start, len));
     return buffer;
 }
+
+bool
+ProcessElfCore::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;
+}

lldb/trunk/source/Target/Process.cpp

if (platform_sp)
                 GetTarget().SetArchitecture(platform_arch);
                 if (log)
                     log->Printf ("Process::%s switching platform to %s and architecture to %s based on info from attach", __FUNCTION__, platform_sp->GetName().AsCString (""), platform_arch.GetTriple().getTriple().c_str ());
             }
         }
         else if (!process_arch.IsValid())
         {
             ProcessInstanceInfo process_info;
-            platform_sp->GetProcessInfo (GetID(), process_info);
+            GetProcessInfo(process_info);
             const ArchSpec &process_arch = process_info.GetArchitecture();
             if (process_arch.IsValid() && !GetTarget().GetArchitecture().IsExactMatch(process_arch))
             {
                 GetTarget().SetArchitecture (process_arch);
                 if (log)
                     log->Printf ("Process::%s switching architecture to %s based on info the platform retrieved for pid %" PRIu64, __FUNCTION__, process_arch.GetTriple().getTriple().c_str (), GetID ());
             }
         }
if (GetWarningsOptimization()
         && !sc.module_sp->GetFileSpec().GetFilename().IsEmpty()
         && sc.function
         && sc.function->GetIsOptimized())
     {
         PrintWarning (Process::Warnings::eWarningsOptimization, sc.module_sp.get(), "%s was compiled with optimization - stepping may behave oddly; variables may not be available.\n", sc.module_sp->GetFileSpec().GetFilename().GetCString());
     }
 }
 
+bool
+Process::GetProcessInfo(ProcessInstanceInfo &info)
+{
+    info.Clear();
+
+    PlatformSP platform_sp = GetTarget().GetPlatform();
+    if (! platform_sp)
+        return false;
+
+    return platform_sp->GetProcessInfo(GetID(), info);
+}
+
 ThreadCollectionSP
 Process::GetHistoryThreads(lldb::addr_t addr)
 {
     ThreadCollectionSP threads;
 
     const MemoryHistorySP &memory_history = MemoryHistory::FindPlugin(shared_from_this());
     
     if (!memory_history) {