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

Pavel Labath via lldb-commits lldb-commits at lists.llvm.org
Mon Oct 24 10:03:18 PDT 2016

labath added a comment.

first round of comments. I will give this another look tomorrow.

Comment at: packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/linux-x86_64_not_crashed.cpp:7
+bar(int x)
Please format these consistently (llvm-style). clang-format will not work by default as we are still ignoring the test folder.

Comment at: source/Plugins/Process/minidump/NtStructures.h:18
+// only about the position of the tls_slots.
+struct TEB64 {
+  llvm::support::ulittle64_t reserved1[12];
Please put this inside the lldb_private::minidump namespace.

Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:73
+  return lldb::ProcessSP(new ProcessMinidump(
+      target_sp, listener_sp, *crash_file, minidump_parser.getValue()));
return std::make_shared<ProcesMinidump>(...

Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:274
+    // 64 bit windows
+    std::string name_tolower = name.getValue();
+    std::transform(name_tolower.begin(), name_tolower.end(),

Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:278
+    if (name_tolower.find("wow64.dll") != std::string::npos) {
+      m_is_wow64 = true;
+    }
There is a hidden assumption here that ReadModuleList() will be called before any other function that depends on the value of `m_is_wow64`. (I am not even sure if this is true in case of GetArchitecture().) I think we should make this more robust. Maybe you could initialize this (along with the filtered_modules list, I guess) in the constructor? It's not really doing extra work, as you're going to need to parse the module list at some very early point anyway.

Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:291
+  std::string module_names[16] = {
+      "D:"
+      "\\src\\llvm\\llvm\\tools\\lldb\\packages\\Python\\lldbsuite\\test\\funct"
Did clang-format break this in such an ugly way? What will happen if you use raw strings instead: `R"(D:\no\double\back\slash)"` ?


More information about the lldb-commits mailing list