[Lldb-commits] [PATCH] D60121: Object/Minidump: Add support for reading the ModuleList stream

[Pavel Labath via Phabricator via lldb-commits]

labath marked 2 inline comments as done.
labath added a comment.

In D60121#1454817 <https://reviews.llvm.org/D60121#1454817>, @jhenderson wrote:

> Code basically looks fine, but somebody else should confirm that the format is correct.


BTW, the structure definitions are just copied from lldb (https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/Process/minidump/MinidumpTypes.h and other files in that directory). I'm also porting the lldb parsing code to use the new llvm stuff as soon as the llvm stuff is ready as a way to ensure things remain interoperable with the existing minidumps and related tests.



================
Comment at: lib/Object/Minidump.cpp:55
+  if (!OptionalStream)
+    return createError("No such stream", object_error::invalid_section_index);
+  auto ExpectedSize =
----------------
jhenderson wrote:
> I wonder whether it would be worth a new class of errors for minidump files? After all, invalid_section_index feels a bit forced for a format without sections!
Yes, I've been wondering about that too. In practice, I don't expect that the consumers will wan't to differentiate the error cases too much here (it usually does not matter if the stream was not present in the file, or if it was truncated -- the end result is the same -- you cannot use it). However, for clarity it might be better to do have a separate type anyway. I'll whip up a separate patch for that.


================
Comment at: unittests/Object/MinidumpTest.cpp:317
+      1, 0, 0, 0,                           // NumberOfStreams,
+      0x20, 0, 0, 0,                        // StreamDirectoryRVA
+      0, 1, 2, 3, 4, 5, 6, 7,               // Checksum, TimeDateStamp
----------------
jhenderson wrote:
> It's a bit jarring seeing both hex and decimal numbers in here at the same time. Is there a particular reason you've used hex here, but decimal for e.g. 116 below?
Yeah, I agree this is not ideal. I started with hex here because it looked more "binary", but then I realized that:
a) decimal is shorter
b) the "size" fields are present in the static_asserts as decimal, and so it was easier to copy the numbers from there as decimal

I'll replace all of these to use decimal, except the "version" field, as that is defined in hex in the header.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60121/new/

https://reviews.llvm.org/D60121