zturner added inline comments.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:29
@@ +28,3 @@
+
+ lldb::ByteOrder byteorder = m_data.GetByteOrder();
+ lldb::offset_t directory_list_offset = m_header->stream_directory_rva;
----------------
I asked some people familiar with breakpad and windows history, and the answer
I got was that while minidump may have support big endian prior to windows 2000
when it supported PPC and MIPS, but probably not anymore.
At this point we have full control over the minidump pipeline. If we want to
generate them from within LLDB, we can write them using little endian. And if
we want to read them, we can assume they are not from a file prior to windows
2000. I think it's safe to assume little endian unless we have strong evidence
that we need to parse a big endian minidump.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:35
@@ +34,3 @@
+ {
+ m_data.ExtractBytes(directory_list_offset, sizeof(MinidumpDirectory),
byteorder, &directory);
+ directory_list_offset += sizeof(MinidumpDirectory);
----------------
Note that this will make a copy of the data. Based on my previous email, this
could be written as follows:
const MiniDumpDirectory *directory = nullptr;
```
for (uint32_t i=0; i < m_header->streams_count; ++i) {
if (auto EC = consumeObject(data, directory))
return EC;
m_directory_map[directory.stream_type = directory->location;
}
```
No copying is involved here. You would need to make a few changes to do this
but I think it is worth it. Same concept applies throughout the rest of this
file, so I won't mention it again.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:13-25
@@ +12,15 @@
+
+// C includes
+
+// C++ includes
+#include <cstring>
+#include <unordered_map>
+
+// Other libraries and framework includes
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/DataExtractor.h"
+#include "llvm/ADT/Optional.h"
+
+// Project includes
+#include "MinidumpTypes.h"
+
----------------
Please include files in the following order:
1. Files in the same directory
2. LLDB includes
3. LLVM includes
4. C and C++ system includes
Although this isn't consistent with the rest of the codebase, it was recently
agreed that this is what we want to move towards, so all new code should follow
this pattern.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:61
@@ +60,3 @@
+ llvm::Optional<MinidumpHeader> m_header;
+ std::unordered_map<int, MinidumpLocationDescriptor> m_directory_map;
+};
----------------
Please use `llvm::DenseMap`. `std::unordered_map` should probably not be used
for anything ever.
================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:67
@@ +66,1 @@
+#endif // liblldb_MinidumpParser_h_
\ No newline at end of file
----------------
Newline here.
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:40
@@ +39,3 @@
+
+enum MinidumpStreamType
+{
----------------
amccarth wrote:
> // Reference:
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms680394.aspx
In LLVM where we parse PDB and CodeView debug info types, we have adopted the
convention that these are all `enum classes`, and we continue to use LLVM
naming style instead of using Microsoft all caps style. So I would write this
as:
```
enum class MiniDumpStreamType : uint32_t {
Unused = 0,
Reserved0 = 1,
Reserved1 = 2,
ThreadList = 3,
...
};
```
The `: uint32_t` is important since enums are signed by default on Microsoft
compilers.
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:85
@@ +84,3 @@
+{
+ MINIDUMP_CPU_ARCHITECTURE_X86 = 0, /* PROCESSOR_ARCHITECTURE_INTEL
*/
+ MINIDUMP_CPU_ARCHITECTURE_MIPS = 1, /* PROCESSOR_ARCHITECTURE_MIPS
*/
----------------
Same goes for these, as well as a few more enums later on. I would make these
all enum classes and name them using the LLVM naming conventions.
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:147
@@ +146,3 @@
+ MINIDUMP_CPU_ARM_ELF_HWCAP_IDIVT = (1 << 18),
+};
+
----------------
Enum classes don't play nicely with flags, but LLVM has a file called
`llvm/ADT/BitmaskEnum.h` which improves this significantly. So these can also
be an enum class, just use the proper macro from that file so that bitwise
operations become possible.
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:151
@@ +150,3 @@
+{
+ uint32_t signature;
+ uint32_t version;
----------------
As mentioned in my previous response, all these could be come LLVM endian aware
types from `llvm/Support/Endian.h' which would allow you to `reinterpret_cast`
straight from the underlying buffer.
================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:165-166
@@ +164,4 @@
+};
+const int MINIDUMP_HEADER_SIZE = 3 * 4 + sizeof(RVA) + 2 * 4 + 8;
+static_assert(sizeof(MinidumpHeader) == MINIDUMP_HEADER_SIZE, "sizeof
MinidumpHeader is not correct!");
+
----------------
The computation here seems unnecessary to me. How about just
`static_assert(sizeof(MinidumpHeader) == 32)`;
https://reviews.llvm.org/D23545
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
