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

Pavel Labath via lldb-commits lldb-commits at lists.llvm.org
Tue Aug 16 04:18:44 PDT 2016

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 @@
+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
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?


More information about the lldb-commits mailing list