[Zucchini] Zucchini-gen: Make number of CreateEquivalenceMap() generations depend on Disassembler.

The number of CreateEquivalenceMap() iterations used be constant
kNumIteraitons = 2. This CL makes the value depend on architecture.
Current assignment:
- DisassemblerNoOp: 1, since no pointers are identified (though in this
  case, CreateEquivalenceMap() should not be called).
- DisassemblerWin32: 2.
Upcoming DisassemblerDex will use 4.

Also applying generic cleanups on headers and comments.

Bug: 729154
Change-Id: Ia12d98fcba500e4c81c8a5d356ce4cadf424ffde
Reviewed-on: https://chromium-review.googlesource.com/961273
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542919}
NOKEYCHECK=True
GitOrigin-RevId: 55aea0a875b80e614464fdd157d9717471f9d64f

12 files changed, 40 insertions, 37 deletions

diff --git a/disassembler.cc b/disassembler.cc
index 18527a7..012e5ef 100644
--- a/disassembler.cc
+++ b/disassembler.cc
@@ -8,6 +8,8 @@
 
 namespace zucchini {
 
+/******** ReferenceGroup ********/
+
 std::unique_ptr<ReferenceReader> ReferenceGroup::GetReader(
     offset_t lower,
     offset_t upper,
@@ -25,12 +27,16 @@ std::unique_ptr<ReferenceReader> ReferenceGroup::GetReader(
 std::unique_ptr<ReferenceWriter> ReferenceGroup::GetWriter(
     MutableBufferView image,
     Disassembler* disasm) const {
-  DCHECK_EQ(image.begin(), disasm->GetImage().begin());
+  DCHECK_EQ(image.begin(), disasm->image().begin());
   DCHECK_EQ(image.size(), disasm->size());
   return (disasm->*writer_factory_)(image);
 }
 
-Disassembler::Disassembler() = default;
+/******** Disassembler ********/
+
+Disassembler::Disassembler(int num_equivalence_iterations)
+    : num_equivalence_iterations_(num_equivalence_iterations) {}
+
 Disassembler::~Disassembler() = default;
 
 }  // namespace zucchini
diff --git a/disassembler.h b/disassembler.h
index 8d41eaa..eced3cf 100644
--- a/disassembler.h
+++ b/disassembler.h
@@ -111,11 +111,13 @@ class Disassembler {
   // Groups must be aggregated by pool.
   virtual std::vector<ReferenceGroup> MakeReferenceGroups() const = 0;
 
-  ConstBufferView GetImage() const { return image_; }
+  ConstBufferView image() const { return image_; }
   size_t size() const { return image_.size(); }
 
+  int num_equivalence_iterations() const { return num_equivalence_iterations_; }
+
  protected:
-  Disassembler();
+  explicit Disassembler(int num_equivalence_iterations);
 
   // Parses |image| and initializes internal states. Returns true on success.
   // This must be called once and before any other operation.
@@ -125,6 +127,10 @@ class Disassembler {
   // only the portion containing the executable file it recognizes.
   ConstBufferView image_;
 
+  // The number of iterations to run for equivalence map generation. This should
+  // roughly be the max length of reference indirection chains.
+  int num_equivalence_iterations_;
+
   DISALLOW_COPY_AND_ASSIGN(Disassembler);
 };
 
diff --git a/disassembler_no_op.cc b/disassembler_no_op.cc
index 69d0eb8..3bc24d0 100644
--- a/disassembler_no_op.cc
+++ b/disassembler_no_op.cc
@@ -6,7 +6,9 @@
 
 namespace zucchini {
 
-DisassemblerNoOp::DisassemblerNoOp() = default;
+// |num_equivalence_iterations_| = 1 since no pointers are present.
+DisassemblerNoOp::DisassemblerNoOp() : Disassembler(1) {}
+
 DisassemblerNoOp::~DisassemblerNoOp() = default;
 
 ExecutableType DisassemblerNoOp::GetExeType() const {
diff --git a/disassembler_win32.cc b/disassembler_win32.cc
index 5bdc503..a932bea 100644
--- a/disassembler_win32.cc
+++ b/disassembler_win32.cc
@@ -86,8 +86,9 @@ bool DisassemblerWin32<Traits>::QuickDetect(ConstBufferView image) {
   return ReadWin32Header<Traits>(image, &source);
 }
 
+// |num_equivalence_iterations_| = 2 for reloc -> abs32.
 template <class Traits>
-DisassemblerWin32<Traits>::DisassemblerWin32() = default;
+DisassemblerWin32<Traits>::DisassemblerWin32() : Disassembler(2) {}
 
 template <class Traits>
 DisassemblerWin32<Traits>::~DisassemblerWin32() = default;
@@ -309,9 +310,6 @@ bool DisassemblerWin32<Traits>::ParseAndStoreRelocBlocks() {
                                               &reloc_block_offsets_);
 }
 
-// TODO(huangs): Print warning if too few abs32 references are found.
-// Empirically, file size / # relocs is < 100, so take 200 as the
-// threshold for warning.
 template <class Traits>
 bool DisassemblerWin32<Traits>::ParseAndStoreAbs32() {
   if (has_parsed_abs32_)
diff --git a/image_index.h b/image_index.h
index 4f07015..6301b3a 100644
--- a/image_index.h
+++ b/image_index.h
@@ -43,14 +43,14 @@ class ImageIndex {
   size_t TypeCount() const {
     if (reference_sets_.empty())
       return 0U;
-    return reference_sets_.rbegin()->first.value() + 1;
+    return reference_sets_.rbegin()->first.value() + 1;  // Max key + 1.
   }
 
   // Returns the array size needed to accommodate all pool values.
   size_t PoolCount() const {
     if (target_pools_.empty())
       return 0U;
-    return target_pools_.rbegin()->first.value() + 1;
+    return target_pools_.rbegin()->first.value() + 1;  // Max key + 1.
   }
 
   // Returns true if |image_[location]| is either:
diff --git a/main_utils.cc b/main_utils.cc
index b874dd0..ee0332a 100644
--- a/main_utils.cc
+++ b/main_utils.cc
@@ -25,9 +25,6 @@
 
 namespace {
 
-#if defined(OS_WIN)
-#endif
-
 /******** Command ********/
 
 // Specifications for a Zucchini command.
diff --git a/rel32_utils_unittest.cc b/rel32_utils_unittest.cc
index 80928de..6d90112 100644
--- a/rel32_utils_unittest.cc
+++ b/rel32_utils_unittest.cc
@@ -9,7 +9,6 @@
 #include <memory>
 #include <vector>
 
-#include "base/memory/ptr_util.h"
 #include "base/optional.h"
 #include "base/test/gtest_util.h"
 #include "components/zucchini/address_translator.h"
diff --git a/test_disassembler.cc b/test_disassembler.cc
index 8d59a93..2d6727b 100644
--- a/test_disassembler.cc
+++ b/test_disassembler.cc
@@ -8,13 +8,16 @@
 
 namespace zucchini {
 
+// |num_equivalence_iterations_| = 2 to cover common case for testing.
 TestDisassembler::TestDisassembler(const ReferenceTypeTraits& traits1,
                                    const std::vector<Reference>& refs1,
                                    const ReferenceTypeTraits& traits2,
                                    const std::vector<Reference>& refs2,
                                    const ReferenceTypeTraits& traits3,
                                    const std::vector<Reference>& refs3)
-    : traits_{traits1, traits2, traits3}, refs_{refs1, refs2, refs3} {}
+    : Disassembler(2),
+      traits_{traits1, traits2, traits3},
+      refs_{refs1, refs2, refs3} {}
 
 TestDisassembler::~TestDisassembler() = default;
 
diff --git a/zucchini_apply_unittest.cc b/zucchini_apply_unittest.cc
index 7e26b7b..f1cb853 100644
--- a/zucchini_apply_unittest.cc
+++ b/zucchini_apply_unittest.cc
@@ -4,19 +4,11 @@
 
 #include "components/zucchini/zucchini_apply.h"
 
-#include <vector>
-
 #include "components/zucchini/image_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace zucchini {
 
-namespace {
-
-using OffsetVector = std::vector<offset_t>;
-
-}  // namespace
-
-// TODO(huangs): Add more tests.
+// TODO(huangs): Add tests.
 
 }  // namespace zucchini
diff --git a/zucchini_commands.cc b/zucchini_commands.cc
index 60b87cb..90208a0 100644
--- a/zucchini_commands.cc
+++ b/zucchini_commands.cc
@@ -7,7 +7,6 @@
 #include <stddef.h>
 #include <stdint.h>
 
-#include <iostream>
 #include <ostream>
 #include <utility>
 
diff --git a/zucchini_gen.cc b/zucchini_gen.cc
index 4be0b8b..0758e5a 100644
--- a/zucchini_gen.cc
+++ b/zucchini_gen.cc
@@ -33,7 +33,6 @@ namespace {
 // Parameters for patch generation.
 constexpr double kMinEquivalenceSimilarity = 12.0;
 constexpr double kMinLabelAffinity = 64.0;
-constexpr size_t kNumIterations = 2;
 
 }  // namespace
 
@@ -46,21 +45,21 @@ std::vector<offset_t> FindExtraTargets(const TargetPool& projected_old_targets,
   return extra_targets;
 }
 
+// Label matching (between "old" and "new") can guide EquivalenceMap
+// construction; but EquivalenceMap induces Label matching. This apparent "chick
+// and egg" problem is solved by alternating 2 steps |num_iterations| times:
+// - Associate targets based on previous EquivalenceMap. Note on the first
+//   iteration, EquivalenceMap is empty, resulting in a no-op.
+// - Construct refined EquivalenceMap based on new targets associations.
 EquivalenceMap CreateEquivalenceMap(const ImageIndex& old_image_index,
-                                    const ImageIndex& new_image_index) {
-  // Label matching (between "old" and "new") can guide EquivalenceMap
-  // construction; but EquivalenceMap induces Label matching. This apparent
-  // "chick and egg" problem is solved by multiple iterations alternating 2
-  // steps:
-  // - Association of targets based on previous EquivalenceMap. Note that the
-  //   EquivalenceMap is empty on first iteration, so this is a no-op.
-  // - Construction of refined EquivalenceMap based on new targets associations.
+                                    const ImageIndex& new_image_index,
+                                    int num_iterations) {
   size_t pool_count = old_image_index.PoolCount();
   // |target_affinities| is outside the loop to reduce allocation.
   std::vector<TargetsAffinity> target_affinities(pool_count);
 
   EquivalenceMap equivalence_map;
-  for (size_t i = 0; i < kNumIterations; ++i) {
+  for (int i = 0; i < num_iterations; ++i) {
     EncodedView old_view(old_image_index);
     EncodedView new_view(new_image_index);
 
@@ -259,7 +258,8 @@ bool GenerateExecutableElement(ExecutableType exe_type,
   DCHECK_EQ(old_image_index.PoolCount(), new_image_index.PoolCount());
 
   EquivalenceMap equivalences =
-      CreateEquivalenceMap(old_image_index, new_image_index);
+      CreateEquivalenceMap(old_image_index, new_image_index,
+                           new_disasm->num_equivalence_iterations());
   OffsetMapper offset_mapper(equivalences);
 
   ReferenceDeltaSink reference_delta_sink;
diff --git a/zucchini_tools.cc b/zucchini_tools.cc
index 784e355..16eff2d 100644
--- a/zucchini_tools.cc
+++ b/zucchini_tools.cc
@@ -13,6 +13,7 @@
 #include <string>
 
 #include "base/bind.h"
+#include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "components/zucchini/disassembler.h"
 #include "components/zucchini/element_detection.h"