[Lldb-commits] [PATCH] D14591: Implement register context for mini dump debugging

[Zachary Turner via lldb-commits]

zturner added inline comments.

================
Comment at: packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:40
@@ +39,3 @@
+        thread = self.process.GetThreadAtIndex(0)
+        # The crash is in main, so there should be one frame on the stack.
+        self.assertEqual(thread.GetNumFrames(), 1)
----------------
I remember us being able to get call stacks higher than main.  But now that I think about it I guess that's only true for live debugging since you have a physical copy of the module loaded in your process, and you can read it's EAT.  In any case, this assumption probably won't be true anymore once we understand PDBs and symbol servers.  Although at that point hopefully we'll have many more tests covering different scenarios.

================
Comment at: packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py:45
@@ +44,3 @@
+        pc = frame.GetPC()
+        eip = frame.FindRegister("eip")
+        self.assertTrue(eip.IsValid())
----------------
Does this work if you change `eip` to `pc`?  If so that would allow this test to work in the presence of 64 bit.

================
Comment at: source/Plugins/Process/Windows/MiniDump/ProcessWinMiniDump.cpp:193
@@ +192,3 @@
+            const auto &mini_dump_thread = thread_list_ptr->Threads[i];
+            std::shared_ptr<ThreadWinMiniDump> thread_sp(new ThreadWinMiniDump(*this, mini_dump_thread.ThreadId));
+            if (mini_dump_thread.ThreadContext.DataSize >= sizeof(CONTEXT))
----------------
Can you change this to

    auto thread_sp = llvm::make_shared<ThreadWinMiniDump>(*this, mini_dump_thread.ThreadId);

================
Comment at: source/Plugins/Process/Windows/MiniDump/ThreadWinMiniDump.h:42-44
@@ -44,4 +41,5 @@
 protected:
-    std::string m_thread_name;
     lldb::RegisterContextSP m_reg_context_sp;
+    class Data;
+    std::unique_ptr<Data> m_data;  // for WinAPI-specific data
 
----------------
Why does this class need a separate `CONTEXT` than the one that is already stored in `m_reg_context_sp`?  It seems like now we're storing the `CONTEXT` twice.

================
Comment at: source/Plugins/Process/Windows/MiniDump/x64/RegisterContextWindowsMiniDump_x64.h:14
@@ +13,3 @@
+#include "lldb/lldb-forward.h"
+#include "../../Common/x64/RegisterContextWindows_x64.h"
+
----------------
Instead of using relative paths, I think you can write this as

    #include "Plugins/Process/Windows/Common/x64/RegisterContextWindows_x64.h"


http://reviews.llvm.org/D14591