[Lldb-commits] [PATCH] D11611: Create a Windows mini-dump target

[Zachary Turner]

zturner added inline comments.

================
Comment at: cmake/LLDBDependencies.cmake:69
@@ -68,2 +68,3 @@
   lldbPluginProcessElfCore
+  lldbPluginProcessWinMinidump
   lldbPluginJITLoaderGDB
----------------
This needs to be removed.  The reason you see ElfCore and other MacOSX ones is because they don't use any Posix-specific APIs, but instead just read/write the core file directly and parse it.  This allows you to debug an ELF core on Windows, for example.  We use Windows Minidump API, so we can't allow someone not on Windows to try and link it, which this will cause to happen.  If someone ever decides to write a minidump plugin that works on non-Windows, we could add that plugin here, and leave the existing one under the windows-only check

================
Comment at: source/API/SystemInitializerFull.cpp:41
@@ -40,2 +40,3 @@
 #include "Plugins/Process/gdb-remote/ProcessGDBRemote.h"
+#include "Plugins/Process/win-minidump/ProcessWinMiniDump.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARF.h"
----------------
All the changes to this file need to be inside of #if defined(_MSC_VER) checks

================
Comment at: source/Plugins/Process/win-minidump/CMakeLists.txt:3
@@ +2,3 @@
+
+set(LLVM_NO_RTTI 1)
+
----------------
Some CMake files still have this line, but I think it's unnecessary.  Feel free to remove

================
Comment at: source/Plugins/Process/win-minidump/Makefile:1
@@ +1,2 @@
+##===- source/Plugins/Process/elf-core/Makefile -----------*- Makefile -*-===##
+#
----------------
I think you can delete this file, nobody on Windows is using the autoconf build, and I don't care to support it since the autoconf build is going away anyway.

================
Comment at: source/Plugins/Process/win-minidump/ProcessWinMiniDump.cpp:237-238
@@ +236,4 @@
+{
+    // TODO
+    return ArchSpec();
+}
----------------
We already have a `DetermineArchitecture()` function.  Is there any reason we can't just return it right here?  Does this function have slightly different semantics or something?

================
Comment at: source/Plugins/Process/win-minidump/ProcessWinMiniDump.h:49
@@ +48,3 @@
+
+    bool CanDebug(lldb_private::Target &target, bool plugin_specified_by_name) override;
+
----------------
Need to reformat these declarations to put return value on separate line.

================
Comment at: source/Plugins/Process/win-minidump/ProcessWinMiniDump.h:86
@@ +85,3 @@
+
+    lldb_private::FileSpec m_core_file;  // TODO:  IS THIS NEEDED?  SHOULD IT BE IN struct Data?
+
----------------
I'd rather remove the TODO and either put it in Data or leave it here.  Either way, be decisive :)


http://reviews.llvm.org/D11611