minidump: Refactor MinidumpCrashpadInfoListWriter into a base class,

MinidumpLocationDescriptorListWriter. This allows easier construction of
minidump structures that are lists of MINIDUMP_LOCATION_DESCRIPTOR
structures in the future.

TEST=minidump_test
R=rsesek@chromium.org

Review URL: https://codereview.chromium.org/707543002

minidump/minidump.gyp

@@ -41,6 +41,8 @@
         'minidump_extensions.h',
         'minidump_file_writer.cc',
         'minidump_file_writer.h',
+        'minidump_location_descriptor_list_writer.cc',
+        'minidump_location_descriptor_list_writer.h',
         'minidump_memory_writer.cc',
         'minidump_memory_writer.h',
         'minidump_misc_info_writer.cc',
@@ -85,6 +87,7 @@
         'minidump_crashpad_info_writer_test.cc',
         'minidump_exception_writer_test.cc',
         'minidump_file_writer_test.cc',
+        'minidump_location_descriptor_list_writer_test.cc',
         'minidump_memory_writer_test.cc',
         'minidump_misc_info_writer_test.cc',
         'minidump_module_crashpad_info_writer_test.cc',
@@ -99,6 +102,8 @@
         'test/minidump_context_test_util.h',
         'test/minidump_file_writer_test_util.cc',
         'test/minidump_file_writer_test_util.h',
+        'test/minidump_location_descriptor_list_test_util.cc',
+        'test/minidump_location_descriptor_list_test_util.h',
         'test/minidump_memory_writer_test_util.cc',
         'test/minidump_memory_writer_test_util.h',
         'test/minidump_string_writer_test_util.cc',

minidump/minidump_crashpad_info_writer_test.cc

@@ -97,7 +97,7 @@ TEST(MinidumpCrashpadInfoWriter, CrashpadModuleList) {
 
   const MinidumpModuleCrashpadInfo* module =
       MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfo>(
-          file_writer.string(), module_list->modules[0]);
+          file_writer.string(), module_list->children[0]);
   ASSERT_TRUE(module);
 
   EXPECT_EQ(MinidumpModuleCrashpadInfo::kVersion, module->version);

minidump/minidump_extensions.h

@@ -296,6 +296,15 @@ struct MinidumpModuleCodeViewRecordPDB70 {
   uint8_t pdb_name[1];
 };
 
