[Lldb-commits] [PATCH] D25905: Minidump plugin: Adding ProcessMinidump, ThreadMinidump and register the plugin in SystemInitializerFull

[Pavel Labath via lldb-commits]

labath added a comment.

Just a bit more cleanup and this will be fine. Sorry about the back and forth, I was in a hurry yesterday.

Zachary, Adrian: any thoughts on your side?



================
Comment at: packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64_not_crashed.cpp:7
+int
+bar(int x)
+{
----------------
labath wrote:
> Please format these consistently (llvm-style). clang-format will not work by default as we are still ignoring the test folder.
Still not correct (4 char indent, inconsistent braces). It might be easiest to just temporarily remove `packages/Python/lldbsuite/.clang-format`. We'll need to figure out a better solution for formatting new test code in the long run though.


================
Comment at: packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/makefile.txt:1
+CC=g++
+FLAGS=-g --std=c++11
----------------
Please add a short blurb explaining how does this build work and why did we choose such a complicated way of generating the minidumps.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:67
+  // skip if the Minidump file is Windows generated, because we are still
+  // work-in-progress
+  if (!minidump_parser ||
----------------
Zach, Adrian: IIUC, the new plugin should generally have feature parity with the windows-only plugin. (Dimitar: could you say exactly what bits are missing?). You should be able to test out this plugin on windows minidumps by removing the windows check below.

After this goes in, we'll be looking to remove the windows-only plugin.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:89
+
+  // calling this here, and not in DoLoadCore, because we need to be sure this
+  // gets called first, because it initialises m_is_wow64 which is used in
----------------
Sorry about the back-and-forth, but I think we should put this back to LoadCore(). I did not realize that ReadModuleList was a private function and not a public interface of the class. :(

In this case we can assume that anyone will call LoadCore before expecting any other values to be valid, and we shouldn't be doing any parsing (especially not if it calls target->AddModule and such) as that is not the pattern anywhere else.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:218
+  arch_spec.SetTriple(triple);
+  return arch_spec;
+}
----------------
return ArchSpec(triple);


https://reviews.llvm.org/D25905