[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream
This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.
Closed by commit rG6e0b1ce48e3c: Object/minidump: Add support for the
MemoryInfoList stream (authored by labath).
Herald added a subscriber: hiraditya.
Changed prior to commit:
https://reviews.llvm.org/D68210?vs=223816=223850#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68210/new/
https://reviews.llvm.org/D68210
Files:
llvm/include/llvm/BinaryFormat/Minidump.h
llvm/include/llvm/BinaryFormat/MinidumpConstants.def
llvm/include/llvm/Object/Minidump.h
llvm/lib/Object/Minidump.cpp
llvm/unittests/Object/MinidumpTest.cpp
Index: llvm/unittests/Object/MinidumpTest.cpp
===
--- llvm/unittests/Object/MinidumpTest.cpp
+++ llvm/unittests/Object/MinidumpTest.cpp
@@ -511,3 +511,202 @@
EXPECT_EQ(0x00090807u, MD.Memory.RVA);
}
}
+
+TEST(MinidumpFile, getMemoryInfoList) {
+ std::vector OneEntry{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 64, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 16, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ };
+
+ // Same as before, but the list header is larger.
+ std::vector BiggerHeader{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 68, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 20, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ 0, 0, 0, 0, // ???
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ };
+
+ // Same as before, but the entry is larger.
+ std::vector BiggerEntry{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 68, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 16, 0, 0, 0, 52, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ 0, 0, 0, 0, // ???
+ };
+
+ for (ArrayRef Data : {OneEntry, BiggerHeader, BiggerEntry}) {
+auto ExpectedFile = create(Data);
+ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+const MinidumpFile = **ExpectedFile;
+auto ExpectedInfo = File.getMemoryInfoList();
+ASSERT_THAT_EXPECTED(ExpectedInfo, Succeeded());
+ASSERT_EQ(1u, std::distance(ExpectedInfo->begin(), ExpectedInfo->end()));
+const MemoryInfo = *ExpectedInfo.get().begin();
+EXPECT_EQ(0x0706050403020100u, Info.BaseAddress);
+EXPECT_EQ(0x0504030201000908u, Info.AllocationBase);
+EXPECT_EQ(MemoryProtection::Execute, Info.AllocationProtect);
+EXPECT_EQ(0x09080706u, Info.Reserved0);
+
[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream
jhenderson accepted this revision. jhenderson added a comment. This revision is now accepted and ready to land. Thanks, LGTM (modulo the Minidump-specific details). Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68210/new/ https://reviews.llvm.org/D68210 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream
labath marked 2 inline comments as done. labath added inline comments. Comment at: unittests/Object/MinidumpTest.cpp:620 + }; + EXPECT_THAT_EXPECTED(cantFail(create(HeaderTooBig))->getMemoryInfoList(), + Failed()); jhenderson wrote: > Here and in the similar places, I'm not convinced that `cantFail` is > appropriate (if the creation code is broken, this will assert and therefore > possibly hide the actual testing failures that show where it went wrong more > precisely). It should probably be a two phase thing: > > ``` > Expected> Minidump = HeaderTooBig); > ASSERT_THAT_EXPECTED(Minidump, Succeeded()); > EXPECTE_THAT_EXPECTED(Minidump->getMemoryInfoList(), Failed()); > ``` Done. The previous solution of just passing the creation error in the helper function was totally bogus, of course. Ideally, it would be possible to express this as a one-liner using gtest matchers (`HasValue(Property(, Failed))`, but unfortunately they are quite incompatible with the move-only Expected semantices. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68210/new/ https://reviews.llvm.org/D68210 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream
labath updated this revision to Diff 223816.
labath added a comment.
use two phase checks
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68210/new/
https://reviews.llvm.org/D68210
Files:
include/llvm/BinaryFormat/Minidump.h
include/llvm/BinaryFormat/MinidumpConstants.def
include/llvm/Object/Minidump.h
lib/Object/Minidump.cpp
unittests/Object/MinidumpTest.cpp
Index: unittests/Object/MinidumpTest.cpp
===
--- unittests/Object/MinidumpTest.cpp
+++ unittests/Object/MinidumpTest.cpp
@@ -511,3 +511,202 @@
EXPECT_EQ(0x00090807u, MD.Memory.RVA);
}
}
+
+TEST(MinidumpFile, getMemoryInfoList) {
+ std::vector OneEntry{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 64, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 16, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ };
+
+ // Same as before, but the list header is larger.
+ std::vector BiggerHeader{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 68, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 20, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ 0, 0, 0, 0, // ???
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ };
+
+ // Same as before, but the entry is larger.
+ std::vector BiggerEntry{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 68, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 16, 0, 0, 0, 52, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ 0, 0, 0, 0, // ???
+ };
+
+ for (ArrayRef Data : {OneEntry, BiggerHeader, BiggerEntry}) {
+auto ExpectedFile = create(Data);
+ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+const MinidumpFile = **ExpectedFile;
+auto ExpectedInfo = File.getMemoryInfoList();
+ASSERT_THAT_EXPECTED(ExpectedInfo, Succeeded());
+ASSERT_EQ(1u, std::distance(ExpectedInfo->begin(), ExpectedInfo->end()));
+const MemoryInfo = *ExpectedInfo.get().begin();
+EXPECT_EQ(0x0706050403020100u, Info.BaseAddress);
+EXPECT_EQ(0x0504030201000908u, Info.AllocationBase);
+EXPECT_EQ(MemoryProtection::Execute, Info.AllocationProtect);
+EXPECT_EQ(0x09080706u, Info.Reserved0);
+EXPECT_EQ(0x0706050403020100u, Info.RegionSize);
+EXPECT_EQ(MemoryState::Commit, Info.State);
+EXPECT_EQ(MemoryProtection::ExecuteRead, Info.Protect);
+EXPECT_EQ(MemoryType::Private, Info.Type);
+EXPECT_EQ(0x01000908u, Info.Reserved1);
+ }
+
+ // Header does not fit into the stream.
+ std::vector
[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream
labath added a comment. (ignore latest diff. I just realized it's totally bogus) Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68210/new/ https://reviews.llvm.org/D68210 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream
labath updated this revision to Diff 223811.
labath added a comment.
Address review comments
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68210/new/
https://reviews.llvm.org/D68210
Files:
include/llvm/BinaryFormat/Minidump.h
include/llvm/BinaryFormat/MinidumpConstants.def
include/llvm/Object/Minidump.h
lib/Object/Minidump.cpp
unittests/Object/MinidumpTest.cpp
Index: unittests/Object/MinidumpTest.cpp
===
--- unittests/Object/MinidumpTest.cpp
+++ unittests/Object/MinidumpTest.cpp
@@ -511,3 +511,204 @@
EXPECT_EQ(0x00090807u, MD.Memory.RVA);
}
}
+
+TEST(MinidumpFile, getMemoryInfoList) {
+ std::vector OneEntry{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 64, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 16, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ };
+
+ // Same as before, but the list header is larger.
+ std::vector BiggerHeader{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 68, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 20, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ 0, 0, 0, 0, // ???
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ };
+
+ // Same as before, but the entry is larger.
+ std::vector BiggerEntry{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 68, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 16, 0, 0, 0, 52, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ 0, 0, 0, 0, // ???
+ };
+
+ for (ArrayRef Data : {OneEntry, BiggerHeader, BiggerEntry}) {
+auto ExpectedFile = create(Data);
+ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+const MinidumpFile = **ExpectedFile;
+auto ExpectedInfo = File.getMemoryInfoList();
+ASSERT_THAT_EXPECTED(ExpectedInfo, Succeeded());
+ASSERT_EQ(1u, std::distance(ExpectedInfo->begin(), ExpectedInfo->end()));
+const MemoryInfo = *ExpectedInfo.get().begin();
+EXPECT_EQ(0x0706050403020100u, Info.BaseAddress);
+EXPECT_EQ(0x0504030201000908u, Info.AllocationBase);
+EXPECT_EQ(MemoryProtection::Execute, Info.AllocationProtect);
+EXPECT_EQ(0x09080706u, Info.Reserved0);
+EXPECT_EQ(0x0706050403020100u, Info.RegionSize);
+EXPECT_EQ(MemoryState::Commit, Info.State);
+EXPECT_EQ(MemoryProtection::ExecuteRead, Info.Protect);
+EXPECT_EQ(MemoryType::Private, Info.Type);
+EXPECT_EQ(0x01000908u, Info.Reserved1);
+ }
+
+ const auto = [](ArrayRef Data)
+ -> Expected> {
+
[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream
jhenderson added inline comments.
Comment at: lib/Object/Minidump.cpp:58
+MinidumpFile::getMemoryInfoList() const {
+ auto OptionalStream = getRawStream(StreamType::MemoryInfoList);
+ if (!OptionalStream)
labath wrote:
> jhenderson wrote:
> > 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.
> Done. I've also changed the other calls to getRawStream.
Thanks!
Comment at: unittests/Object/MinidumpTest.cpp:620
+ };
+ EXPECT_THAT_EXPECTED(cantFail(create(HeaderTooBig))->getMemoryInfoList(),
+ Failed());
Here and in the similar places, I'm not convinced that `cantFail` is
appropriate (if the creation code is broken, this will assert and therefore
possibly hide the actual testing failures that show where it went wrong more
precisely). It should probably be a two phase thing:
```
Expected> Minidump = HeaderTooBig);
ASSERT_THAT_EXPECTED(Minidump, Succeeded());
EXPECTE_THAT_EXPECTED(Minidump->getMemoryInfoList(), Failed());
```
Comment at: unittests/Object/MinidumpTest.cpp:624
+ // Header fits into the stream, but it is too small to contain the required
+ // entries).
+ std::vector HeaderTooSmall{
Nit: delete the ')'
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68210/new/
https://reviews.llvm.org/D68210
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream
labath added a comment.
Thanks for the review. I am fairly confident in the minidump details, as I
based this code on the existing functional implementation in lldb, which I have
also cross-referenced with the publicly available microsoft documentation.
@amccarth, @clayborg: do you want to have a look at the minidump details?
Comment at: lib/Object/Minidump.cpp:58
+MinidumpFile::getMemoryInfoList() const {
+ auto OptionalStream = getRawStream(StreamType::MemoryInfoList);
+ if (!OptionalStream)
jhenderson wrote:
> 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.
Done. I've also changed the other calls to getRawStream.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68210/new/
https://reviews.llvm.org/D68210
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream
labath updated this revision to Diff 223616.
labath marked 6 inline comments as done.
labath added a comment.
Address review comments
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68210/new/
https://reviews.llvm.org/D68210
Files:
include/llvm/BinaryFormat/Minidump.h
include/llvm/BinaryFormat/MinidumpConstants.def
include/llvm/Object/Minidump.h
lib/Object/Minidump.cpp
unittests/Object/MinidumpTest.cpp
Index: unittests/Object/MinidumpTest.cpp
===
--- unittests/Object/MinidumpTest.cpp
+++ unittests/Object/MinidumpTest.cpp
@@ -511,3 +511,199 @@
EXPECT_EQ(0x00090807u, MD.Memory.RVA);
}
}
+
+TEST(MinidumpFile, getMemoryInfoList) {
+ std::vector OneEntry{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 64, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 16, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ };
+
+ // Same as before, but the list header is larger.
+ std::vector BiggerHeader{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 68, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 20, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ 0, 0, 0, 0, // ???
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ };
+
+ // Same as before, but the entry is larger.
+ std::vector BiggerEntry{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 68, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 16, 0, 0, 0, 52, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ 0, 0, 0, 0, // ???
+ };
+
+ for (ArrayRef Data : {OneEntry, BiggerHeader, BiggerEntry}) {
+auto ExpectedFile = create(Data);
+ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+const MinidumpFile = **ExpectedFile;
+auto ExpectedInfo = File.getMemoryInfoList();
+ASSERT_THAT_EXPECTED(ExpectedInfo, Succeeded());
+ASSERT_EQ(1u, std::distance(ExpectedInfo->begin(), ExpectedInfo->end()));
+const MemoryInfo = *ExpectedInfo.get().begin();
+EXPECT_EQ(0x0706050403020100u, Info.BaseAddress);
+EXPECT_EQ(0x0504030201000908u, Info.AllocationBase);
+EXPECT_EQ(MemoryProtection::Execute, Info.AllocationProtect);
+EXPECT_EQ(0x09080706u, Info.Reserved0);
+EXPECT_EQ(0x0706050403020100u, Info.RegionSize);
+EXPECT_EQ(MemoryState::Commit, Info.State);
+EXPECT_EQ(MemoryProtection::ExecuteRead, Info.Protect);
+EXPECT_EQ(MemoryType::Private, Info.Type);
+EXPECT_EQ(0x01000908u, Info.Reserved1);
+ }
+
+ // Header does not
[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream
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 = */ 0xu),
+};
I believe if you format this line as:
```
LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/0xu),
```
clang-format will leave it unedited. I believe it has special rules for
`/*=*/` 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 = 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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D68210: Object/minidump: Add support for the MemoryInfoList stream
labath created this revision.
labath added reviewers: amccarth, jhenderson, clayborg.
Herald added a project: LLVM.
This patch adds the definitions of the constants and structures
necessary to interpret the MemoryInfoList minidump stream, as well as
the object::MinidumpFile interface to access the stream.
While the code is fairly simple, there is one important deviation from
the other minidump streams, which is worth calling out explicitly.
Unlike other "List" streams, the size of the records inside
MemoryInfoList stream is not known statically. Instead it is described
in the stream header. This makes it impossible to return
ArrayRef from the accessor method, as it is done with other
streams. Instead, I create an iterator class, which can be parameterized
by the runtime size of the structure, and return
iterator_range instead.
Repository:
rL LLVM
https://reviews.llvm.org/D68210
Files:
include/llvm/BinaryFormat/Minidump.h
include/llvm/BinaryFormat/MinidumpConstants.def
include/llvm/Object/Minidump.h
lib/Object/Minidump.cpp
unittests/Object/MinidumpTest.cpp
Index: unittests/Object/MinidumpTest.cpp
===
--- unittests/Object/MinidumpTest.cpp
+++ unittests/Object/MinidumpTest.cpp
@@ -511,3 +511,180 @@
EXPECT_EQ(0x00090807u, MD.Memory.RVA);
}
}
+
+TEST(MinidumpFile, getMemoryInfoList) {
+ std::vector OneEntry{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 64, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 16, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ };
+
+ // Same as before, but the list header is larger.
+ std::vector BiggerHeader{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 68, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 20, 0, 0, 0, 48, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ 0, 0, 0, 0, // ???
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ };
+
+ // Same as before, but the entry is larger.
+ std::vector BiggerEntry{
+ // Header
+ 'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+ 1, 0, 0, 0, // NumberOfStreams,
+ 32, 0, 0, 0, // StreamDirectoryRVA
+ 0, 1, 2, 3, 4, 5, 6, 7, // Checksum, TimeDateStamp
+ 0, 0, 0, 0, 0, 0, 0, 0, // Flags
+// Stream Directory
+ 16, 0, 0, 0, 68, 0, 0, 0, // Type, DataSize,
+ 44, 0, 0, 0, // RVA
+ // MemoryInfoListHeader
+ 16, 0, 0, 0, 52, 0, 0, 0, // SizeOfHeader, SizeOfEntry
+ 1, 0, 0, 0, 0, 0, 0, 0, // NumberOfEntries
+ // MemoryInfo
+ 0, 1, 2, 3, 4, 5, 6, 7, // BaseAddress
+ 8, 9, 0, 1, 2, 3, 4, 5, // AllocationBase
+ 16, 0, 0, 0, 6, 7, 8, 9, // AllocationProtect, Reserved0
+ 0, 1, 2, 3, 4, 5, 6, 7, // RegionSize
+ 0, 16, 0, 0, 32, 0, 0, 0, // State, Protect
+ 0, 0, 2, 0, 8, 9, 0, 1, // Type, Reserved1
+ 0, 0, 0, 0, // ???
+ };
+
+ for (ArrayRef Data : {OneEntry, BiggerHeader, BiggerEntry}) {
+auto ExpectedFile = create(Data);
+ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+const MinidumpFile = **ExpectedFile;
+auto ExpectedInfo = File.getMemoryInfoList();
+ASSERT_THAT_EXPECTED(ExpectedInfo, Succeeded());
+
