[Lldb-commits] [PATCH] D23545: Minidump parsing

[Pavel Labath via lldb-commits]

labath added a comment.

The implementation looks nice and clean. I have only some stylistic comments giving it finishing touches..


================
Comment at: source/Plugins/Process/minidump/CMakeLists.txt:6
@@ +5,3 @@
+  MinidumpParser.cpp
+  #ProcessMinidump.cpp
+  #ThreadMinidump.cpp
----------------
Just remove the commented lines.

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:28
@@ +27,3 @@
+void
+MinidumpParser::SetData(const DataExtractor &data)
+{
----------------
What's the anticipated usage of this function? I am wondering whether it shouldn't clear the cached data (m_header, m_directory_map) as well. Or maybe, since you already have a matching contructor, just remove the function and have the user construct a new object if he needs to (?)

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:33
@@ +32,3 @@
+
+// this method should be called before doing anything else with the parser
+bool
----------------
This kind of comment should go into the header, as it speaks about the interface.

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:14
@@ +13,3 @@
+// C includes
+#include <cstring>
+
----------------
(nit) if you include <cstring>, it's not a `C` include :)
I'd put that under `C++`

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:45
@@ +44,3 @@
+
+    llvm::Optional<DirectoryMapConstIter>
+    GetStream(MinidumpStreamType stream_type);
----------------
Does the user need to know that you are returning an iterator? The map sounds like an implementation detail. If you don't have any usages for that in mind, it would be just cleaner to return the `MinidumpLocationDescriptor` (or maybe a reference to it) directly. Then you could remove the typedef above as well...

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:83
@@ +82,3 @@
+        thread = MinidumpThread::Parse(data, offset);
+        if (thread)
+            thread_list.push_back(MinidumpThread(thread.getValue()));
----------------
Should we abort upon encountering the first invalid thread?

I am thinking of a situation where a corrupt file claims to have UINT32_MAX threads, and we end up spinning here for quite a long time...

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:35
@@ +34,3 @@
+
+#define MINIDUMP_SIGNATURE 0x504d444d // 'PMDM'
+#define MINIDUMP_VERSION 0x0000a793   // 42899
----------------
Let's change these into (anonymous) enums as well. We should avoid macros when possible.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:120
@@ +119,3 @@
+    MINIDUMP_OS_NACL = 0x8205      /* Native Client (NaCl) */
+} MINIDUMPOSPlatform;
+
----------------
This declares a variable. I assume you did not want that.

================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:10
@@ +9,3 @@
+
+#if defined(_MSC_VER) && (_HAS_EXCEPTIONS == 0)
+// Workaround for MSVC standard library bug, which fails to include <thread> when
----------------
I think this is only necessary when you are actually using threads.

================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:51
@@ +50,3 @@
+        lldb::DataBufferSP data_sp(minidump_file.MemoryMapFileContents());
+        DataExtractor data(data_sp, lldb::eByteOrderLittle, 4);
+        ASSERT_GT(data.GetByteSize(), 0UL);
----------------
Since it's the parser who handles setting the byte order and such, maybe we should just pass the data buffer to it, and let it construct the data extractor internally. Would that work?


https://reviews.llvm.org/D23545