[Zucchini] Rel32Finder: Make rel32 accept / reject semantics explicit.

Zucchini heuristically scans assembly code byte-by-byte for rel32
references. When found, the result needs validation, and on X86 / X64,
this directs where to scan next: If accepted, scan resumes after the
instruction containing the rel32 found; if rejected, scan resumes on
the next byte.

Rel32Finder implements the above interactively: GetNext() emits the
next candidate rel32, and the caller needs to call Accept() to signal
acceptance (else rejection is assumed).

Inherited classes of Rel32Finder implements architecture-specific code
via Scan(), which caches results. Previously, Scan() also returns a
range for the instruction found. If accepted, scan resumes after the
range; if rejected, scan resumes 1 byte after the start of range.

Problem: The "scan 1 byte after" scheme works well for X86 / X64 and
fixed-size instructions (by aligning in Scan()). However, for THUMB2
instructions in ARM, which has easily discernible 2-byte and 4-byte
op codes, for both "accept" and "reject", scan should resume on the
next instruction.

This CL refactors Rel32Finder to solve the above, with other cleanup.
Details:
* Change Scan() to return (new struct) NextIterators, which stores
  iterator for "accept" and "reject" cases.
* Rename Reset() to SetRegion() to assign |region_|, and remove the
  |region_| assignment via constructor.
* Add Rel32FinderIntel::SetResult().
* Move more code from .h to .cc.
* Rename |next_cursor_| to |accept_it_|.
* Extensive comment updates.

Bug: 943315,918867
Change-Id: Ie0a0b380975c35b0aedb013037f8d69673c9697c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1529166
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Commit-Queue: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#643098}
NOKEYCHECK=True
GitOrigin-RevId: 47fef62aa7626d9a47fc1986b8e51e6e866570d1

5 files changed, 147 insertions, 126 deletions

