From c6f88d164e44c0c710b783f7b89676b38f79f440 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Thu, 25 Aug 2016 15:01:27 -0700 Subject: [PATCH] Have MinidumpMemoryListWriter deal directly in SnapshotMinidumpMemoryWriters This is as a precursor to https://chromium-review.googlesource.com/374539 which merges MemorySnapshots and so needs to be able to update them from the minidump code. MinidumpMemoryWriter existed to be able to mock for tests; that behaviour is wrapped up in TestMemorySnapshot now. BUG=crashpad:61, chromium:638370 Change-Id: I825ec57493b12fc1848018585c14544faa7e66d4 Reviewed-on: https://chromium-review.googlesource.com/374019 Reviewed-by: Mark Mentovai --- minidump/minidump_memory_writer.cc | 116 ++++++------------ minidump/minidump_memory_writer.h | 74 ++++------- minidump/minidump_thread_writer.cc | 8 +- minidump/minidump_thread_writer.h | 18 +-- .../test/minidump_memory_writer_test_util.cc | 41 +------ .../test/minidump_memory_writer_test_util.h | 19 +-- 6 files changed, 87 insertions(+), 189 deletions(-) diff --git a/minidump/minidump_memory_writer.cc b/minidump/minidump_memory_writer.cc index 064b6e94..9513c739 100644 --- a/minidump/minidump_memory_writer.cc +++ b/minidump/minidump_memory_writer.cc @@ -24,77 +24,45 @@ #include "util/numeric/safe_assignment.h" namespace crashpad { -namespace { -class SnapshotMinidumpMemoryWriter final : public MinidumpMemoryWriter, - public MemorySnapshot::Delegate { - public: - explicit SnapshotMinidumpMemoryWriter(const MemorySnapshot* memory_snapshot) - : MinidumpMemoryWriter(), - MemorySnapshot::Delegate(), - memory_snapshot_(memory_snapshot), - file_writer_(nullptr) { - } +SnapshotMinidumpMemoryWriter::SnapshotMinidumpMemoryWriter( + const MemorySnapshot* memory_snapshot) + : internal::MinidumpWritable(), + MemorySnapshot::Delegate(), + memory_descriptor_(), + registered_memory_descriptors_(), + memory_snapshot_(memory_snapshot), + file_writer_(nullptr) {} - ~SnapshotMinidumpMemoryWriter() override {} +SnapshotMinidumpMemoryWriter::~SnapshotMinidumpMemoryWriter() {} - // MemorySnapshot::Delegate: - - bool MemorySnapshotDelegateRead(void* data, size_t size) override { - DCHECK_EQ(state(), kStateWritable); - DCHECK_EQ(size, MemoryRangeSize()); - return file_writer_->Write(data, size); - } - - protected: - // MinidumpMemoryWriter: - - bool WriteObject(FileWriterInterface* file_writer) override { - DCHECK_EQ(state(), kStateWritable); - DCHECK(!file_writer_); - - base::AutoReset file_writer_reset(&file_writer_, - file_writer); - - // This will result in MemorySnapshotDelegateRead() being called. - return memory_snapshot_->Read(this); - } - - uint64_t MemoryRangeBaseAddress() const override { - DCHECK_EQ(state(), kStateFrozen); - return memory_snapshot_->Address(); - } - - size_t MemoryRangeSize() const override { - DCHECK_GE(state(), kStateFrozen); - return memory_snapshot_->Size(); - } - - private: - const MemorySnapshot* memory_snapshot_; - FileWriterInterface* file_writer_; - - DISALLOW_COPY_AND_ASSIGN(SnapshotMinidumpMemoryWriter); -}; - -} // namespace - -MinidumpMemoryWriter::~MinidumpMemoryWriter() { +bool SnapshotMinidumpMemoryWriter::MemorySnapshotDelegateRead(void* data, + size_t size) { + DCHECK_EQ(state(), kStateWritable); + DCHECK_EQ(size, UnderlyingSnapshot().Size()); + return file_writer_->Write(data, size); } -std::unique_ptr MinidumpMemoryWriter::CreateFromSnapshot( - const MemorySnapshot* memory_snapshot) { - return base::WrapUnique(new SnapshotMinidumpMemoryWriter(memory_snapshot)); +bool SnapshotMinidumpMemoryWriter::WriteObject( + FileWriterInterface* file_writer) { + DCHECK_EQ(state(), kStateWritable); + DCHECK(!file_writer_); + + base::AutoReset file_writer_reset(&file_writer_, + file_writer); + + // This will result in MemorySnapshotDelegateRead() being called. + return memory_snapshot_->Read(this); } const MINIDUMP_MEMORY_DESCRIPTOR* -MinidumpMemoryWriter::MinidumpMemoryDescriptor() const { +SnapshotMinidumpMemoryWriter::MinidumpMemoryDescriptor() const { DCHECK_EQ(state(), kStateWritable); return &memory_descriptor_; } -void MinidumpMemoryWriter::RegisterMemoryDescriptor( +void SnapshotMinidumpMemoryWriter::RegisterMemoryDescriptor( MINIDUMP_MEMORY_DESCRIPTOR* memory_descriptor) { DCHECK_LE(state(), kStateFrozen); @@ -102,13 +70,7 @@ void MinidumpMemoryWriter::RegisterMemoryDescriptor( RegisterLocationDescriptor(&memory_descriptor->Memory); } -MinidumpMemoryWriter::MinidumpMemoryWriter() - : MinidumpWritable(), - memory_descriptor_(), - registered_memory_descriptors_() { -} - -bool MinidumpMemoryWriter::Freeze() { +bool SnapshotMinidumpMemoryWriter::Freeze() { DCHECK_EQ(state(), kStateMutable); if (!MinidumpWritable::Freeze()) { @@ -120,26 +82,26 @@ bool MinidumpMemoryWriter::Freeze() { return true; } -size_t MinidumpMemoryWriter::Alignment() { +size_t SnapshotMinidumpMemoryWriter::Alignment() { DCHECK_GE(state(), kStateFrozen); return 16; } -size_t MinidumpMemoryWriter::SizeOfObject() { +size_t SnapshotMinidumpMemoryWriter::SizeOfObject() { DCHECK_GE(state(), kStateFrozen); - return MemoryRangeSize(); + return UnderlyingSnapshot().Size(); } -bool MinidumpMemoryWriter::WillWriteAtOffsetImpl(FileOffset offset) { +bool SnapshotMinidumpMemoryWriter::WillWriteAtOffsetImpl(FileOffset offset) { DCHECK_EQ(state(), kStateFrozen); // There will always be at least one registered descriptor, the one for this // object’s own memory_descriptor_ field. DCHECK_GE(registered_memory_descriptors_.size(), 1u); - uint64_t base_address = MemoryRangeBaseAddress(); + uint64_t base_address = UnderlyingSnapshot().Address(); decltype(registered_memory_descriptors_[0]->StartOfMemoryRange) local_address; if (!AssignIfInRange(&local_address, base_address)) { LOG(ERROR) << "base_address " << base_address << " out of range"; @@ -154,7 +116,7 @@ bool MinidumpMemoryWriter::WillWriteAtOffsetImpl(FileOffset offset) { return MinidumpWritable::WillWriteAtOffsetImpl(offset); } -internal::MinidumpWritable::Phase MinidumpMemoryWriter::WritePhase() { +internal::MinidumpWritable::Phase SnapshotMinidumpMemoryWriter::WritePhase() { // Memory dumps are large and are unlikely to be consumed in their entirety. // Data accesses are expected to be sparse and sporadic, and are expected to // occur after all of the other structural and informational data from the @@ -178,14 +140,14 @@ void MinidumpMemoryListWriter::AddFromSnapshot( DCHECK_EQ(state(), kStateMutable); for (const MemorySnapshot* memory_snapshot : memory_snapshots) { - std::unique_ptr memory = - MinidumpMemoryWriter::CreateFromSnapshot(memory_snapshot); + std::unique_ptr memory( + new SnapshotMinidumpMemoryWriter(memory_snapshot)); AddMemory(std::move(memory)); } } void MinidumpMemoryListWriter::AddMemory( - std::unique_ptr memory_writer) { + std::unique_ptr memory_writer) { DCHECK_EQ(state(), kStateMutable); AddExtraMemory(memory_writer.get()); @@ -193,7 +155,7 @@ void MinidumpMemoryListWriter::AddMemory( } void MinidumpMemoryListWriter::AddExtraMemory( - MinidumpMemoryWriter* memory_writer) { + SnapshotMinidumpMemoryWriter* memory_writer) { DCHECK_EQ(state(), kStateMutable); memory_writers_.push_back(memory_writer); @@ -232,7 +194,7 @@ std::vector MinidumpMemoryListWriter::Children() { DCHECK_LE(children_.size(), memory_writers_.size()); std::vector children; - for (MinidumpMemoryWriter* child : children_) { + for (SnapshotMinidumpMemoryWriter* child : children_) { children.push_back(child); } @@ -247,7 +209,7 @@ bool MinidumpMemoryListWriter::WriteObject(FileWriterInterface* file_writer) { iov.iov_len = sizeof(memory_list_base_); std::vector iovecs(1, iov); - for (const MinidumpMemoryWriter* memory_writer : memory_writers_) { + for (const SnapshotMinidumpMemoryWriter* memory_writer : memory_writers_) { iov.iov_base = memory_writer->MinidumpMemoryDescriptor(); iov.iov_len = sizeof(MINIDUMP_MEMORY_DESCRIPTOR); iovecs.push_back(iov); diff --git a/minidump/minidump_memory_writer.h b/minidump/minidump_memory_writer.h index c23997a2..b01505fa 100644 --- a/minidump/minidump_memory_writer.h +++ b/minidump/minidump_memory_writer.h @@ -26,34 +26,19 @@ #include "base/macros.h" #include "minidump/minidump_stream_writer.h" #include "minidump/minidump_writable.h" +#include "snapshot/memory_snapshot.h" #include "util/file/file_io.h" #include "util/stdlib/pointer_container.h" namespace crashpad { -class MemorySnapshot; - //! \brief The base class for writers of memory ranges pointed to by //! MINIDUMP_MEMORY_DESCRIPTOR objects in a minidump file. -//! -//! This is an abstract base class because users are expected to provide their -//! own implementations that, when possible, obtain the memory contents -//! on-demand in their WriteObject() methods. Memory ranges may be large, and -//! the alternative construction would require the contents of multiple ranges -//! to be held in memory simultaneously while a minidump file is being written. -class MinidumpMemoryWriter : public internal::MinidumpWritable { +class SnapshotMinidumpMemoryWriter : public internal::MinidumpWritable, + public MemorySnapshot::Delegate { public: - ~MinidumpMemoryWriter() override; - - //! \brief Creates a concrete initialized MinidumpMemoryWriter based on \a - //! memory_snapshot. - //! - //! \param[in] memory_snapshot The memory snapshot to use as source data. - //! - //! \return An object of a MinidumpMemoryWriter subclass initialized using the - //! source data in \a memory_snapshot. - static std::unique_ptr CreateFromSnapshot( - const MemorySnapshot* memory_snapshot); + explicit SnapshotMinidumpMemoryWriter(const MemorySnapshot* memory_snapshot); + ~SnapshotMinidumpMemoryWriter() override; //! \brief Returns a MINIDUMP_MEMORY_DESCRIPTOR referencing the data that this //! object writes. @@ -76,24 +61,14 @@ class MinidumpMemoryWriter : public internal::MinidumpWritable { //! \note Valid in #kStateFrozen or any preceding state. void RegisterMemoryDescriptor(MINIDUMP_MEMORY_DESCRIPTOR* memory_descriptor); - protected: - MinidumpMemoryWriter(); - - //! \brief Returns the base address of the memory region in the address space - //! of the process that the snapshot describes. - //! - //! \note This method will only be called in #kStateFrozen. - virtual uint64_t MemoryRangeBaseAddress() const = 0; - - //! \brief Returns the size of the memory region in bytes. - //! - //! \note This method will only be called in #kStateFrozen or a subsequent - //! state. - virtual size_t MemoryRangeSize() const = 0; + private: + // MemorySnapshot::Delegate: + bool MemorySnapshotDelegateRead(void* data, size_t size) override; // MinidumpWritable: bool Freeze() override; size_t SizeOfObject() final; + bool WriteObject(FileWriterInterface* file_writer) override; //! \brief Returns the object’s desired byte-boundary alignment. //! @@ -119,13 +94,18 @@ class MinidumpMemoryWriter : public internal::MinidumpWritable { //! \note Valid in any state. Phase WritePhase() final; - private: + //! \brief Gets the underlying memory snapshot that the memory writer will + //! write to the minidump. + const MemorySnapshot& UnderlyingSnapshot() const { return *memory_snapshot_; } + MINIDUMP_MEMORY_DESCRIPTOR memory_descriptor_; // weak std::vector registered_memory_descriptors_; + const MemorySnapshot* memory_snapshot_; + FileWriterInterface* file_writer_; - DISALLOW_COPY_AND_ASSIGN(MinidumpMemoryWriter); + DISALLOW_COPY_AND_ASSIGN(SnapshotMinidumpMemoryWriter); }; //! \brief The writer for a MINIDUMP_MEMORY_LIST stream in a minidump file, @@ -135,8 +115,8 @@ class MinidumpMemoryListWriter final : public internal::MinidumpStreamWriter { MinidumpMemoryListWriter(); ~MinidumpMemoryListWriter() override; - //! \brief Adds a concrete initialized MinidumpMemoryWriter for each memory - //! snapshot in \a memory_snapshots to the MINIDUMP_MEMORY_LIST. + //! \brief Adds a concrete initialized SnapshotMinidumpMemoryWriter for each + //! memory snapshot in \a memory_snapshots to the MINIDUMP_MEMORY_LIST. //! //! Memory snapshots are added in the fashion of AddMemory(). //! @@ -146,15 +126,15 @@ class MinidumpMemoryListWriter final : public internal::MinidumpStreamWriter { void AddFromSnapshot( const std::vector& memory_snapshots); - //! \brief Adds a MinidumpMemoryWriter to the MINIDUMP_MEMORY_LIST. + //! \brief Adds a SnapshotMinidumpMemoryWriter to the MINIDUMP_MEMORY_LIST. //! //! This object takes ownership of \a memory_writer and becomes its parent in //! the overall tree of internal::MinidumpWritable objects. //! //! \note Valid in #kStateMutable. - void AddMemory(std::unique_ptr memory_writer); + void AddMemory(std::unique_ptr memory_writer); - //! \brief Adds a MinidumpMemoryWriter that’s a child of another + //! \brief Adds a SnapshotMinidumpMemoryWriter that’s a child of another //! internal::MinidumpWritable object to the MINIDUMP_MEMORY_LIST. //! //! \a memory_writer does not become a child of this object, but the @@ -163,12 +143,12 @@ class MinidumpMemoryListWriter final : public internal::MinidumpStreamWriter { //! internal::MinidumpWritable tree. //! //! This method exists to be called by objects that have their own - //! MinidumpMemoryWriter children but wish for them to also appear in the - //! minidump file’s MINIDUMP_MEMORY_LIST. MinidumpThreadWriter, which has a - //! MinidumpMemoryWriter for thread stack memory, is an example. + //! SnapshotMinidumpMemoryWriter children but wish for them to also appear in + //! the minidump file’s MINIDUMP_MEMORY_LIST. MinidumpThreadWriter, which has + //! a SnapshotMinidumpMemoryWriter for thread stack memory, is an example. //! //! \note Valid in #kStateMutable. - void AddExtraMemory(MinidumpMemoryWriter* memory_writer); + void AddExtraMemory(SnapshotMinidumpMemoryWriter* memory_writer); protected: // MinidumpWritable: @@ -181,8 +161,8 @@ class MinidumpMemoryListWriter final : public internal::MinidumpStreamWriter { MinidumpStreamType StreamType() const override; private: - std::vector memory_writers_; // weak - PointerVector children_; + std::vector memory_writers_; // weak + PointerVector children_; MINIDUMP_MEMORY_LIST memory_list_base_; DISALLOW_COPY_AND_ASSIGN(MinidumpMemoryListWriter); diff --git a/minidump/minidump_thread_writer.cc b/minidump/minidump_thread_writer.cc index 0dd1dba9..9307ed84 100644 --- a/minidump/minidump_thread_writer.cc +++ b/minidump/minidump_thread_writer.cc @@ -51,8 +51,8 @@ void MinidumpThreadWriter::InitializeFromSnapshot( const MemorySnapshot* stack_snapshot = thread_snapshot->Stack(); if (stack_snapshot && stack_snapshot->Size() > 0) { - std::unique_ptr stack = - MinidumpMemoryWriter::CreateFromSnapshot(stack_snapshot); + std::unique_ptr stack( + new SnapshotMinidumpMemoryWriter(stack_snapshot)); SetStack(std::move(stack)); } @@ -68,7 +68,7 @@ const MINIDUMP_THREAD* MinidumpThreadWriter::MinidumpThread() const { } void MinidumpThreadWriter::SetStack( - std::unique_ptr stack) { + std::unique_ptr stack) { DCHECK_EQ(state(), kStateMutable); stack_ = std::move(stack); @@ -172,7 +172,7 @@ void MinidumpThreadListWriter::AddThread( DCHECK_EQ(state(), kStateMutable); if (memory_list_writer_) { - MinidumpMemoryWriter* stack = thread->Stack(); + SnapshotMinidumpMemoryWriter* stack = thread->Stack(); if (stack) { memory_list_writer_->AddExtraMemory(stack); } diff --git a/minidump/minidump_thread_writer.h b/minidump/minidump_thread_writer.h index e5094fef..e34ea061 100644 --- a/minidump/minidump_thread_writer.h +++ b/minidump/minidump_thread_writer.h @@ -33,7 +33,7 @@ namespace crashpad { class MinidumpContextWriter; class MinidumpMemoryListWriter; -class MinidumpMemoryWriter; +class SnapshotMinidumpMemoryWriter; class ThreadSnapshot; //! \brief The writer for a MINIDUMP_THREAD object in a minidump file. @@ -67,8 +67,8 @@ class MinidumpThreadWriter final : public internal::MinidumpWritable { //! \note Valid in #kStateWritable. const MINIDUMP_THREAD* MinidumpThread() const; - //! \brief Returns a MinidumpMemoryWriter that will write the memory region - //! corresponding to this object’s stack. + //! \brief Returns a SnapshotMinidumpMemoryWriter that will write the memory + //! region corresponding to this object’s stack. //! //! If the thread does not have a stack, or its stack could not be determined, //! this will return `nullptr`. @@ -80,7 +80,7 @@ class MinidumpThreadWriter final : public internal::MinidumpWritable { //! MinidumpMemoryListWriter::AddExtraMemory(). //! //! \note Valid in any state. - MinidumpMemoryWriter* Stack() const { return stack_.get(); } + SnapshotMinidumpMemoryWriter* Stack() const { return stack_.get(); } //! \brief Arranges for MINIDUMP_THREAD::Stack to point to the MINIDUMP_MEMORY //! object to be written by \a stack. @@ -89,7 +89,7 @@ class MinidumpThreadWriter final : public internal::MinidumpWritable { //! overall tree of internal::MinidumpWritable objects. //! //! \note Valid in #kStateMutable. - void SetStack(std::unique_ptr stack); + void SetStack(std::unique_ptr stack); //! \brief Arranges for MINIDUMP_THREAD::ThreadContext to point to the CPU //! context to be written by \a context. @@ -130,7 +130,7 @@ class MinidumpThreadWriter final : public internal::MinidumpWritable { private: MINIDUMP_THREAD thread_; - std::unique_ptr stack_; + std::unique_ptr stack_; std::unique_ptr context_; DISALLOW_COPY_AND_ASSIGN(MinidumpThreadWriter); @@ -161,9 +161,9 @@ class MinidumpThreadListWriter final : public internal::MinidumpStreamWriter { //! region should be added to as extra memory. //! //! Each MINIDUMP_THREAD object can contain a reference to a - //! MinidumpMemoryWriter object that contains a snapshot of its stack memory. - //! In the overall tree of internal::MinidumpWritable objects, these - //! MinidumpMemoryWriter objects are considered children of their + //! SnapshotMinidumpMemoryWriter object that contains a snapshot of its stac + //! memory. In the overall tree of internal::MinidumpWritable objects, these + //! SnapshotMinidumpMemoryWriter objects are considered children of their //! MINIDUMP_THREAD, and are referenced by a MINIDUMP_MEMORY_DESCRIPTOR //! contained in the MINIDUMP_THREAD. It is also possible for the same memory //! regions to have MINIDUMP_MEMORY_DESCRIPTOR objects present in a diff --git a/minidump/test/minidump_memory_writer_test_util.cc b/minidump/test/minidump_memory_writer_test_util.cc index 4944e6df..3b94b9c8 100644 --- a/minidump/test/minidump_memory_writer_test_util.cc +++ b/minidump/test/minidump_memory_writer_test_util.cc @@ -22,48 +22,15 @@ namespace test { TestMinidumpMemoryWriter::TestMinidumpMemoryWriter(uint64_t base_address, size_t size, uint8_t value) - : MinidumpMemoryWriter(), - base_address_(base_address), - expected_offset_(-1), - size_(size), - value_(value) { + : SnapshotMinidumpMemoryWriter(&test_snapshot_) { + test_snapshot_.SetAddress(base_address); + test_snapshot_.SetSize(size); + test_snapshot_.SetValue(value); } TestMinidumpMemoryWriter::~TestMinidumpMemoryWriter() { } -uint64_t TestMinidumpMemoryWriter::MemoryRangeBaseAddress() const { - EXPECT_EQ(state(), kStateFrozen); - return base_address_; -} - -size_t TestMinidumpMemoryWriter::MemoryRangeSize() const { - EXPECT_GE(state(), kStateFrozen); - return size_; -} - -bool TestMinidumpMemoryWriter::WillWriteAtOffsetImpl(FileOffset offset) { - EXPECT_EQ(state(), kStateFrozen); - expected_offset_ = offset; - bool rv = MinidumpMemoryWriter::WillWriteAtOffsetImpl(offset); - EXPECT_TRUE(rv); - return rv; -} - -bool TestMinidumpMemoryWriter::WriteObject(FileWriterInterface* file_writer) { - EXPECT_EQ(state(), kStateWritable); - EXPECT_EQ(expected_offset_, file_writer->Seek(0, SEEK_CUR)); - - bool rv = true; - if (size_ > 0) { - std::string data(size_, value_); - rv = file_writer->Write(&data[0], size_); - EXPECT_TRUE(rv); - } - - return rv; -} - void ExpectMinidumpMemoryDescriptor( const MINIDUMP_MEMORY_DESCRIPTOR* expected, const MINIDUMP_MEMORY_DESCRIPTOR* observed) { diff --git a/minidump/test/minidump_memory_writer_test_util.h b/minidump/test/minidump_memory_writer_test_util.h index 5c56898c..db198909 100644 --- a/minidump/test/minidump_memory_writer_test_util.h +++ b/minidump/test/minidump_memory_writer_test_util.h @@ -25,34 +25,23 @@ #include #include "base/macros.h" +#include "snapshot/test/test_memory_snapshot.h" #include "util/file/file_writer.h" namespace crashpad { namespace test { -//! \brief A MinidumpMemoryWriter implementation used for testing. +//! \brief A SnapshotMinidumpMemoryWriter implementation used for testing. //! //! TestMinidumpMemoryWriter objects are created with a fixed base address and //! size, and will write the same byte (\a value) repeatedly, \a size times. -class TestMinidumpMemoryWriter final : public MinidumpMemoryWriter { +class TestMinidumpMemoryWriter final : public SnapshotMinidumpMemoryWriter { public: TestMinidumpMemoryWriter(uint64_t base_address, size_t size, uint8_t value); ~TestMinidumpMemoryWriter(); - protected: - // MinidumpMemoryWriter: - uint64_t MemoryRangeBaseAddress() const override; - size_t MemoryRangeSize() const override; - - // MinidumpWritable: - bool WillWriteAtOffsetImpl(FileOffset offset) override; - bool WriteObject(FileWriterInterface* file_writer) override; - private: - uint64_t base_address_; - FileOffset expected_offset_; - size_t size_; - uint8_t value_; + TestMemorySnapshot test_snapshot_; DISALLOW_COPY_AND_ASSIGN(TestMinidumpMemoryWriter); };