+//! \brief A list of MINIDUMP_LOCATION_DESCRIPTOR objects.
+struct __attribute__((packed, aligned(4))) MinidumpLocationDescriptorList {
+  //! \brief The number of children present in the #children array.
+  uint32_t count;
+
+  //! \brief Pointers to other structures in the minidump file.
+  MINIDUMP_LOCATION_DESCRIPTOR children[0];
+};
+
 //! \brief A key-value pair.
 struct __attribute__((packed, aligned(4))) MinidumpSimpleStringDictionaryEntry {
   //! \brief ::RVA of a MinidumpUTF8String containing the key of a key-value
@@ -330,7 +339,7 @@ struct __attribute__((packed, aligned(4))) MinidumpSimpleStringDictionary {
 //! #version, so that newer parsers will be able to determine whether the added
 //! fields are valid or not.
 //!
-//! \sa MinidumpModuleCrashpadInfoList
+//! \sa #MinidumpModuleCrashpadInfoList
 struct __attribute__((packed, aligned(4))) MinidumpModuleCrashpadInfo {
   //! \brief The structure’s currently-defined version number.
   //!
@@ -369,22 +378,18 @@ struct __attribute__((packed, aligned(4))) MinidumpModuleCrashpadInfo {
 //! This structure augments the information provided by
 //! MINIDUMP_MODULE_LIST. The minidump file must contain a module list stream
 //! (::kMinidumpStreamTypeModuleList) in order for this structure to appear.
-struct __attribute__((packed, aligned(4))) MinidumpModuleCrashpadInfoList {
+//!
-  //! \brief The number of modules present in the #modules array.
+//! MinidumpModuleCrashpadInfoList::count may be less than the value of
-  //!
+//! MINIDUMP_MODULE_LIST::NumberOfModules because not every MINIDUMP_MODULE
-  //! This may be less than the value of MINIDUMP_MODULE_LIST::NumberOfModules
+//! structure carried within the minidump file will necessarily have
-  //! because not every MINIDUMP_MODULE structure carried within the minidump
+//! Crashpad-specific information provided by a MinidumpModuleCrashpadInfo
-  //! file will necessarily have Crashpad-specific information provided by a
+//! structure.
-  //! MinidumpModuleCrashpadInfo structure.
+//!
-  uint32_t count;
+//! MinidumpModuleCrashpadInfoList::children references
-
+//! MinidumpModuleCrashpadInfo children indirectly through
-  //! \brief Pointers to MinidumpModuleCrashpadInfo structures.
+//! MINIDUMP_LOCATION_DESCRIPTOR pointers to allow for future growth of the
-  //!
+//! MinidumpModuleCrashpadInfo structure.
-  //! These are referenced indirectly through MINIDUMP_LOCATION_DESCRIPTOR
+using MinidumpModuleCrashpadInfoList = MinidumpLocationDescriptorList;
-  //! pointers to allow for future growth of the MinidumpModuleCrashpadInfo
-  //! structure.
-  MINIDUMP_LOCATION_DESCRIPTOR modules[0];
-};
 
 //! \brief Additional Crashpad-specific information carried within a minidump
 //!     file.
@@ -412,7 +417,7 @@ struct __attribute__((packed, aligned(4))) MinidumpCrashpadInfo {
   //! no need for any fields present in later versions.
   uint32_t version;
 
-  //! \brief A pointer to a MinidumpModuleCrashpadInfoList structure.
+  //! \brief A pointer to a #MinidumpModuleCrashpadInfoList structure.
   //!
   //! This field is present when #version is at least `1`.
   MINIDUMP_LOCATION_DESCRIPTOR module_list;

minidump/minidump_location_descriptor_list_writer.cc

@@ -0,0 +1,102 @@
+// Copyright 2014 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "minidump/minidump_location_descriptor_list_writer.h"
+
+#include "base/logging.h"
+#include "util/file/file_writer.h"
+#include "util/numeric/safe_assignment.h"
+
+namespace crashpad {
+
+MinidumpLocationDescriptorListWriter::MinidumpLocationDescriptorListWriter()
+    : MinidumpWritable(),
+      location_descriptor_list_base_(),
+      children_(),
+      child_location_descriptors_() {
+}
+
+MinidumpLocationDescriptorListWriter::~MinidumpLocationDescriptorListWriter() {
+}
+
+void MinidumpLocationDescriptorListWriter::AddChild(
+    scoped_ptr<internal::MinidumpWritable> child) {
+  DCHECK_EQ(state(), kStateMutable);
+
+  children_.push_back(child.release());
+}
+
+bool MinidumpLocationDescriptorListWriter::Freeze() {
+  DCHECK_EQ(state(), kStateMutable);
+  DCHECK(child_location_descriptors_.empty());
+
+  if (!MinidumpWritable::Freeze()) {
+    return false;
+  }
+
+  size_t child_count = children_.size();
+  if (!AssignIfInRange(&location_descriptor_list_base_.count, child_count)) {
+    LOG(ERROR) << "child_count " << child_count << " out of range";
+    return false;
+  }
+
+  child_location_descriptors_.resize(child_count);
+  for (size_t index = 0; index < child_count; ++index) {
+    children_[index]->RegisterLocationDescriptor(
+        &child_location_descriptors_[index]);
+  }
+
+  return true;
+}
+
+size_t MinidumpLocationDescriptorListWriter::SizeOfObject() {
+  DCHECK_GE(state(), kStateFrozen);
+
+  return sizeof(location_descriptor_list_base_) +
+         children_.size() * sizeof(MINIDUMP_LOCATION_DESCRIPTOR);
+}
+
+std::vector<internal::MinidumpWritable*>
+MinidumpLocationDescriptorListWriter::Children() {
+  DCHECK_GE(state(), kStateFrozen);
+
+  std::vector<MinidumpWritable*> children;
+  for (MinidumpWritable* child : children_) {
+    children.push_back(child);
+  }
+
+  return children;
+}
+
+bool MinidumpLocationDescriptorListWriter::WriteObject(
+    FileWriterInterface* file_writer) {
+  DCHECK_EQ(state(), kStateWritable);
+  DCHECK_EQ(children_.size(), child_location_descriptors_.size());
+
+  WritableIoVec iov;
+  iov.iov_base = &location_descriptor_list_base_;
+  iov.iov_len = sizeof(location_descriptor_list_base_);
+  std::vector<WritableIoVec> iovecs(1, iov);
+
+  if (!child_location_descriptors_.empty()) {
+    iov.iov_base = &child_location_descriptors_[0];
+    iov.iov_len = child_location_descriptors_.size() *
+                  sizeof(MINIDUMP_LOCATION_DESCRIPTOR);
+    iovecs.push_back(iov);
+  }
+
+  return file_writer->WriteIoVec(&iovecs);
+}
+
+}  // namespace crashpad

minidump/minidump_location_descriptor_list_writer.h

@@ -0,0 +1,82 @@
+// Copyright 2014 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef CRASHPAD_MINIDUMP_LOCATION_DESCRIPTOR_LIST_WRITER_H_
+#define CRASHPAD_MINIDUMP_LOCATION_DESCRIPTOR_LIST_WRITER_H_
+
+#include <stdint.h>
+#include <sys/types.h>
+
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+#include "minidump/minidump_extensions.h"
+#include "minidump/minidump_writable.h"
+#include "util/stdlib/pointer_container.h"
+
+namespace crashpad {
+
+//! \brief The writer for a MinidumpLocationDescriptorList object in a minidump
+//!     file, containing a list of MINIDUMP_LOCATION_DESCRIPTOR objects.
+class MinidumpLocationDescriptorListWriter : public internal::MinidumpWritable {
+ protected:
+  MinidumpLocationDescriptorListWriter();
+  ~MinidumpLocationDescriptorListWriter() override;
+
+  //! \brief Adds a MINIDUMP_LOCATION_DESCRIPTOR referencing an
+  //!     internal::MinidumpWritable to the MinidumpLocationDescriptorList.
+  //!
+  //! This object takes ownership of \a child and becomes its parent in the
+  //! overall tree of internal::MinidumpWritable objects.
+  //!
+  //! To provide type-correctness, subclasses are expected to provide a public
+  //! method that accepts a `scoped_ptr`-wrapped argument of the proper
+  //! internal::MinidumpWritable subclass, and call this method with that
+  //! argument.
+  //!
+  //! \note Valid in #kStateMutable.
+  void AddChild(scoped_ptr<MinidumpWritable> child);
+
+  //! \brief Returns `true` if no child objects have been added by AddChild(),
+  //!     and `false` if child objects are present.
+  bool IsEmpty() const { return children_.empty(); }
+
+  //! \brief Returns an object’s MINIDUMP_LOCATION_DESCRIPTOR objects
+  //!     referencing its children.
+  //!
+  //! \note The returned vector will be empty until the object advances to
+  //!     #kStateFrozen or beyond.
+  const std::vector<MINIDUMP_LOCATION_DESCRIPTOR>& child_location_descriptors()
+      const {
+    return child_location_descriptors_;
+  }
+
+  // MinidumpWritable:
+  bool Freeze() override;
+  size_t SizeOfObject() override;
+  std::vector<MinidumpWritable*> Children() override;
+  bool WriteObject(FileWriterInterface* file_writer) override;
+
+ private:
+  MinidumpLocationDescriptorList location_descriptor_list_base_;
+  PointerVector<MinidumpWritable> children_;
+  std::vector<MINIDUMP_LOCATION_DESCRIPTOR> child_location_descriptors_;
+
+  DISALLOW_COPY_AND_ASSIGN(MinidumpLocationDescriptorListWriter);
+};
+
+}  // namespace crashpad
+
+#endif  // CRASHPAD_MINIDUMP_LOCATION_DESCRIPTOR_LIST_WRITER_H_

minidump/minidump_location_descriptor_list_writer_test.cc

@@ -0,0 +1,133 @@
+// Copyright 2014 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "minidump/minidump_location_descriptor_list_writer.h"
+
+#include "base/strings/stringprintf.h"
+#include "gtest/gtest.h"
+#include "minidump/test/minidump_location_descriptor_list_test_util.h"
+#include "minidump/test/minidump_writable_test_util.h"
+#include "util/file/string_file_writer.h"
+
+namespace crashpad {
+namespace test {
+namespace {
+
+class TestMinidumpWritable final : public internal::MinidumpWritable {
+ public:
+  explicit TestMinidumpWritable(uint32_t value)
+      : MinidumpWritable(),
+        value_(value) {
+  }
+
+  ~TestMinidumpWritable() override {}
+
+ protected:
+  // MinidumpWritable:
+
+  size_t SizeOfObject() override { return sizeof(value_); }
+
+  bool WriteObject(FileWriterInterface* file_writer) override {
+    return file_writer->Write(&value_, sizeof(value_));
+  }
+
+ private:
+  uint32_t value_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestMinidumpWritable);
+};
+
+class TestMinidumpLocationDescriptorListWriter final
+    : public MinidumpLocationDescriptorListWriter {
+ public:
+  TestMinidumpLocationDescriptorListWriter()
+      : MinidumpLocationDescriptorListWriter() {
+  }
+
+  ~TestMinidumpLocationDescriptorListWriter() override {}
+
+  void AddChild(uint32_t value) {
+    auto child = make_scoped_ptr(new TestMinidumpWritable(value));
+    MinidumpLocationDescriptorListWriter::AddChild(child.Pass());
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestMinidumpLocationDescriptorListWriter);
+};
+
+TEST(MinidumpLocationDescriptorListWriter, Empty) {
+  TestMinidumpLocationDescriptorListWriter list_writer;
+
+  StringFileWriter file_writer;
+
+  ASSERT_TRUE(list_writer.WriteEverything(&file_writer));
+  EXPECT_EQ(sizeof(MinidumpLocationDescriptorList),
+            file_writer.string().size());
+
+  const MinidumpLocationDescriptorList* list =
+      MinidumpLocationDescriptorListAtStart(file_writer.string(), 0);
+  ASSERT_TRUE(list);
+}
+
+TEST(MinidumpLocationDescriptorListWriter, OneChild) {
+  TestMinidumpLocationDescriptorListWriter list_writer;
+
+  const uint32_t kValue = 0;
+  list_writer.AddChild(kValue);
+
+  StringFileWriter file_writer;
+
+  ASSERT_TRUE(list_writer.WriteEverything(&file_writer));
+
+  const MinidumpLocationDescriptorList* list =
+      MinidumpLocationDescriptorListAtStart(file_writer.string(), 1);
+  ASSERT_TRUE(list);
+
+  const uint32_t* child = MinidumpWritableAtLocationDescriptor<uint32_t>(
+      file_writer.string(), list->children[0]);
+  ASSERT_TRUE(child);
+  EXPECT_EQ(kValue, *child);
+}
+
+TEST(MinidumpLocationDescriptorListWriter, ThreeChildren) {
+  TestMinidumpLocationDescriptorListWriter list_writer;
+
+  const uint32_t kValues[] = { 0x80000000, 0x55555555, 0x66006600 };
+
+  list_writer.AddChild(kValues[0]);
+  list_writer.AddChild(kValues[1]);
+  list_writer.AddChild(kValues[2]);
+
+  StringFileWriter file_writer;
+
+  ASSERT_TRUE(list_writer.WriteEverything(&file_writer));
+
+  const MinidumpLocationDescriptorList* list =
+      MinidumpLocationDescriptorListAtStart(file_writer.string(),
+                                            arraysize(kValues));
+  ASSERT_TRUE(list);
+
+  for (size_t index = 0; index < arraysize(kValues); ++index) {
+    SCOPED_TRACE(base::StringPrintf("index %zu", index));
+
+    const uint32_t* child = MinidumpWritableAtLocationDescriptor<uint32_t>(
+        file_writer.string(), list->children[index]);
+    ASSERT_TRUE(child);
+    EXPECT_EQ(kValues[index], *child);
+  }
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace crashpad

minidump/minidump_module_crashpad_info_writer.cc

@@ -105,10 +105,7 @@ bool MinidumpModuleCrashpadInfoWriter::WriteObject(
 }
 
 MinidumpModuleCrashpadInfoListWriter::MinidumpModuleCrashpadInfoListWriter()
-    : MinidumpWritable(),
+    : MinidumpLocationDescriptorListWriter() {
-      module_list_base_(),
-      modules_(),
-      module_location_descriptors_() {
 }
 
 MinidumpModuleCrashpadInfoListWriter::~MinidumpModuleCrashpadInfoListWriter() {
@@ -117,8 +114,8 @@ MinidumpModuleCrashpadInfoListWriter::~MinidumpModuleCrashpadInfoListWriter() {
 void MinidumpModuleCrashpadInfoListWriter::InitializeFromSnapshot(
     const std::vector<const ModuleSnapshot*>& module_snapshots) {
   DCHECK_EQ(state(), kStateMutable);
-  DCHECK(modules_.empty());
+  DCHECK(IsEmpty());
-  DCHECK(module_location_descriptors_.empty());
+  DCHECK(child_location_descriptors().empty());
 
   size_t count = module_snapshots.size();
   for (size_t index = 0; index < count; ++index) {
@@ -136,69 +133,7 @@ void MinidumpModuleCrashpadInfoListWriter::AddModule(
     scoped_ptr<MinidumpModuleCrashpadInfoWriter> module) {
   DCHECK_EQ(state(), kStateMutable);
 
-  modules_.push_back(module.release());
+  AddChild(module.Pass());
-}
-
-bool MinidumpModuleCrashpadInfoListWriter::Freeze() {
-  DCHECK_EQ(state(), kStateMutable);
-  DCHECK(module_location_descriptors_.empty());
-
-  if (!MinidumpWritable::Freeze()) {
-    return false;
-  }
-
-  size_t module_count = modules_.size();
-  if (!AssignIfInRange(&module_list_base_.count, module_count)) {
-    LOG(ERROR) << "module_count " << module_count << " out of range";
-    return false;
-  }
-
-  module_location_descriptors_.resize(module_count);
-  for (size_t index = 0; index < module_count; ++index) {
-    modules_[index]->RegisterLocationDescriptor(
-        &module_location_descriptors_[index]);
-  }
-
-  return true;
-}
-
-size_t MinidumpModuleCrashpadInfoListWriter::SizeOfObject() {
-  DCHECK_GE(state(), kStateFrozen);
-
-  return sizeof(module_list_base_) +
-         modules_.size() * sizeof(MINIDUMP_LOCATION_DESCRIPTOR);
-}
-
-std::vector<internal::MinidumpWritable*>
-MinidumpModuleCrashpadInfoListWriter::Children() {
-  DCHECK_GE(state(), kStateFrozen);
-
-  std::vector<MinidumpWritable*> children;
-  for (MinidumpModuleCrashpadInfoWriter* module : modules_) {
-    children.push_back(module);
-  }
-
-  return children;
-}
-
-bool MinidumpModuleCrashpadInfoListWriter::WriteObject(
-    FileWriterInterface* file_writer) {
-  DCHECK_EQ(state(), kStateWritable);
-  DCHECK_EQ(modules_.size(), module_location_descriptors_.size());
-
-  WritableIoVec iov;
-  iov.iov_base = &module_list_base_;
-  iov.iov_len = sizeof(module_list_base_);
-  std::vector<WritableIoVec> iovecs(1, iov);
-
-  if (!module_location_descriptors_.empty()) {
-    iov.iov_base = &module_location_descriptors_[0];
-    iov.iov_len = module_location_descriptors_.size() *
-                  sizeof(MINIDUMP_LOCATION_DESCRIPTOR);
-    iovecs.push_back(iov);
-  }
-
-  return file_writer->WriteIoVec(&iovecs);
 }
 
 }  // namespace crashpad

minidump/minidump_module_crashpad_info_writer.h

@@ -23,6 +23,7 @@
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "minidump/minidump_extensions.h"
+#include "minidump/minidump_location_descriptor_list_writer.h"
 #include "minidump/minidump_writable.h"
 #include "util/stdlib/pointer_container.h"
 
@@ -99,7 +100,7 @@ class MinidumpModuleCrashpadInfoWriter final
 //! \brief The writer for a MinidumpModuleCrashpadInfoList object in a minidump
 //!     file, containing a list of MinidumpModuleCrashpadInfo objects.
 class MinidumpModuleCrashpadInfoListWriter final
-    : public internal::MinidumpWritable {
+    : public MinidumpLocationDescriptorListWriter {
  public:
   MinidumpModuleCrashpadInfoListWriter();
   ~MinidumpModuleCrashpadInfoListWriter() override;
@@ -128,18 +129,7 @@ class MinidumpModuleCrashpadInfoListWriter final
   //! \note Valid in #kStateMutable.
   void AddModule(scoped_ptr<MinidumpModuleCrashpadInfoWriter> module);
 
- protected:
-  // MinidumpWritable:
-  bool Freeze() override;
-  size_t SizeOfObject() override;
-  std::vector<MinidumpWritable*> Children() override;
-  bool WriteObject(FileWriterInterface* file_writer) override;
-
  private:
-  MinidumpModuleCrashpadInfoList module_list_base_;
-  PointerVector<MinidumpModuleCrashpadInfoWriter> modules_;
-  std::vector<MINIDUMP_LOCATION_DESCRIPTOR> module_location_descriptors_;
-
   DISALLOW_COPY_AND_ASSIGN(MinidumpModuleCrashpadInfoListWriter);
 };
 

minidump/minidump_module_crashpad_info_writer_test.cc

@@ -20,6 +20,7 @@
 #include "minidump/minidump_extensions.h"
 #include "minidump/minidump_simple_string_dictionary_writer.h"
 #include "minidump/test/minidump_file_writer_test_util.h"
+#include "minidump/test/minidump_location_descriptor_list_test_util.h"
 #include "minidump/test/minidump_string_writer_test_util.h"
 #include "minidump/test/minidump_writable_test_util.h"
 #include "snapshot/test/test_module_snapshot.h"
@@ -29,17 +30,6 @@ namespace crashpad {
 namespace test {
 namespace {
 
-const MinidumpModuleCrashpadInfoList* MinidumpModuleCrashpadInfoListAtStart(
-    const std::string& file_contents,
-    size_t count) {
-  MINIDUMP_LOCATION_DESCRIPTOR location_descriptor;
-  location_descriptor.DataSize = sizeof(MinidumpModuleCrashpadInfoList) +
-                                 count * sizeof(MINIDUMP_LOCATION_DESCRIPTOR);
-  location_descriptor.Rva = 0;
-  return MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfoList>(
-      file_contents, location_descriptor);
-}
-
 TEST(MinidumpModuleCrashpadInfoWriter, EmptyList) {
   StringFileWriter file_writer;
 
@@ -50,10 +40,8 @@ TEST(MinidumpModuleCrashpadInfoWriter, EmptyList) {
             file_writer.string().size());
 
   const MinidumpModuleCrashpadInfoList* module_list =
-      MinidumpModuleCrashpadInfoListAtStart(file_writer.string(), 0);
+      MinidumpLocationDescriptorListAtStart(file_writer.string(), 0);
   ASSERT_TRUE(module_list);
-
-  EXPECT_EQ(0u, module_list->count);
 }
 
 TEST(MinidumpModuleCrashpadInfoWriter, EmptyModule) {
@@ -72,14 +60,12 @@ TEST(MinidumpModuleCrashpadInfoWriter, EmptyModule) {
             file_writer.string().size());
 
   const MinidumpModuleCrashpadInfoList* module_list =
-      MinidumpModuleCrashpadInfoListAtStart(file_writer.string(), 1);
+      MinidumpLocationDescriptorListAtStart(file_writer.string(), 1);
   ASSERT_TRUE(module_list);
 
-  ASSERT_EQ(1u, module_list->count);
-
   const MinidumpModuleCrashpadInfo* module =
       MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfo>(
-          file_writer.string(), module_list->modules[0]);
+          file_writer.string(), module_list->children[0]);
   ASSERT_TRUE(module);
 
   EXPECT_EQ(MinidumpModuleCrashpadInfo::kVersion, module->version);
@@ -122,14 +108,12 @@ TEST(MinidumpModuleCrashpadInfoWriter, FullModule) {
             file_writer.string().size());
 
   const MinidumpModuleCrashpadInfoList* module_list =
-      MinidumpModuleCrashpadInfoListAtStart(file_writer.string(), 1);
+      MinidumpLocationDescriptorListAtStart(file_writer.string(), 1);
   ASSERT_TRUE(module_list);
 
-  ASSERT_EQ(1u, module_list->count);
-
   const MinidumpModuleCrashpadInfo* module =
       MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfo>(
-          file_writer.string(), module_list->modules[0]);
+          file_writer.string(), module_list->children[0]);
   ASSERT_TRUE(module);
 
   EXPECT_EQ(MinidumpModuleCrashpadInfo::kVersion, module->version);
@@ -210,14 +194,12 @@ TEST(MinidumpModuleCrashpadInfoWriter, ThreeModules) {
   EXPECT_TRUE(module_list_writer.WriteEverything(&file_writer));
 
   const MinidumpModuleCrashpadInfoList* module_list =
-      MinidumpModuleCrashpadInfoListAtStart(file_writer.string(), 3);
+      MinidumpLocationDescriptorListAtStart(file_writer.string(), 3);
   ASSERT_TRUE(module_list);
 
-  ASSERT_EQ(3u, module_list->count);
-
   const MinidumpModuleCrashpadInfo* module_0 =
       MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfo>(
-          file_writer.string(), module_list->modules[0]);
+          file_writer.string(), module_list->children[0]);
   ASSERT_TRUE(module_0);
 
   EXPECT_EQ(MinidumpModuleCrashpadInfo::kVersion, module_0->version);
@@ -238,7 +220,7 @@ TEST(MinidumpModuleCrashpadInfoWriter, ThreeModules) {
 
   const MinidumpModuleCrashpadInfo* module_1 =
       MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfo>(
-          file_writer.string(), module_list->modules[1]);
+          file_writer.string(), module_list->children[1]);
   ASSERT_TRUE(module_1);
 
   EXPECT_EQ(MinidumpModuleCrashpadInfo::kVersion, module_1->version);
@@ -251,7 +233,7 @@ TEST(MinidumpModuleCrashpadInfoWriter, ThreeModules) {
 
   const MinidumpModuleCrashpadInfo* module_2 =
       MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfo>(
-          file_writer.string(), module_list->modules[2]);
+          file_writer.string(), module_list->children[2]);
   ASSERT_TRUE(module_2);
 
   EXPECT_EQ(MinidumpModuleCrashpadInfo::kVersion, module_2->version);
@@ -312,14 +294,12 @@ TEST(MinidumpModuleCrashpadInfoWriter, InitializeFromSnapshot) {
   ASSERT_TRUE(module_list_writer.WriteEverything(&file_writer));
 
   const MinidumpModuleCrashpadInfoList* module_list =
-      MinidumpModuleCrashpadInfoListAtStart(file_writer.string(), 2);
+      MinidumpLocationDescriptorListAtStart(file_writer.string(), 2);
   ASSERT_TRUE(module_list);
 
-  ASSERT_EQ(2u, module_list->count);
-
   const MinidumpModuleCrashpadInfo* module_0 =
       MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfo>(
-          file_writer.string(), module_list->modules[0]);
+          file_writer.string(), module_list->children[0]);
   ASSERT_TRUE(module_0);
 
   EXPECT_EQ(MinidumpModuleCrashpadInfo::kVersion, module_0->version);
@@ -346,7 +326,7 @@ TEST(MinidumpModuleCrashpadInfoWriter, InitializeFromSnapshot) {
 
   const MinidumpModuleCrashpadInfo* module_2 =
       MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfo>(
-          file_writer.string(), module_list->modules[1]);
+          file_writer.string(), module_list->children[1]);
   ASSERT_TRUE(module_2);
 
   EXPECT_EQ(MinidumpModuleCrashpadInfo::kVersion, module_2->version);

minidump/minidump_writable.cc

@@ -31,6 +31,9 @@ const size_t kMaximumAlignment = 16;
 namespace crashpad {
 namespace internal {
 
+MinidumpWritable::~MinidumpWritable() {
+}
+
 bool MinidumpWritable::WriteEverything(FileWriterInterface* file_writer) {
   DCHECK_EQ(state_, kStateMutable);
 
@@ -89,9 +92,6 @@ MinidumpWritable::MinidumpWritable()
       state_(kStateMutable) {
 }
 
-MinidumpWritable::~MinidumpWritable() {
-}
-
 bool MinidumpWritable::Freeze() {
   DCHECK_EQ(state_, kStateMutable);
   state_ = kStateFrozen;

minidump/minidump_writable.h

@@ -32,6 +32,8 @@ namespace internal {
 //!     file.
 class MinidumpWritable {
  public:
+  virtual ~MinidumpWritable();
+
   //! \brief Writes an object and all of its children to a minidump file.
   //!
   //! Use this on the root object of a tree of MinidumpWritable objects,
@@ -134,13 +136,6 @@ class MinidumpWritable {
 
   MinidumpWritable();
 
-  // This doesn’t really need to be virtual because nothing ever deletes a
-  // MinidumpWritable* through an interface pointer with that type, and this is
-  // guaranteed by being protected. Regardless, the style guide is somewhat
-  // insistent.
-  // http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance
-  virtual ~MinidumpWritable();
-
   //! \brief The state of the object.
   State state() const { return state_; }
 

minidump/test/minidump_location_descriptor_list_test_util.cc

@@ -0,0 +1,48 @@
+// Copyright 2014 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "minidump/test/minidump_location_descriptor_list_test_util.h"
+
+#include <dbghelp.h>
+
+#include "minidump/minidump_extensions.h"
+#include "minidump/test/minidump_writable_test_util.h"
+
+namespace crashpad {
+namespace test {
+
+const MinidumpLocationDescriptorList* MinidumpLocationDescriptorListAtStart(
+    const std::string& file_contents, size_t count) {
+  MINIDUMP_LOCATION_DESCRIPTOR location_descriptor;
+  location_descriptor.DataSize = sizeof(MinidumpLocationDescriptorList) +
+                                 count * sizeof(MINIDUMP_LOCATION_DESCRIPTOR);
+  location_descriptor.Rva = 0;
+
+  const MinidumpLocationDescriptorList* list =
+      MinidumpWritableAtLocationDescriptor<MinidumpLocationDescriptorList>(
+          file_contents, location_descriptor);
+  if (!list) {
+    return nullptr;
+  }
+
+  if (list->count != count) {
+    EXPECT_EQ(count, list->count);
+    return nullptr;
+  }
+
+  return list;
+}
+
+}  // namespace test
+}  // namespace crashpad

minidump/test/minidump_location_descriptor_list_test_util.h

@@ -0,0 +1,45 @@
+// Copyright 2014 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef CRASHPAD_MINIDUMP_TEST_MINIDUMP_LOCATION_DESCRIPTOR_LIST_TEST_UTIL_H_
+#define CRASHPAD_MINIDUMP_TEST_MINIDUMP_LOCATION_DESCRIPTOR_LIST_TEST_UTIL_H_
+
+#include <sys/types.h>
+
+#include <string>
+
+namespace crashpad {
+
+struct MinidumpLocationDescriptorList;
+
+namespace test {
+
+//! \brief Returns the MinidumpLocationDescriptorList at the start of a minidump
+//!     file.
+//!
+//! \param[in] file_contents The contents of the minidump file.
+//! \param[in] count The number of MINIDUMP_LOCATION_DESCRIPTOR objects expected
+//!     in the MinidumpLocationDescriptorList. This function will only be
+//!     successful if exactly this many objects are present, and if space for
+//!     them exists in \a file_contents.
+//!
+//! \return On success, the MinidumpLocationDescriptorList at the beginning of
+//!     the file. On failure, raises a gtest assertion and returns `nullptr`.
+const MinidumpLocationDescriptorList* MinidumpLocationDescriptorListAtStart(
+    const std::string& file_contents, size_t count);
+
+}  // namespace test
+}  // namespace crashpad
+
+#endif  // CRASHPAD_MINIDUMP_TEST_MINIDUMP_LOCATION_DESCRIPTOR_LIST_TEST_UTIL_H_

minidump/test/minidump_writable_test_util.cc

@@ -178,8 +178,8 @@ struct MinidumpThreadListTraits {
   }
 };
 
-struct MinidumpModuleCrashpadInfoListTraits {
+struct MinidumpLocationDescriptorListTraits {
-  using ListType = MinidumpModuleCrashpadInfoList;
+  using ListType = MinidumpLocationDescriptorList;
   static constexpr size_t kElementSize = sizeof(MINIDUMP_LOCATION_DESCRIPTOR);
   static size_t ElementCount(const ListType* list) {
     return list->count;
@@ -243,11 +243,11 @@ const MINIDUMP_THREAD_LIST* MinidumpWritableAtLocationDescriptor<
 }
 
 template <>
-const MinidumpModuleCrashpadInfoList*
+const MinidumpLocationDescriptorList*
-MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfoList>(
+MinidumpWritableAtLocationDescriptor<MinidumpLocationDescriptorList>(
     const std::string& file_contents,
     const MINIDUMP_LOCATION_DESCRIPTOR& location) {
-  return MinidumpListAtLocationDescriptor<MinidumpModuleCrashpadInfoListTraits>(
+  return MinidumpListAtLocationDescriptor<MinidumpLocationDescriptorListTraits>(
       file_contents, location);
 }
 

minidump/test/minidump_writable_test_util.h

@@ -83,7 +83,7 @@ MINIDUMP_ALLOW_OVERSIZED_DATA(MINIDUMP_DIRECTORY);
 MINIDUMP_ALLOW_OVERSIZED_DATA(MINIDUMP_MEMORY_LIST);
 MINIDUMP_ALLOW_OVERSIZED_DATA(MINIDUMP_MODULE_LIST);
 MINIDUMP_ALLOW_OVERSIZED_DATA(MINIDUMP_THREAD_LIST);
-MINIDUMP_ALLOW_OVERSIZED_DATA(MinidumpModuleCrashpadInfoList);
+MINIDUMP_ALLOW_OVERSIZED_DATA(MinidumpLocationDescriptorList);
 MINIDUMP_ALLOW_OVERSIZED_DATA(MinidumpSimpleStringDictionary);
 
 // These types have final fields carrying variable-sized data (typically string
@@ -182,8 +182,8 @@ const MINIDUMP_THREAD_LIST* MinidumpWritableAtLocationDescriptor<
                           const MINIDUMP_LOCATION_DESCRIPTOR& location);
 
 template <>
-const MinidumpModuleCrashpadInfoList*
+const MinidumpLocationDescriptorList*
-MinidumpWritableAtLocationDescriptor<MinidumpModuleCrashpadInfoList>(
+MinidumpWritableAtLocationDescriptor<MinidumpLocationDescriptorList>(
     const std::string& file_contents,
     const MINIDUMP_LOCATION_DESCRIPTOR& location);