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

[Pavel Labath via lldb-commits]

labath added a comment.

> > Do you have a suggestion where the parsing code to be, if not in the same directory?

> 

> 

> The parsing code will end up being used by multiple plugins--the new one you're building for Linux and the existing one that's Windows-specific.


I was hoping that we could avoid having a separate plugin for each OS. ProcessElfCore already handles linux and freebsd core files. It should be even simpler for minidumps, as the format there should be more similar. If that does not play out then we will need to move the generic code to a separate place, but I am hoping we can keep it in one plugin (maybe we can have a small class inside the plugin which abstracts any os-specific details we encounter).

> What I thought would happen would be that you'd write the parsing code first, hook up the Windows-minidump plugin to use it (since the Windows-minidump plugin tests would help validate that your parsing is correct/complete) and then either add a new plugin for non-Windows.  That would argue for the minidump parsing to be in a common location that could be used by both plugins.  I don't have a good idea of where that would go.

> 

> Another approach is to make the Windows plugin platform agnostic once you replace the limited use of the minidump APIs with your own parsing code.

> 

> Food for thought.  No need to hold up this patch for that.  The code can be moved later if appropriate.


We'll see how it goes...


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:22
@@ +21,3 @@
+{
+    llvm::ArrayRef<uint8_t> header_data(m_data_sp->GetBytes(), sizeof(MinidumpHeader));
+    m_header = MinidumpHeader::Parse(header_data);
----------------
You should check whether the data buffer contains more than `sizeof(MinidumpHeader)` bytes. Or you can just use `m_data_sp->GetByteSize()` for size, and let the parse function deal with that.

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:31
@@ +30,3 @@
+    Error error;
+    llvm::ArrayRef<uint8_t> directory_data(m_data_sp->GetBytes() + directory_list_offset,
+                                           sizeof(MinidumpDirectory) * m_header.getValue()->streams_count);
----------------
Same thing. How do you know this memory exists? Is that checked somewhere?

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:60
@@ +59,3 @@
+{
+    llvm::DenseMap<uint32_t, MinidumpLocationDescriptor>::iterator iter = m_directory_map.find((uint32_t)stream_type);
+    if (iter == m_directory_map.end())
----------------
Suggestion: `auto iter = ` ?

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:260
@@ +259,3 @@
+
+    uint8_t number_of_processors;
+    uint8_t product_type;
----------------
dvlahovski wrote:
> amccarth wrote:
> > I'll concede that the static_assert is useful.  Given that, showing the arithmetic explicitly will be helpful when one of these assertions trips, because you'll be able to see how it's supposed to correspond to the struct.
> Yes that was my inital insentive to write explicitly the arithmetic
What I did not like about the original approach is that it declared a new global variable (in a header, so it is visible to everyone), just to store the temporary result. I think we should avoid that. I don't really care whether you write `sizeof(FOO) = 47` or `sizeof(FOO) = 4*10+4+3`


https://reviews.llvm.org/D23545