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

[Pavel Labath via lldb-commits]

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
+int
+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(),
----------------
llvm::StringRef(*name).endswith_lower("wow64.dll")


================
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)"` ?


https://reviews.llvm.org/D25905