Enable saving of mini dumps with lldb process save-core.

[amccarth]

Summary

Enable the saving of a mini dump (on Windows) using the existing lldb save-core command.

Also plumbs a new method into SBProcess, and adds a very basic test to make sure it produces a file.

Future tests will open the mini dump in LLDB and make sure that it has the right information.

[zturner]

+greg since this touches some SB stuff and the ObjectFile plugins.

include/lldb/API/SBProcess.h

345–346 ↗

(On Diff #40575)

Greg, is there any reason why this couldn't be an llvm::StringRef? I know historically we have used this char* mechanism, but it seems like StringRef could be a better, safer alternative than passing raw strings through the SB API. As long as you provide an appropriate typemap, everything should work out.

packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py

20 ↗

(On Diff #40575)

How do you feel about @skipIf(oslist=not_in(['windows']))?

The generic skipIf and expectedFailure decorators were extended recently and are now flexible enough to handle unless cases, so I'm partial to the idea of eventually killing off the rest of the decorators, and just having a single one that handles everything. But I want to dogfood the idea first and see what people think about it :)

32 ↗

(On Diff #40575)

Can you assert true that the file exists? Is there some thing additional that we can add where you then load the core and try to read something out (say the executable name), and assert that it's equal to a.out?

Right now all we know is that there's a file.

33 ↗

(On Diff #40575)

This isn't exception safe right now. For example, it won't unlink the file if one the asserts fails.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp

17 ↗

(On Diff #40575)

I think it would be better to just not even compile this file on non-Windows. Exclude it at the CMake level, and have SaveCore() return false directly. In the future we may add a way to deal with mini dumps outside of the Windows API, and then we can add that as a separate file like ObjectFilePECOFFMiniDumpRaw.cpp which can be used always, and then delete this one.

[clayborg]

See inlined comments.

include/lldb/API/SBProcess.h

344–346 ↗

(On Diff #40575)

I don't want any llvm in our public API. Especially llvm::StringRef as it likes to assert when people use it incorrectly (construct one with nullptr and it will assert). So no llvm or clang in our public API.

source/API/SBProcess.cpp

1433–1437 ↗

(On Diff #40575)

You need to check your process_sp before using it since it comes from a std::weak_ptr<>. You also need to take the process run lock. You also don't need to create a lldb::SBFileSpec since you actually need a lldb_private::FileSpec. The code should be:

lldb::SBError
SBProcess::SaveCore(const char *file_name)
{
    lldb::SBError error;
    if (process_sp)
    {
        Mutex::Locker api_locker (process_sp->GetTarget().GetAPIMutex());
        FileSpec core_file(file_name, false);
        error.ref() = PluginManager::SaveCore(process_sp, core_file.get());
    }
    else
        error.SetErrorString ("SBProcess is invalid");
    return error;
}

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp

52 ↗

(On Diff #40575)

You should fill in the error and probably use a #else clause:

#else

error.SetErrorString("windows mini dumps are only supported on native windows machines");
return false;

#endif

[clayborg]

Code correction and added that we should make sure the process is alive and stopped.

source/API/SBProcess.cpp

1433–1437 ↗

(On Diff #40575)

Code correction:

lldb::SBError
SBProcess::SaveCore(const char *file_name)
{
    lldb::SBError error;
    if (process_sp)
    {
        Mutex::Locker api_locker (process_sp->GetTarget().GetAPIMutex());
        FileSpec core_file(file_name, false);
        error.ref() = PluginManager::SaveCore(process_sp, core_file);
    }
    else
        error.SetErrorString ("SBProcess is invalid");
    return error;
}

We might also want to check to make sure the process is alive (the "save-core" command makes sure the process has been launched) and we should also make sure it does the right thing if the process is running (stop the process, make a core file, then resume if the process was running, or just fail if the process is running).

[zturner]

Fair enough. I figured it was worth asking about, but the rationale makes
sense.

[zturner]

source/API/SBProcess.cpp

1433–1437 ↗

(On Diff #40575)

I actually wonder if it should just generate an error if the process is not stopped. Putting a bunch of extra logic in the implementation of SaveCore() seems counter to the "thin" nature of the wrapping. Seems to me like the person using the SB API should stop it themselves before calling SaveCore.

What do you think?

[clayborg]

Yes, I agree Zach, the caller of SaveCore should verify the process is in a good state first. The only downside of that is if the windows mini-dumper can actually make a core file on a running process, we might want to allow this and make actual SaveCore implementations make that call?

[zturner]

In D14793#293007, @clayborg wrote:

Yes, I agree Zach, the caller of SaveCore should verify the process is in a good state first. The only downside of that is if the windows mini-dumper can actually make a core file on a running process, we might want to allow this and make actual SaveCore implementations make that call?

Windows has to have a stopped process before you can make a core. In other windows debuggers (MSVC and WinDbg, for example) the user of the debugger has to issue a stop command before the "Save Core Dump" command will work.

[clayborg]

OK, then callers of SaveCore should make sure the process is in a good state before calling the API.

[amccarth]

include/lldb/API/SBProcess.h

344–346 ↗

(On Diff #40575)

Acknowledged.

packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py

20 ↗

(On Diff #40575)

Where is not_in defined?

source/API/SBProcess.cpp

1433–1437 ↗

(On Diff #40575)

OK, I check process_sp, I take the mutex, and I ensure that the process is already stopped.

I also added another test to make sure SaveCore fails if the process is not stopped.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp

17 ↗

(On Diff #40575)

But if I pull this file out, then I have to have special casing in both the CMake and ObjectFilePECOFF file, which seems a violation of DRY.

If we make a general mini dump writer for any platform, then this just replaces this implementation.

52 ↗

(On Diff #40575)

Setting the error here would create a subtle bug.

Returning false will cause the PluginManager to keep trying other plugins to see if there's one that can handle it. If none of the plugins succeed, it produces a "no ObjectFile plugins were able to save a core for this process" message. But if one of them does, it returns whatever the last error set was.

It seems lame that PluginManager::SaveCore is sharing the same instance of the error result across attempts. I'd be happy to fix it there.

As for the #else, that was my original inclination, but I've been told that LLVM/LLDB style is to prefer early returns.

[amccarth]

Addresses some of the comments.

[clayborg]

Fix the caps on the error message, remove the new files and inline the code and this will be ready.

source/API/SBProcess.cpp

1445 ↗

(On Diff #40716)

remove the caps on "The" so it it just "the".

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

159 ↗

(On Diff #40716)

I would rather just have the code from SaveMiniDump inlined in this file, no need for a new file just for one function.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp

52–53 ↗

(On Diff #40716)

That is fine and returning false makes more sense after reading your above comment.

53–55 ↗

(On Diff #40716)

Remove this file and inline the code into ObjectFilePECOFF::SaveCore().

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.h

1–24 ↗

(On Diff #40716)

Remove this file and inline the code into ObjectFilePECOFF::SaveCore().

[amccarth]

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

159 ↗

(On Diff #40716)

I would prefer that too, but it's not possible.

This implementation depends on Windows headers which create conflicts with identifiers used in the main file. (I had a note about that somewhere. If I can't find it, I'll add a comment explanining that.)

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp

53–55 ↗

(On Diff #40716)

Can't do, as explained.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.h

1–24 ↗

(On Diff #40716)

Can't do as explained in previous response.

[clayborg]

Ok, so fix the caps and rename the file to be WindowsMiniDump.cpp/.h. I would rather it not have the "ObjectFilePECOFF" prefix because it would indicated that it is a new subclass of ObjectFile.

[amccarth]

The new files are renamed per clayborg's suggestion.

packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py

21 ↗

(On Diff #40716)

Given that this is proving to be a diversion right now, I'm going back to skipUnlessWindows.

[amccarth]

Renames the files per the suggestion and switches back to the @skipIfWindows decorator on the tests (for now).

[clayborg]

Looks good. Thanks for all the fixes.

[amccarth]

Greg: Since this adds a new file and I don't have a Mac, can you update
the Xcode project?

[clayborg]

Yep, I took care of that shortly after your commit.

lldb/trunk/include/lldb/API/SBProcess.h

public:
     GetExtendedBacktraceTypeAtIndex (uint32_t idx);
     
     lldb::SBThreadCollection
     GetHistoryThreads (addr_t addr);
     
     bool
     IsInstrumentationRuntimePresent(InstrumentationRuntimeType type);
 
+    // Save the state of the process in a core file (or mini dump on Windows).
+    lldb::SBError
+    SaveCore(const char *file_name);
+
 protected:
     friend class SBAddress;
     friend class SBBreakpoint;
     friend class SBBreakpointLocation;
     friend class SBCommandInterpreter;
     friend class SBDebugger;
     friend class SBExecutionContext;
     friend class SBFunction;

lldb/trunk/include/lldb/API/SBTarget.h

Launch (SBListener &listener,
             const char *stdin_path,
             const char *stdout_path,
             const char *stderr_path,
             const char *working_directory,
             uint32_t launch_flags,   // See LaunchFlags
             bool stop_at_entry,
             lldb::SBError& error);
 
+    SBProcess
+    LoadCore (const char *core_file);
+
     //------------------------------------------------------------------
     /// Launch a new process with sensible defaults.
     ///
     /// @param[in] argv
     ///     The argument array.
     ///
     /// @param[in] envp
     ///     The environment array.
public:
     LaunchSimple (const char **argv, 
                   const char **envp,
                   const char *working_directory);
     
     SBProcess
     Launch (SBLaunchInfo &launch_info, SBError& error);
     
     SBProcess
-    LoadCore (const char *core_file);
-
-    SBProcess
     Attach (SBAttachInfo &attach_info, SBError& error);
 
     //------------------------------------------------------------------
     /// Attach to process with pid.
     ///
     /// @param[in] listener
     ///     An optional listener that will receive all process events.
     ///     If \a listener is valid then \a listener will listen to all

lldb/trunk/packages/Python/lldbsuite/test/functionalities/process_save_core/Makefile

+LEVEL = ../../make
+
+CXX_SOURCES := main.cpp
+
+include $(LEVEL)/Makefile.rules
+

lldb/trunk/packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py

+"""
+Test saving a core file (or mini dump).
+"""
+
+from __future__ import print_function
+
+import os, time
+import lldb
+from lldbsuite.test.lldbtest import *
+
+class ProcessSaveCoreTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @not_remote_testsuite_ready
+    @skipUnlessWindows
+    def test_cannot_save_core_unless_process_stopped(self):
+        """Test that SaveCore fails if the process isn't stopped."""
+        self.build()
+        exe = os.path.join(os.getcwd(), "a.out")
+        core = os.path.join(os.getcwd(), "core.dmp")
+        target = self.dbg.CreateTarget(exe)
+        process = target.LaunchSimple(None, None, self.get_process_working_directory())
+        self.assertNotEqual(process.GetState(), lldb.eStateStopped)
+        error = process.SaveCore(core)
+        self.assertTrue(error.Fail())
+
+    @not_remote_testsuite_ready
+    @skipUnlessWindows
+    def test_save_windows_mini_dump(self):
+        """Test that we can save a Windows mini dump."""
+        self.build()
+        exe = os.path.join(os.getcwd(), "a.out")
+        core = os.path.join(os.getcwd(), "core.dmp")
+        try:
+            target = self.dbg.CreateTarget(exe)
+            breakpoint = target.BreakpointCreateByName("bar")
+            process = target.LaunchSimple(None, None, self.get_process_working_directory())
+            self.assertEqual(process.GetState(), lldb.eStateStopped)
+            self.assertTrue(process.SaveCore(core))
+            self.assertTrue(os.path.isfile(core))
+            self.assertTrue(process.Kill().Success())
+
+            # To verify, we'll launch with the mini dump, and ensure that we see
+            # the executable in the module list.
+            target = self.dbg.CreateTarget(None)
+            process = target.LoadCore(core)
+            files = [target.GetModuleAtIndex(i).GetFileSpec() for i in range(0, target.GetNumModules())]
+            paths = [os.path.join(f.GetDirectory(), f.GetFilename()) for f in files]
+            self.assertTrue(exe in paths)
+
+        finally:
+            # Clean up the mini dump file.
+            self.assertTrue(self.dbg.DeleteTarget(target))
+            if (os.path.isfile(core)):
+                os.unlink(core)
+
+

lldb/trunk/packages/Python/lldbsuite/test/functionalities/process_save_core/main.cpp

+int global = 42;
+
+int
+bar(int x)
+{
+  int y = 4*x + global;
+  return y;
+}
+
+int
+foo(int x)
+{
+  int y = 2*bar(3*x);
+  return y;
+}
+
+int
+main()
+{
+  return 0 * foo(1);
+}

lldb/trunk/scripts/interface/SBProcess.i

public:
     GetExtendedBacktraceTypeAtIndex (uint32_t idx);
 
     lldb::SBThreadCollection
     GetHistoryThreads (addr_t addr);
              
     bool
     IsInstrumentationRuntimePresent(lldb::InstrumentationRuntimeType type);
 
+    lldb::SBError
+    SaveCore(const char *file_name);
+
     %pythoncode %{
         def __get_is_alive__(self):
             '''Returns "True" if the process is currently alive, "False" otherwise'''
             s = self.GetState()
             if (s == eStateAttaching or 
                 s == eStateLaunching or 
                 s == eStateStopped or 
                 s == eStateRunning or 

lldb/trunk/source/API/SBProcess.cpp

 
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-types.h"
 
 #include "lldb/Interpreter/Args.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/PluginManager.h"
 #include "lldb/Core/State.h"
 #include "lldb/Core/Stream.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/SystemRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
SBProcess::IsInstrumentationRuntimePresent(InstrumentationRuntimeType type)
     
     InstrumentationRuntimeSP runtime_sp = process_sp->GetInstrumentationRuntime(type);
     
     if (! runtime_sp.get())
         return false;
     
     return runtime_sp->IsActive();
 }
+
+lldb::SBError
+SBProcess::SaveCore(const char *file_name)
+{
+    lldb::SBError error;
+    ProcessSP process_sp(GetSP());
+    if (!process_sp)
+    {
+        error.SetErrorString("SBProcess is invalid");
+        return error;
+    }
+
+    Mutex::Locker api_locker(process_sp->GetTarget().GetAPIMutex());
+
+    if (process_sp->GetState() != eStateStopped)
+    {
+        error.SetErrorString("the process is not stopped");
+        return error;
+    }
+
+    FileSpec core_file(file_name, false);
+    error.ref() = PluginManager::SaveCore(process_sp, core_file);
+    return error;
+}

lldb/trunk/source/Commands/CommandObjectTarget.cpp

DoExecute (Args& command, CommandReturnObject &result) override
         {
             FileSpec symfile (m_symbol_file.GetOptionValue().GetCurrentValue());
             if (symfile)
             {
                 if (symfile.Exists())
                 {
                     if (!symfile.Readable())
                     {
-                        result.AppendErrorWithFormat("symbol file '%s' is not readable", core_file.GetPath().c_str());
+                        result.AppendErrorWithFormat("symbol file '%s' is not readable", symfile.GetPath().c_str());
                         result.SetStatus (eReturnStatusFailed);
                         return false;
                     }
                 }
                 else
                 {
                     char symfile_path[PATH_MAX];
                     symfile.GetPath(symfile_path, sizeof(symfile_path));

lldb/trunk/source/Plugins/ObjectFile/PECOFF/CMakeLists.txt

 add_lldb_library(lldbPluginObjectFilePECOFF
   ObjectFilePECOFF.cpp
+  WindowsMiniDump.cpp
   )

lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h

public:
     GetModuleSpecifications (const lldb_private::FileSpec& file,
                              lldb::DataBufferSP& data_sp,
                              lldb::offset_t data_offset,
                              lldb::offset_t file_offset,
                              lldb::offset_t length,
                              lldb_private::ModuleSpecList &specs);
 
     static bool
+    SaveCore (const lldb::ProcessSP &process_sp,
+              const lldb_private::FileSpec &outfile,
+              lldb_private::Error &error);
+
+    static bool
     MagicBytesMatch (lldb::DataBufferSP& data_sp);
     
     bool
     ParseHeader() override;
     
     bool
     SetLoadAddress(lldb_private::Target &target, lldb::addr_t value, bool value_is_offset) override;
 

lldb/trunk/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

 //===-- ObjectFilePECOFF.cpp ------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ObjectFilePECOFF.h"
+#include "WindowsMiniDump.h"
 
 #include "llvm/Support/COFF.h"
 
 #include "lldb/Core/ArchSpec.h"
 #include "lldb/Core/DataBuffer.h"
 #include "lldb/Host/FileSpec.h"
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/StreamString.h"
 #include "lldb/Core/Timer.h"
 #include "lldb/Core/UUID.h"
 #include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 
 #define IMAGE_DOS_SIGNATURE             0x5A4D      // MZ
 #define IMAGE_NT_SIGNATURE              0x00004550  // PE00
 #define OPT_HEADER_MAGIC_PE32           0x010b
 #define OPT_HEADER_MAGIC_PE32_PLUS      0x020b
 
 using namespace lldb;
 using namespace lldb_private;
 
 void
 ObjectFilePECOFF::Initialize()
 {
     PluginManager::RegisterPlugin (GetPluginNameStatic(),
                                    GetPluginDescriptionStatic(),
                                    CreateInstance,
                                    CreateMemoryInstance,
-                                   GetModuleSpecifications);
+                                   GetModuleSpecifications,
+                                   SaveCore);
 }
 
 void
 ObjectFilePECOFF::Terminate()
 {
     PluginManager::UnregisterPlugin (CreateInstance);
 }
 
if (ObjectFilePECOFF::MagicBytesMatch(data_sp))
                 }
             }
         }
     }
 
     return specs.GetSize() - initial_count;
 }
 
+bool
+ObjectFilePECOFF::SaveCore(const lldb::ProcessSP &process_sp,
+                           const lldb_private::FileSpec &outfile,
+                           lldb_private::Error &error)
+{
+    return SaveMiniDump(process_sp, outfile, error);
+}
+
 
 bool
 ObjectFilePECOFF::MagicBytesMatch (DataBufferSP& data_sp)
 {
     DataExtractor data(data_sp, eByteOrderLittle, 4);
     lldb::offset_t offset = 0;
     uint16_t magic = data.GetU16 (&offset);
     return magic == IMAGE_DOS_SIGNATURE;

lldb/trunk/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h

+//===-- WindowsMiniDump.h ---------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef liblldb_WindowsMiniDump_h_
+#define liblldb_WindowsMiniDump_h_
+
+#include "lldb/Target/Process.h"
+
+namespace lldb_private {
+
+bool
+SaveMiniDump(const lldb::ProcessSP &process_sp,
+             const lldb_private::FileSpec &outfile,
+             lldb_private::Error &error);
+
+}  // namespace lldb_private
+
+#endif

lldb/trunk/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp

+//===-- WindowsMiniDump.cpp -------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// This function is separated out from ObjectFilePECOFF.cpp to name avoid name
+// collisions with WinAPI preprocessor macros.
+
+#include "WindowsMiniDump.h"
+#include "lldb/Host/FileSpec.h"
+#include "llvm/Support/ConvertUTF.h"
+
+#ifdef _WIN32
+#include "lldb/Host/windows/windows.h"
+#include <DbgHelp.h>  // for MiniDumpWriteDump
+#endif
+
+namespace lldb_private {
+
+bool
+SaveMiniDump(const lldb::ProcessSP &process_sp,
+             const lldb_private::FileSpec &outfile,
+             lldb_private::Error &error)
+{
+    if (!process_sp) return false;
+#ifdef _WIN32
+    HANDLE process_handle = ::OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, process_sp->GetID());
+    const std::string file_name = outfile.GetCString();
+    std::wstring wide_name;
+    wide_name.resize(file_name.size() + 1);
+    char * result_ptr = reinterpret_cast<char *>(&wide_name[0]);
+    const UTF8 *error_ptr = nullptr;
+    if (!llvm::ConvertUTF8toWide(sizeof(wchar_t), file_name, result_ptr, error_ptr)) {
+        error.SetErrorString("cannot convert file name");
+        return false;
+    }
+    HANDLE file_handle = ::CreateFileW(wide_name.c_str(), GENERIC_WRITE, FILE_SHARE_READ, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
+    const auto result = ::MiniDumpWriteDump(process_handle, process_sp->GetID(), file_handle, MiniDumpNormal, NULL, NULL, NULL);
+    ::CloseHandle(file_handle);
+    ::CloseHandle(process_handle);
+    if (!result)
+    {
+        error.SetError(::GetLastError(), lldb::eErrorTypeWin32);
+        return false;
+    }
+    return true;
+#endif
+    return false;
+}
+
+}  // namesapce lldb_private