[Minidump] Support multiple exceptions in a minidump #107319
Conversation
|
@llvm/pr-subscribers-lldb @llvm/pr-subscribers-llvm-binary-utilities Author: Jacob Lalonde (Jlalond) ChangesA fork of #97470, splitting off the LLVM changes from the LLDB specific changes. This patch enables a minidump file to have multiple exceptions, exposed via an iterator of Expected streams. Full diff: https://github.com/llvm/llvm-project/pull/107319.diff 5 Files Affected:
diff --git a/llvm/include/llvm/Object/Minidump.h b/llvm/include/llvm/Object/Minidump.h
index 65ad6b171c2dc1..cf3d88889e900e 100644
--- a/llvm/include/llvm/Object/Minidump.h
+++ b/llvm/include/llvm/Object/Minidump.h
@@ -83,13 +83,16 @@ class MinidumpFile : public Binary {
return getListStream<minidump::Thread>(minidump::StreamType::ThreadList);
}
- /// Returns the contents of the Exception stream. An error is returned if the
- /// file does not contain this stream, or the stream is smaller than the size
- /// of the ExceptionStream structure. The internal consistency of the stream
- /// is not checked in any way.
- Expected<const minidump::ExceptionStream &> getExceptionStream() const {
- return getStream<minidump::ExceptionStream>(
- minidump::StreamType::Exception);
+ /// Returns the contents of the Exception stream. An error is returned if the
+ /// associated stream is smaller than the size of the ExceptionStream
+ /// structure. Or the directory supplied is not of kind exception stream.
+ Expected<const minidump::ExceptionStream &>
+ getExceptionStream(minidump::Directory Directory) const {
+ if (Directory.Type != minidump::StreamType::Exception) {
+ return createError("Not an exception stream");
+ }
+
+ return getStreamFromDirectory<minidump::ExceptionStream>(Directory);
}
/// Returns the list of descriptors embedded in the MemoryList stream. The
@@ -216,8 +219,71 @@ class MinidumpFile : public Binary {
bool IsEnd;
};
+class ExceptionStreamsIterator {
+ public:
+
+ static ExceptionStreamsIterator begin(ArrayRef<minidump::Directory> Streams, const MinidumpFile *File) {
+ return ExceptionStreamsIterator(Streams, File);
+ }
+
+ static ExceptionStreamsIterator end() {
+ return ExceptionStreamsIterator();
+ }
+
+ bool operator==(const ExceptionStreamsIterator &R) const {
+ return Streams.empty() && R.Streams.empty();
+ }
+
+ bool operator!=(const ExceptionStreamsIterator &R) const { return !(*this == R); }
+
+ Expected<const minidump::ExceptionStream &>
+ operator*() {
+ return ReadCurrent();
+ }
+
+ Expected<const minidump::ExceptionStream &>
+ operator->() {
+ return ReadCurrent();
+ }
+
+ ExceptionStreamsIterator &
+ operator++ () {
+ if (!Streams.empty())
+ Streams = Streams.drop_front();
+
+
+ return *this;
+ }
+
+ private:
+ ExceptionStreamsIterator(ArrayRef<minidump::Directory> Streams, const MinidumpFile *File)
+ : Streams(Streams), File(File) {}
+
+ ExceptionStreamsIterator() : Streams(ArrayRef<minidump::Directory>()), File(nullptr) {}
+
+ ArrayRef<minidump::Directory> Streams;
+ const MinidumpFile *File;
+
+ Expected<const minidump::ExceptionStream&> ReadCurrent() {
+ assert(!Streams.empty());
+ Expected<const minidump::ExceptionStream &> ExceptionStream =
+ File->getExceptionStream(Streams.front());
+ if (!ExceptionStream)
+ return ExceptionStream.takeError();
+
+ return ExceptionStream;
+ }
+ };
+
using FallibleMemory64Iterator = llvm::fallible_iterator<Memory64Iterator>;
+ /// Returns an iterator that reads each exception stream independently. The
+ /// contents of the exception strema are not validated before being read, an
+ /// error will be returned if the stream is not large enough to contain an
+ /// exception stream, or if the stream points beyond the end of the file.
+ iterator_range<ExceptionStreamsIterator>
+ getExceptionStreams() const;
+
/// Returns an iterator that pairs each descriptor with it's respective
/// content from the Memory64List stream. An error is returned if the file
/// does not contain a Memory64List stream, or if the descriptor data is
@@ -256,14 +322,21 @@ class MinidumpFile : public Binary {
MinidumpFile(MemoryBufferRef Source, const minidump::Header &Header,
ArrayRef<minidump::Directory> Streams,
- DenseMap<minidump::StreamType, std::size_t> StreamMap)
+ DenseMap<minidump::StreamType, std::size_t> StreamMap,
+ std::vector<minidump::Directory> ExceptionStreams)
: Binary(ID_Minidump, Source), Header(Header), Streams(Streams),
- StreamMap(std::move(StreamMap)) {}
+ StreamMap(std::move(StreamMap)), ExceptionStreams(std::move(ExceptionStreams)) {}
ArrayRef<uint8_t> getData() const {
return arrayRefFromStringRef(Data.getBuffer());
}
+ /// Return the stream of the given type, cast to the appropriate type. Checks
+ /// that the stream is large enough to hold an object of this type.
+ template <typename T>
+ Expected<const T &>
+ getStreamFromDirectory(minidump::Directory Directory) const;
+
/// Return the stream of the given type, cast to the appropriate type. Checks
/// that the stream is large enough to hold an object of this type.
template <typename T>
@@ -277,8 +350,18 @@ class MinidumpFile : public Binary {
const minidump::Header &Header;
ArrayRef<minidump::Directory> Streams;
DenseMap<minidump::StreamType, std::size_t> StreamMap;
+ std::vector<minidump::Directory> ExceptionStreams;
};
+template <typename T>
+Expected<const T &>
+MinidumpFile::getStreamFromDirectory(minidump::Directory Directory) const {
+ ArrayRef<uint8_t> Stream = getRawStream(Directory);
+ if (Stream.size() >= sizeof(T))
+ return *reinterpret_cast<const T *>(Stream.data());
+ return createEOFError();
+}
+
template <typename T>
Expected<const T &> MinidumpFile::getStream(minidump::StreamType Type) const {
if (std::optional<ArrayRef<uint8_t>> Stream = getRawStream(Type)) {
diff --git a/llvm/lib/Object/Minidump.cpp b/llvm/lib/Object/Minidump.cpp
index 93b2e2cecd1053..f334cd2a9a2e71 100644
--- a/llvm/lib/Object/Minidump.cpp
+++ b/llvm/lib/Object/Minidump.cpp
@@ -53,6 +53,12 @@ Expected<std::string> MinidumpFile::getString(size_t Offset) const {
return Result;
}
+iterator_range<llvm::object::MinidumpFile::ExceptionStreamsIterator>
+MinidumpFile::getExceptionStreams() const {
+ return make_range(ExceptionStreamsIterator::begin(ExceptionStreams, this),
+ ExceptionStreamsIterator::end());
+}
+
Expected<iterator_range<MinidumpFile::MemoryInfoIterator>>
MinidumpFile::getMemoryInfoList() const {
std::optional<ArrayRef<uint8_t>> Stream =
@@ -128,6 +134,7 @@ MinidumpFile::create(MemoryBufferRef Source) {
return ExpectedStreams.takeError();
DenseMap<StreamType, std::size_t> StreamMap;
+ std::vector<Directory> ExceptionStreams;
for (const auto &StreamDescriptor : llvm::enumerate(*ExpectedStreams)) {
StreamType Type = StreamDescriptor.value().Type;
const LocationDescriptor &Loc = StreamDescriptor.value().Location;
@@ -143,6 +150,13 @@ MinidumpFile::create(MemoryBufferRef Source) {
continue;
}
+ // Exceptions can be treated as a special case of streams. Other streams
+ // represent a list of entities, but exceptions are unique per stream.
+ if (Type == StreamType::Exception) {
+ ExceptionStreams.push_back(StreamDescriptor.value());
+ continue;
+ }
+
if (Type == DenseMapInfo<StreamType>::getEmptyKey() ||
Type == DenseMapInfo<StreamType>::getTombstoneKey())
return createError("Cannot handle one of the minidump streams");
@@ -153,7 +167,7 @@ MinidumpFile::create(MemoryBufferRef Source) {
}
return std::unique_ptr<MinidumpFile>(
- new MinidumpFile(Source, Hdr, *ExpectedStreams, std::move(StreamMap)));
+ new MinidumpFile(Source, Hdr, *ExpectedStreams, std::move(StreamMap), ExceptionStreams));
}
iterator_range<MinidumpFile::FallibleMemory64Iterator>
diff --git a/llvm/lib/ObjectYAML/MinidumpYAML.cpp b/llvm/lib/ObjectYAML/MinidumpYAML.cpp
index 10b8676b5c4cfb..1818823180b7d4 100644
--- a/llvm/lib/ObjectYAML/MinidumpYAML.cpp
+++ b/llvm/lib/ObjectYAML/MinidumpYAML.cpp
@@ -499,7 +499,7 @@ Stream::create(const Directory &StreamDesc, const object::MinidumpFile &File) {
switch (Kind) {
case StreamKind::Exception: {
Expected<const minidump::ExceptionStream &> ExpectedExceptionStream =
- File.getExceptionStream();
+ File.getExceptionStream(StreamDesc);
if (!ExpectedExceptionStream)
return ExpectedExceptionStream.takeError();
Expected<ArrayRef<uint8_t>> ExpectedThreadContext =
diff --git a/llvm/unittests/Object/MinidumpTest.cpp b/llvm/unittests/Object/MinidumpTest.cpp
index d2d9f115bb94a9..6dcdccc4dea81e 100644
--- a/llvm/unittests/Object/MinidumpTest.cpp
+++ b/llvm/unittests/Object/MinidumpTest.cpp
@@ -751,8 +751,10 @@ TEST(MinidumpFile, getExceptionStream) {
auto ExpectedFile = create(Data);
ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
const MinidumpFile &File = **ExpectedFile;
- Expected<const minidump::ExceptionStream &> ExpectedStream =
- File.getExceptionStream();
+ auto ExceptionIterator =
+ File.getExceptionStreams().begin();
+
+ Expected<const ExceptionStream &> ExpectedStream = *ExceptionIterator;
ASSERT_THAT_EXPECTED(ExpectedStream, Succeeded());
EXPECT_EQ(0x04030201u, ExpectedStream->ThreadId);
const minidump::Exception &Exception = ExpectedStream->ExceptionRecord;
diff --git a/llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp b/llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp
index a8b8da925d21d9..6b2963902a24b8 100644
--- a/llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp
+++ b/llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp
@@ -162,8 +162,10 @@ TEST(MinidumpYAML, ExceptionStream) {
ASSERT_EQ(1u, File.streams().size());
- Expected<const minidump::ExceptionStream &> ExpectedStream =
- File.getExceptionStream();
+ auto ExceptionIterator =
+ File.getExceptionStreams().begin();
+
+ Expected<const ExceptionStream &> ExpectedStream = *ExceptionIterator;
ASSERT_THAT_EXPECTED(ExpectedStream, Succeeded());
@@ -205,9 +207,10 @@ TEST(MinidumpYAML, ExceptionStream_NoParameters) {
ASSERT_EQ(1u, File.streams().size());
- Expected<const minidump::ExceptionStream &> ExpectedStream =
- File.getExceptionStream();
+ auto ExceptionIterator =
+ File.getExceptionStreams().begin();
+ Expected<const ExceptionStream &> ExpectedStream = *ExceptionIterator;
ASSERT_THAT_EXPECTED(ExpectedStream, Succeeded());
const minidump::ExceptionStream &Stream = *ExpectedStream;
@@ -261,8 +264,10 @@ TEST(MinidumpYAML, ExceptionStream_TooManyParameters) {
ASSERT_EQ(1u, File.streams().size());
- Expected<const minidump::ExceptionStream &> ExpectedStream =
- File.getExceptionStream();
+ auto ExceptionIterator =
+ File.getExceptionStreams().begin();
+
+ Expected<const ExceptionStream &> ExpectedStream = *ExceptionIterator;
ASSERT_THAT_EXPECTED(ExpectedStream, Succeeded());
@@ -312,8 +317,10 @@ TEST(MinidumpYAML, ExceptionStream_ExtraParameter) {
ASSERT_EQ(1u, File.streams().size());
- Expected<const minidump::ExceptionStream &> ExpectedStream =
- File.getExceptionStream();
+ auto ExceptionIterator =
+ File.getExceptionStreams().begin();
+
+ Expected<const ExceptionStream &> ExpectedStream = *ExceptionIterator;
ASSERT_THAT_EXPECTED(ExpectedStream, Succeeded());
@@ -398,4 +405,47 @@ TEST(MinidumpYAML, MemoryRegion_64bit) {
ASSERT_THAT(*DescTwoExpectedContentSlice, arrayRefFromStringRef("world"));
ASSERT_EQ(Iterator, MemoryList.end());
+
+}
+
+// Test that we can parse multiple exception streams.
+TEST(MinidumpYAML, ExceptionStream_MultipleExceptions) {
+ SmallString<0> Storage;
+ auto ExpectedFile = toBinary(Storage, R"(
+--- !minidump
+Streams:
+ - Type: Exception
+ Thread ID: 0x7
+ Exception Record:
+ Exception Code: 0x23
+ Exception Flags: 0x5
+ Exception Record: 0x0102030405060708
+ Exception Address: 0x0a0b0c0d0e0f1011
+ Number of Parameters: 2
+ Parameter 0: 0x99
+ Parameter 1: 0x23
+ Parameter 2: 0x42
+ Thread Context: 3DeadBeefDefacedABadCafe
+ - Type: Exception
+ Thread ID: 0x5
+ Exception Record:
+ Exception Code: 0x23
+ Exception Flags: 0x5
+ Exception Record: 0x0102030405060708
+ Exception Address: 0x0a0b0c0d0e0f1011
+ Thread Context: 3DeadBeefDefacedABadCafe)");
+
+ ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+ object::MinidumpFile &File = **ExpectedFile;
+
+ ASSERT_EQ(2u, File.streams().size());
+
+ size_t count = 0;
+ for (auto exception_stream : File.getExceptionStreams()) {
+ count++;
+ ASSERT_THAT_EXPECTED(exception_stream, Succeeded());
+ ASSERT_THAT(0x23u, exception_stream->ExceptionRecord.ExceptionCode);
+ }
+
+ ASSERT_THAT(2u, count);
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
llvm/include/llvm/Object/Minidump.h
Outdated
| static ExceptionStreamsIterator end() { return ExceptionStreamsIterator(); } | ||
|
|
||
| bool operator==(const ExceptionStreamsIterator &R) const { | ||
| return Streams.empty() && R.Streams.empty(); |
There was a problem hiding this comment.
| return Streams.empty() && R.Streams.empty(); | |
| return Streams.begin() == R.Streams.begin(); |
So that it also works for non-end iterators. (You just need to be careful to construct the end iterator with an [end, end) ArrayRef instead of [null, null)
There was a problem hiding this comment.
I replied on the Make_range comment as well, but I coudln't get the initializer list {end(), end()} to build. So I stuck with the behavior of the other iterators in the file, and compared by size.
llvm/lib/Object/Minidump.cpp
Outdated
| return make_range(ExceptionStreamsIterator::begin(ExceptionStreams, this), | ||
| ExceptionStreamsIterator::end()); |
There was a problem hiding this comment.
I.e., something like this:
| return make_range(ExceptionStreamsIterator::begin(ExceptionStreams, this), | |
| ExceptionStreamsIterator::end()); | |
| return make_range(ExceptionStreamsIterator(ExceptionStreams, this), | |
| ExceptionStreamsIterator({ExceptionStreams.end(), ExceptionStreams.end()}, this)); |
There was a problem hiding this comment.
Sadly, the initializer list didn't work. So I looked at the MemoryListIterator, and it compares on size of the backing collection
There was a problem hiding this comment.
Yeah, sorry, I didn't really test this cause I didn't mean it to be taken literally -- just as an illustration of the kind of ArrayRef that you should create. I guess it was necessary to do something like ArrayRef(end(), end()) (or in the worst case ArrayRef<T>(end(), end())) to resolve the ambiguity.
Comparing the sizes is okay (there's the risk of false positives when comparing iterators from two different files, but those kinds are UB for normal iterators anyway), although I think the pointer comparison would produce results more similar to what typical iterators (e.g. raw pointers) would do in these situations.
There was a problem hiding this comment.
No worries, I tried ArrayRef<T>(end(), end()) as well and couldn't get it to work. I liked the idea to support pointer comparison instead of size but I didn't want to increase the scope of the patch
|
@labath As usual great review feedback, but I realized we have a slight issue with this patch. If we just change llvm in this PR, and not the LLDB call sites we will break the build. To simplify this, I'm going to keep the singular Any concerns with this plan? |
…lizer list isn't convertable, and the iterator doesn't have the traits for std::next
…s of it in a different patch
Jlalond
force-pushed
the
llvm-minidump-multiple-exception
branch
from
df500b1 to
66367b9
Compare
If a patch in one project requires changes to another project in order to keep that project building, then it's of course fine to include changes to the other project in the same PR. It's just that they should be kept as small as possible. The thing I want to avoid is making a complex change to one project and simultaneously make a complex change (depending on the first one) in the other project and putting both on the same review. So, I would be fine with making some sort of a minimal change to lldb (e.g. change it just enough to build and use a single exception stream) in this PR. Your approach (keeping the llvm interface until lldb is modified) is the same idea, but in the opposite direction, and it's also fine. |
llvm/lib/Object/Minidump.cpp
Outdated
| return make_range(ExceptionStreamsIterator::begin(ExceptionStreams, this), | ||
| ExceptionStreamsIterator::end()); |
There was a problem hiding this comment.
Yeah, sorry, I didn't really test this cause I didn't mean it to be taken literally -- just as an illustration of the kind of ArrayRef that you should create. I guess it was necessary to do something like ArrayRef(end(), end()) (or in the worst case ArrayRef<T>(end(), end())) to resolve the ambiguity.
Comparing the sizes is okay (there's the risk of false positives when comparing iterators from two different files, but those kinds are UB for normal iterators anyway), although I think the pointer comparison would produce results more similar to what typical iterators (e.g. raw pointers) would do in these situations.
Appreciate it, my question was more for me to learn if there was any unforeseen issues I could run into either way. As I am still new to LLDB/LLVM I'm trying to absorb as much expertise as possible. |
A fork of #97470, splitting off the LLVM changes from the LLDB specific changes. This patch enables a minidump file to have multiple exceptions, exposed via an iterator of Expected streams.