diff --git a/disassembler_elf.cc b/disassembler_elf.cc
index ef050a4..474f43e 100644
--- a/disassembler_elf.cc
+++ b/disassembler_elf.cc
@@ -371,13 +371,12 @@ void DisassemblerElfIntel<Traits>::ParseExecSection(
 
   ConstBufferView region(image_.begin() + section.sh_offset, section.sh_size);
   Abs32GapFinder gap_finder(image_, region, abs32_locations_, 4);
-  std::unique_ptr<Rel32FinderIntel> finder =
-      std::make_unique<typename Traits::Rel32FinderUse>(image_);
+  typename Traits::Rel32FinderUse finder;
   for (auto gap = gap_finder.GetNext(); gap.has_value();
        gap = gap_finder.GetNext()) {
-    finder->Reset(gap.value());
-    for (auto rel32 = finder->GetNext(); rel32.has_value();
-         rel32 = finder->GetNext()) {
+    finder.SetRegion(gap.value());
+    for (auto rel32 = finder.GetNext(); rel32.has_value();
+         rel32 = finder.GetNext()) {
       offset_t rel32_offset =
           base::checked_cast<offset_t>(rel32->location - image_.begin());
       rva_t rel32_rva = rva_t(rel32_offset + from_offset_to_rva);
@@ -385,7 +384,7 @@ void DisassemblerElfIntel<Traits>::ParseExecSection(
       if (target_rva_checker.IsValid(target_rva) &&
           (rel32->can_point_outside_section ||
            (start_rva <= target_rva && target_rva < end_rva))) {
-        finder->Accept();
+        finder.Accept();
         rel32_locations_.push_back(rel32_offset);
       }
     }
diff --git a/disassembler_win32.cc b/disassembler_win32.cc
index 01c4fde..645ea07 100644
--- a/disassembler_win32.cc
+++ b/disassembler_win32.cc
@@ -377,11 +377,11 @@ bool DisassemblerWin32<Traits>::ParseAndStoreRel32() {
         image_[{section.file_offset_of_raw_data, section.size_of_raw_data}];
     Abs32GapFinder gap_finder(image_, region, abs32_locations_,
                               Traits::kVAWidth);
-    typename Traits::RelFinder finder(image_);
+    typename Traits::RelFinder finder;
     // Iterate over gaps between abs32 references, to avoid collision.
     for (auto gap = gap_finder.GetNext(); gap.has_value();
          gap = gap_finder.GetNext()) {
-      finder.Reset(gap.value());
+      finder.SetRegion(gap.value());
       // Iterate over heuristically detected rel32 references, validate, and add
       // to |rel32_locations_|.
       for (auto rel32 = finder.GetNext(); rel32.has_value();
diff --git a/rel32_finder.cc b/rel32_finder.cc
index f010aaf..8fb88f5 100644
--- a/rel32_finder.cc
+++ b/rel32_finder.cc
@@ -63,52 +63,74 @@ base::Optional<ConstBufferView> Abs32GapFinder::GetNext() {
 
 /******** Rel32Finder ********/
 
-Rel32Finder::Rel32Finder() = default;
-
-Rel32Finder::Rel32Finder(ConstBufferView region)
-    : region_(region), next_cursor_(region_.begin()) {}
+Rel32Finder::Rel32Finder() {}
 
 Rel32Finder::~Rel32Finder() = default;
 
+void Rel32Finder::SetRegion(ConstBufferView region) {
+  region_ = region;
+  accept_it_ = region.begin();
+}
+
+bool Rel32Finder::FindNext() {
+  NextIterators next_iters = Scan(region_);
+  if (next_iters.reject == nullptr) {
+    region_.seek(region_.end());
+    return false;
+  }
+  region_.seek(next_iters.reject);
+  accept_it_ = next_iters.accept;
+  DCHECK_GE(accept_it_, region_.begin());
+  DCHECK_LE(accept_it_, region_.end());
+  return true;
+}
+
+void Rel32Finder::Accept() {
+  region_.seek(accept_it_);
+}
+
+/******** Rel32FinderIntel ********/
+
+Rel32Finder::NextIterators Rel32FinderIntel::SetResult(
+    ConstBufferView::const_iterator cursor,
+    uint32_t opcode_size,
+    bool can_point_outside_section) {
+  rel32_ = {cursor + opcode_size, can_point_outside_section};
+  return {cursor + 1, cursor + (opcode_size + 4)};
+}
+
 /******** Rel32FinderX86 ********/
 
-ConstBufferView Rel32FinderX86::Scan(ConstBufferView region) {
+Rel32Finder::NextIterators Rel32FinderX86::Scan(ConstBufferView region) {
   ConstBufferView::const_iterator cursor = region.begin();
   while (cursor < region.end()) {
     // Heuristic rel32 detection by looking for opcodes that use them.
     if (cursor + 5 <= region.end()) {
-      if (cursor[0] == 0xE8 || cursor[0] == 0xE9) {  // JMP rel32; CALL rel32
-        rel32_ = {cursor + 1, false};
-        return ConstBufferView::FromRange(cursor, rel32_.location + 4);
-      }
+      if (cursor[0] == 0xE8 || cursor[0] == 0xE9)  // JMP rel32; CALL rel32
+        return SetResult(cursor, 1, false);
     }
     if (cursor + 6 <= region.end()) {
-      if (cursor[0] == 0x0F && (cursor[1] & 0xF0) == 0x80) {  // Jcc long form
-        rel32_ = {cursor + 2, false};
-        return ConstBufferView::FromRange(cursor, rel32_.location + 4);
-      }
+      if (cursor[0] == 0x0F && (cursor[1] & 0xF0) == 0x80)  // Jcc long form
+        return SetResult(cursor, 2, false);
     }
     ++cursor;
   }
-  return {region.end(), 0};
+  return {nullptr, nullptr};
 }
 
 /******** Rel32FinderX64 ********/
 
-ConstBufferView Rel32FinderX64::Scan(ConstBufferView region) {
+Rel32Finder::NextIterators Rel32FinderX64::Scan(ConstBufferView region) {
   ConstBufferView::const_iterator cursor = region.begin();
   while (cursor < region.end()) {
     // Heuristic rel32 detection by looking for opcodes that use them.
     if (cursor + 5 <= region.end()) {
-      if (cursor[0] == 0xE8 || cursor[0] == 0xE9) {  // JMP rel32; CALL rel32
-        rel32_ = {cursor + 1, false};
-        return ConstBufferView::FromRange(cursor, rel32_.location + 4);
-      }
+      if (cursor[0] == 0xE8 || cursor[0] == 0xE9)  // JMP rel32; CALL rel32
+        return SetResult(cursor, 1, false);
     }
     if (cursor + 6 <= region.end()) {
       if (cursor[0] == 0x0F && (cursor[1] & 0xF0) == 0x80) {  // Jcc long form
-        rel32_ = {cursor + 2, false};
-        return ConstBufferView::FromRange(cursor, rel32_.location + 4);
+        return SetResult(cursor, 2, false);
       } else if ((cursor[0] == 0xFF &&
                   (cursor[1] == 0x15 || cursor[1] == 0x25)) ||
                  ((cursor[0] == 0x89 || cursor[0] == 0x8B ||
@@ -128,13 +150,12 @@ ConstBufferView Rel32FinderX64::Scan(ConstBufferView region) {
         //  ModR/M : MMRRRMMM
         //   MM = 00 & MMM = 101 => rip+disp32
         //   RRR: selects reg operand from [eax|ecx|...|edi]
-        rel32_ = {cursor + 2, true};
-        return ConstBufferView::FromRange(cursor, rel32_.location + 4);
+        return SetResult(cursor, 2, true);
       }
     }
     ++cursor;
   }
-  return {region.end(), 0};
+  return {nullptr, nullptr};
 }
 
 }  // namespace zucchini
diff --git a/rel32_finder.h b/rel32_finder.h
index 798983e..a4691fd 100644
--- a/rel32_finder.h
+++ b/rel32_finder.h
@@ -17,36 +17,35 @@
 
 namespace zucchini {
 
-// See README.md for definitions on abs32 and rel32 references. We assume the
-// following:
-// - Abs32 locations have fixed lengths, and never overlap.
-// - Rel32 locations can be reasonably identified by heuristically disassembling
-//   machine code.
-// - Rel32 locations never overlap with each other, and never with abs32
-//   locations.
-
-// Abs32GapFinder is a class that iterates over all contiguous gaps in |region|
-// that lie outside of |abs32_locations| elements, each spanning |abs_width|
-// bytes. For example, given
-//   region = [base_ + 8, base_ + 25),
+// See README.md for definitions on abs32 and rel32 references. The following
+// are assumed:
+// * Abs32 reference bodies have fixed widths.
+// * Rel32 locations can be identified by heuristically disassembling machine
+//   code, and errors are tolerated.
+// * The collection all abs32 and rel32 reference bodies do not overlap.
+
+// A class to visit non-empty contiguous gaps in |region| that lie outside of
+// |abs32_locations| elements, each with a body that spans |abs32_width_| bytes.
+// For example, given:
+//   region = [base_ + 4, base_ + 26),
 //   abs32_locations = {2, 6, 15, 20, 27},
 //   abs32_width_ = 4,
-// we obtain the following:
+// the following is obtained:
 //             111111111122222222223   -> offsets
 //   0123456789012345678901234567890
-//   ........*****************......   -> region = *
+//   ....**********************.....   -> region = *
 //     ^   ^        ^    ^      ^      -> abs32 locations
-//     aaaaaaaa     aaaa aaaa   aaaa   -> abs32 locations with width
-//   ........--*****----*----*......   -> region excluding abs32 -> 3 gaps
-// The resulting gaps (must be non-empty) are:
-//   [10, 15), [19, 20), [24, 25).
+//     aaaaaaaa     aaaa aaaa   aaaa   -> abs32 bodies
+//   ....------*****----*----**.....   -> regions excluding abs32 -> 3 gaps
+// The resulting gaps (non-empty, so [6, 6) is excluded) are:
+//   [10, 15), [19, 20), [24, 26).
 // These gaps can then be passed to Rel32Finder (below) to find rel32 references
-// that are guaranteed to not overlap with any abs32 references.
+// with bodies that are guaranteed to not overlap with any abs32 bodies.
 class Abs32GapFinder {
  public:
-  // |abs32_locations| is a sorted list of non-overlapping abs32 reference
-  // locations in |image|, each spanning |abs32_width| bytes. Gaps are searched
-  // in |region|, which must be part of |image|.
+  // |abs32_locations| is a sorted list of non-overlapping abs32 locations in
+  // |image|, each spanning |abs32_width| bytes. Gaps are searched in |region|,
+  // which must be part of |image|.
   Abs32GapFinder(ConstBufferView image,
                  ConstBufferView region,
                  const std::vector<offset_t>& abs32_locations,
@@ -67,61 +66,54 @@ class Abs32GapFinder {
   DISALLOW_COPY_AND_ASSIGN(Abs32GapFinder);
 };
 
-// A class to parse executable bytes of an image to find rel32 locations.
-// Architecture-specific parse details are delegated to inherited classes.
-// This is typically used along with Abs32GapFinder to find search regions.
-// The caller may filter rel32 locations, based on rel32 targets.
+// A class to scan regions within an image to find successive rel32 references.
+// Architecture-specific parsing and result extraction are delegated to
+// inherited classes. This is typically used along with Abs32GapFinder to find
+// search regions.
 class Rel32Finder {
  public:
   Rel32Finder();
-  // |region| is the region being scanned for rel32 references.
-  explicit Rel32Finder(ConstBufferView region);
   virtual ~Rel32Finder();
 
-  // Reset object to start scanning for rel32 references in |region|.
-  void Reset(ConstBufferView region) {
-    next_cursor_ = region.begin();
-    region_ = region;
-  }
+  // Assigns the scan |region| for rel32 references to enable FindNext() use.
+  void SetRegion(ConstBufferView region);
 
-  // Accept the last reference found. Next call to FindNext() will scan starting
-  // beyond that reference, instead of the current search position.
-  void Accept() { region_.seek(next_cursor_); }
+  // When a rel32 reference is found, the caller needs to decide whether to keep
+  // the result (perhaps following more validation). If it decides to keep the
+  // result, then it must call Accept(), so the next call to FindNext() can skip
+  // the accepted rel32 reference.
+  void Accept();
 
   // Accessors for unittest.
-  ConstBufferView::const_iterator next_cursor() const { return next_cursor_; }
+  ConstBufferView::const_iterator accept_it() const { return accept_it_; }
   ConstBufferView region() const { return region_; }
 
  protected:
-  // Scans for the next rel32 reference. If a reference is found, advances the
-  // search position beyond it and returns true. Otherwise, moves the search
-  // position to the end of the region and returns false.
-  bool FindNext() {
-    ConstBufferView result = Scan(region_);
-    region_.seek(result.begin());
-    next_cursor_ = result.end();
-    if (region_.empty())
-      return false;
-    region_.remove_prefix(1);
-    DCHECK_GE(next_cursor_, region_.begin());
-    DCHECK_LE(next_cursor_, region_.end());
-    return true;
-  }
+  // Alternatives for where to continue the next scan when a rel32 reference is
+  // found. nulls indicate that no rel32 references remain.
+  struct NextIterators {
+    // The next iterator if the caller does not call Accept().
+    ConstBufferView::const_iterator reject;
+
+    // The next iterator if the caller calls Accept().
+    ConstBufferView::const_iterator accept;
+  };
 
-  // Architecture-specific rel32 reference detection, which scans executable
-  // bytes given by |region|. For each rel32 reference found, the implementation
-  // should cache the necessary data to be retrieved via accessors and return a
-  // region starting at the current search position, and ending beyond the
-  // reference that was just found, or an empty region starting at the end of
-  // the search region if no more reference is found. By default, the next time
-  // FindNext() is called, |region| will start at the current search position,
-  // unless Accept() was called, in which case |region| will start beyond the
-  // last reference.
-  virtual ConstBufferView Scan(ConstBufferView region) = 0;
+  // Scans for the next rel32 reference, and returns whether any is found, so a
+  // "while" loop can be used for iterative rel32 extraction. The results are
+  // cached in Rel32Finder_Impl and obtained by Rel32Finder_Impl::GetRel32().
+  bool FindNext();
+
+  // Detects and extracts architecture-specific rel32 reference. For each one
+  // found, the implementation should cache the necessary data to be retrieved
+  // via accessors. Returns a NextIterators that stores alternatives for where
+  // to continue the scan. If no rel32 reference is found then the returned
+  // NextIterators are nulls.
+  virtual NextIterators Scan(ConstBufferView region) = 0;
 
  private:
   ConstBufferView region_;
-  ConstBufferView::const_iterator next_cursor_ = nullptr;
+  ConstBufferView::const_iterator accept_it_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(Rel32Finder);
 };
@@ -150,11 +142,16 @@ class Rel32FinderIntel : public Rel32Finder {
   }
 
  protected:
+  // Helper for Scan() that also assigns |rel32_|.
+  Rel32Finder::NextIterators SetResult(ConstBufferView::const_iterator cursor,
+                                       uint32_t code_size,
+                                       bool can_point_outside_section);
+
   // Cached results.
   Result rel32_;
 
   // Rel32Finder:
-  ConstBufferView Scan(ConstBufferView region) override = 0;
+  NextIterators Scan(ConstBufferView region) override = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(Rel32FinderIntel);
@@ -167,7 +164,7 @@ class Rel32FinderX86 : public Rel32FinderIntel {
 
  private:
   // Rel32Finder:
-  ConstBufferView Scan(ConstBufferView region) override;
+  NextIterators Scan(ConstBufferView region) override;
 
   DISALLOW_COPY_AND_ASSIGN(Rel32FinderX86);
 };
@@ -179,7 +176,7 @@ class Rel32FinderX64 : public Rel32FinderIntel {
 
  private:
   // Rel32Finder:
-  ConstBufferView Scan(ConstBufferView region) override;
+  NextIterators Scan(ConstBufferView region) override;
 
   DISALLOW_COPY_AND_ASSIGN(Rel32FinderX64);
 };
diff --git a/rel32_finder_unittest.cc b/rel32_finder_unittest.cc
index 2da76ad..c32f814 100644
--- a/rel32_finder_unittest.cc
+++ b/rel32_finder_unittest.cc
@@ -122,12 +122,12 @@ class TestRel32Finder : public Rel32Finder {
  public:
   using Rel32Finder::Rel32Finder;
 
-  bool GetNext() { return Rel32Finder::FindNext(); }
-
   // Rel32Finder:
-  ConstBufferView Scan(ConstBufferView region) override { return next_result; }
+  NextIterators Scan(ConstBufferView region) override { return next_result; }
+
+  bool GetNext() { return FindNext(); }
 
-  ConstBufferView next_result;
+  NextIterators next_result;
 };
 
 }  // namespace
@@ -137,47 +137,48 @@ TEST(Rel32FinderTest, Scan) {
   std::vector<uint8_t> buffer(kRegionTotal);
   ConstBufferView image(buffer.data(), buffer.size());
 
-  TestRel32Finder finder(image);
+  TestRel32Finder finder;
+  finder.SetRegion(image);
 
   auto check_finder_state = [&](const TestRel32Finder& finder,
                                 size_t expected_cursor,
-                                size_t expected_next_cursor) {
+                                size_t expected_accept_it) {
     CHECK_LE(expected_cursor, kRegionTotal);
-    CHECK_LE(expected_next_cursor, kRegionTotal);
+    CHECK_LE(expected_accept_it, kRegionTotal);
 
     EXPECT_EQ(image.begin() + expected_cursor, finder.region().begin());
-    EXPECT_EQ(image.begin() + expected_next_cursor, finder.next_cursor());
+    EXPECT_EQ(image.begin() + expected_accept_it, finder.accept_it());
   };
 
   check_finder_state(finder, 0, 0);
 
-  finder.next_result = ConstBufferView(image.begin() + 0, 1);
+  finder.next_result = {image.begin() + 1, image.begin() + 1};
   EXPECT_TRUE(finder.GetNext());
   check_finder_state(finder, 1, 1);
 
-  finder.next_result = ConstBufferView(image.begin() + 1, 1);
+  finder.next_result = {image.begin() + 2, image.begin() + 2};
   EXPECT_TRUE(finder.GetNext());
   check_finder_state(finder, 2, 2);
 
-  finder.next_result = ConstBufferView(image.begin() + 4, 2);
+  finder.next_result = {image.begin() + 5, image.begin() + 6};
   EXPECT_TRUE(finder.GetNext());
   check_finder_state(finder, 5, 6);
   finder.Accept();
   check_finder_state(finder, 6, 6);
 
-  finder.next_result = ConstBufferView(image.begin() + 6, 1);
+  finder.next_result = {image.begin() + 7, image.begin() + 7};
   EXPECT_TRUE(finder.GetNext());
   check_finder_state(finder, 7, 7);
 
-  finder.next_result = ConstBufferView(image.begin() + 7, 1);
+  finder.next_result = {image.begin() + 8, image.begin() + 8};
   EXPECT_TRUE(finder.GetNext());
   check_finder_state(finder, 8, 8);
 
-  finder.next_result = ConstBufferView(image.begin() + 98, 1);
+  finder.next_result = {image.begin() + 99, image.begin() + 99};
   EXPECT_TRUE(finder.GetNext());
   check_finder_state(finder, 99, 99);
 
-  finder.next_result = ConstBufferView(image.end(), 0);
+  finder.next_result = {nullptr, nullptr};
   EXPECT_FALSE(finder.GetNext());
   check_finder_state(finder, 99, 99);
 }
@@ -211,9 +212,10 @@ TEST(Rel32FinderX86Test, FindNext) {
   ConstBufferView image =
       ConstBufferView::FromRange(std::begin(data), std::end(data));
 
-  Rel32FinderX86 rel_finder(image);
+  Rel32FinderX86 rel_finder;
+  rel_finder.SetRegion(image);
 
-  // List of expected locations as pairs of (cursor position, rel32 position).
+  // List of expected locations as pairs of {cursor offset, rel32 offset}.
   std::vector<std::pair<size_t, size_t>> expected_locations = {
       {0x04, 0x04}, {0x09, 0x09}, {0x0E, 0x0F}, {0x14, 0x15}, {0x1A, 0x1B},
       {0x20, 0x21}, {0x26, 0x27}, {0x2C, 0x2D}, {0x32, 0x33}, {0x38, 0x39},
@@ -228,7 +230,7 @@ TEST(Rel32FinderX86Test, FindNext) {
     EXPECT_EQ(location.first,
               size_t(rel_finder.region().begin() - image.begin()));
     EXPECT_EQ(location.second, size_t(result->location - image.begin()));
-    EXPECT_EQ(result->location + 4, rel_finder.next_cursor());
+    EXPECT_EQ(result->location + 4, rel_finder.accept_it());
     EXPECT_FALSE(result->can_point_outside_section);
     rel_finder.Accept();
   }
@@ -245,20 +247,21 @@ TEST(Rel32FinderX86Test, Accept) {
   ConstBufferView image =
       ConstBufferView::FromRange(std::begin(data), std::end(data));
 
-  auto next_location = [&](Rel32FinderX86& rel_finder) {
+  auto next_location = [&](Rel32FinderX86& rel_finder) -> size_t {
     auto result = rel_finder.GetNext();
     EXPECT_TRUE(result.has_value());
     return result->location - image.begin();
   };
 
-  Rel32FinderX86 rel_finder(image);
+  Rel32FinderX86 rel_finder;
+  rel_finder.SetRegion(image);
 
-  EXPECT_EQ(0x05, next_location(rel_finder));  // False positive.
+  EXPECT_EQ(0x05U, next_location(rel_finder));  // False positive.
   rel_finder.Accept();
   // False negative: shadowed by 0x05
   // EXPECT_EQ(0x06, next_location(rel_finder));
-  EXPECT_EQ(0x0A, next_location(rel_finder));  // False positive.
-  EXPECT_EQ(0x0B, next_location(rel_finder));  // Found if 0x0A is discarded.
+  EXPECT_EQ(0x0AU, next_location(rel_finder));  // False positive.
+  EXPECT_EQ(0x0BU, next_location(rel_finder));  // Found if 0x0A is discarded.
 }
 
 TEST(Rel32FinderX64Test, FindNext) {
@@ -308,9 +311,10 @@ TEST(Rel32FinderX64Test, FindNext) {
   ConstBufferView image =
       ConstBufferView::FromRange(std::begin(data), std::end(data));
 
-  Rel32FinderX64 rel_finder(image);
+  Rel32FinderX64 rel_finder;
+  rel_finder.SetRegion(image);
 
-  // Lists of expected locations as pairs of (cursor position, rel32 position).
+  // Lists of expected locations as pairs of {cursor offset, rel32 offset}.
   std::vector<std::pair<size_t, size_t>> expected_locations = {
       {0x04, 0x04}, {0x09, 0x09}, {0x0E, 0x0F}, {0x14, 0x15}, {0x1A, 0x1B},
       {0x20, 0x21}, {0x26, 0x27}, {0x2C, 0x2D}, {0x32, 0x33}, {0x38, 0x39},
@@ -323,25 +327,25 @@ TEST(Rel32FinderX64Test, FindNext) {
       {0xAF, 0xB0}, {0xB6, 0xB7}, {0xBD, 0xBE}, {0xC4, 0xC5}, {0xCB, 0xCC},
       {0xD2, 0xD3}, {0xD9, 0xDA}, {0xE0, 0xE1},
   };
+  // Jump instructions, which cannot point outside section.
   for (auto location : expected_locations) {
     auto result = rel_finder.GetNext();
     EXPECT_TRUE(result.has_value());
-
     EXPECT_EQ(location.first,
               size_t(rel_finder.region().begin() - image.begin()));
     EXPECT_EQ(location.second, size_t(result->location - image.begin()));
-    EXPECT_EQ(result->location + 4, rel_finder.next_cursor());
+    EXPECT_EQ(result->location + 4, rel_finder.accept_it());
     EXPECT_FALSE(result->can_point_outside_section);
     rel_finder.Accept();
   }
+  // PC-relative data access instructions, which can point outside section.
   for (auto location : expected_locations_rip) {
     auto result = rel_finder.GetNext();
     EXPECT_TRUE(result.has_value());
-
     EXPECT_EQ(location.first,
               size_t(rel_finder.region().begin() - image.begin()));
     EXPECT_EQ(location.second, size_t(result->location - image.begin()));
-    EXPECT_EQ(result->location + 4, rel_finder.next_cursor());
+    EXPECT_EQ(result->location + 4, rel_finder.accept_it());
     EXPECT_TRUE(result->can_point_outside_section);
     rel_finder.Accept();
   }