[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream

[James Henderson via Phabricator via lldb-commits]

jhenderson added a comment.

Can't comment too much on the file format details, but I've made some more general comments.

FYI, I'll be away from end of day Wednesday for 2 and a half weeks, so won't be able to further review after that point until I'm back.



================
Comment at: include/llvm/BinaryFormat/Minidump.h:81
+#include "llvm/BinaryFormat/MinidumpConstants.def"
+  LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue = */ 0xffffffffu),
+};
----------------
I believe if you format this line as:


```
LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/0xffffffffu),
```

clang-format will leave it unedited. I believe it has special rules for `/*<Name>=*/<value>` to label parameters.


================
Comment at: lib/Object/Minidump.cpp:58
+MinidumpFile::getMemoryInfoList() const {
+  auto OptionalStream = getRawStream(StreamType::MemoryInfoList);
+  if (!OptionalStream)
----------------
I probably should have picked up on this in previous reviews, but this is too much `auto` for my liking, as it's not obvious from the call site what `getRawStream` returns.


================
Comment at: lib/Object/Minidump.cpp:66
+  const minidump::MemoryInfoListHeader &H = ExpectedHeader.get()[0];
+  auto ExpectedData = getDataSlice(*OptionalStream, H.SizeOfHeader,
+                                   H.SizeOfEntry * H.NumberOfEntries);
----------------
Ditto.


================
Comment at: unittests/Object/MinidumpTest.cpp:617-618
+      // MemoryInfoListHeader
+      16, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+      1, 0, 0, 0,               // ???
+  };
----------------
I might make the data here be of size 15 to test the edge case.

It's probably also worth a test case where the header size as specified by SizeOfHeader fits in the data but is smaller than the expected value.


================
Comment at: unittests/Object/MinidumpTest.cpp:634
+      // MemoryInfoListHeader
+      16, 0, 0, 0, 52, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+      1, 0, 0, 0, 0, 0, 0, 0,   // NumberOfEntries
----------------
I might go for a value of 49 to test the edge value here.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68210