[Lldb-commits] [PATCH] D14793: Enable saving of mini dumps with lldb process save-core.

[Zachary Turner via lldb-commits]

zturner added a reviewer: clayborg.
zturner added a comment.

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


================
Comment at: include/lldb/API/SBProcess.h:345-346
@@ +344,4 @@
+    // Save the state of the process in a core file (or mini dump on Windows).
+    lldb::SBError
+    SaveCore(const char *file_name);
+
----------------
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.



================
Comment at: packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:20
@@ +19,3 @@
+    @not_remote_testsuite_ready
+    @skipUnlessWindows
+    def test_windows_mini_dump (self):
----------------
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 :)

================
Comment at: packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:32
@@ +31,3 @@
+        self.assertTrue(process.Kill().Success())
+        # Clean up the mini dump file.
+        os.unlink(core)
----------------
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.

================
Comment at: packages/Python/lldbsuite/test/functionalities/process_save_core/TestProcessSaveCore.py:33
@@ +32,3 @@
+        # Clean up the mini dump file.
+        os.unlink(core)
+
----------------
This isn't exception safe right now.  For example, it won't unlink the file if one the asserts fails.

================
Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFFMiniDump.cpp:17
@@ +16,3 @@
+
+#ifdef _WIN32
+#include "lldb/Host/windows/windows.h"
----------------
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.


http://reviews.llvm.org/D14793