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

[Adrian McCarthy via lldb-commits]

amccarth added a comment.

In https://reviews.llvm.org/D23545#519675, @dvlahovski wrote:

> In https://reviews.llvm.org/D23545#516808, @amccarth wrote:
>
> > Are we putting this code in the right place?  I wouldn't expect minidump parsing to fall under Plugins/Process.
> >
> > I assume the eventual intent is to turn the Windows-specific code into a user of your new code?  I look forward to seeing that happen.
>
>
> My idea was that the new plugin will live in Plugins/Process/minidump.
>  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.

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.


================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:22
@@ +21,3 @@
+llvm::Optional<const MinidumpHeader *>
+MinidumpHeader::Parse(llvm::ArrayRef<uint8_t> &data)
+{
----------------
I find it amusing that all these `Parse` methods are in MinidumpTypes.cpp rather than MinidumpParser.cpp.  Just an observation--not a request to change it.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:133
@@ +132,3 @@
+// This matches the Linux kernel definitions from <asm/hwcaps.h>
+enum MinidumpPCPUInformationARMElfHwCaps
+{
----------------
zturner wrote:
> Should this be `enum class`?
These look like individual bits that will be bitwise-ORed together, which is trickier to do with an enum class.

But you may still want to specify the underlying type, like `uint32_t`.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:258
@@ +257,3 @@
+    llvm::support::ulittle16_t processor_level;
+    llvm::support::ulittle16_t processor_revision;
+
----------------
dvlahovski wrote:
> The idea of this is to be sure that the compiler didn't align stuff wrongly/not in the way we expect.
OK.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:260
@@ +259,3 @@
+
+    uint8_t number_of_processors;
+    uint8_t product_type;
----------------
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.


https://reviews.llvm.org/D23545