From c803763b7f4ee725717839b679cadeb1637b6be5 Mon Sep 17 00:00:00 2001
From: huangs <huangs@chromium.org>
Date: Thu, 19 May 2016 11:16:40 -0700
Subject: [PATCH] [Courgette] Using LabelManager to reduce Courgette-apply peak
 RAM by 25%.

AssemblyProgram previously allocates new Label instances as it parses
an executable and emits instructions. This CL replaces the flow by using
LabelManager to precompute Labels in one array. This allows us to reduce
Courgette-apply peak RAM by 25%, measured by "choke RAM until failure"
method. Details:
- We precompute Labels in AssemblyProgram::PrecomputeLabels(), which
  relies on RvaVisitor inherited classes for architecture-specific
  extraction of abs32 and rel32 targets.
- TrimLabel()'s complex post-processing flow is simplified using
  PrecomputeLabels(), which runs before main file parse.
  - This requires RemoveUnusedRel32Locations() to update rel32.
  - Deprecating C_TRIM_FAILED error message.
- Moving more common functionality to Disassembler, but duplicating
  some code for win32-x86 and win32-x64 to follow existing pattern.

BUG=613216

Review-Url: https://codereview.chromium.org/1935203002
Cr-Commit-Position: refs/heads/master@{#394815}
---
 courgette/adjustment_method_unittest.cc |  20 +++-
 courgette/assembly_program.cc           | 122 +++++++-----------------
 courgette/assembly_program.h            |  30 +++---
 courgette/courgette.h                   |   1 -
 courgette/disassembler.cc               |  31 ++++++
 courgette/disassembler.h                |  48 ++++++++++
 courgette/disassembler_elf_32.cc        |  98 ++++++++++++++-----
 courgette/disassembler_elf_32.h         |  38 ++++++--
 courgette/disassembler_elf_32_arm.cc    |   7 +-
 courgette/disassembler_elf_32_arm.h     |   2 +-
 courgette/disassembler_elf_32_x86.cc    |   4 +-
 courgette/disassembler_elf_32_x86.h     |   2 +-
 courgette/disassembler_win32_x64.cc     |  35 ++++++-
 courgette/disassembler_win32_x64.h      |   5 +
 courgette/disassembler_win32_x86.cc     |  32 ++++++-
 courgette/disassembler_win32_x86.h      |   5 +
 courgette/encoded_program.cc            |  76 +++++++++------
 courgette/encoded_program.h             |  15 ++-
 courgette/encoded_program_unittest.cc   |  69 +++++---------
 courgette/image_utils.h                 |   8 +-
 courgette/label_manager.cc              |  11 ---
 courgette/label_manager.h               |   4 -
 courgette/program_detector.cc           |   3 -
 courgette/rel32_finder_win32_x86.cc     |   3 +-
 24 files changed, 416 insertions(+), 253 deletions(-)

diff --git a/courgette/adjustment_method_unittest.cc b/courgette/adjustment_method_unittest.cc
index 3200f10c60994..d8b987c2d89e1 100644
--- a/courgette/adjustment_method_unittest.cc
+++ b/courgette/adjustment_method_unittest.cc
@@ -5,11 +5,13 @@
 #include <memory>
 #include <string>
 #include <utility>
+#include <vector>
 
 #include "base/strings/string_util.h"
 #include "courgette/assembly_program.h"
 #include "courgette/courgette.h"
 #include "courgette/encoded_program.h"
+#include "courgette/image_utils.h"
 #include "courgette/streams.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
@@ -32,8 +34,20 @@ class AdjustmentMethodTest : public testing::Test {
         new courgette::AssemblyProgram(courgette::EXE_WIN_32_X86));
     prog->set_image_base(0x00400000);
 
-    courgette::Label* labelA = prog->FindOrMakeAbs32Label(0x00410000);
-    courgette::Label* labelB = prog->FindOrMakeAbs32Label(0x00410004);
+    courgette::RVA kRvaA = 0x00410000;
+    courgette::RVA kRvaB = 0x00410004;
+
+    std::vector<courgette::RVA> abs32_rvas;
+    abs32_rvas.push_back(kRvaA);
+    abs32_rvas.push_back(kRvaB);
+    std::vector<courgette::RVA> rel32_rvas;  // Stub.
+
+    courgette::TrivialRvaVisitor abs32_visitor(abs32_rvas);
+    courgette::TrivialRvaVisitor rel32_visitor(rel32_rvas);
+    prog->PrecomputeLabels(&abs32_visitor, &rel32_visitor);
+
+    courgette::Label* labelA = prog->FindAbs32Label(kRvaA);
+    courgette::Label* labelB = prog->FindAbs32Label(kRvaB);
 
     EXPECT_TRUE(prog->EmitAbs32(labelA));
     EXPECT_TRUE(prog->EmitAbs32(labelA));
@@ -88,7 +102,6 @@ class AdjustmentMethodTest : public testing::Test {
   }
 };
 
-
 void AdjustmentMethodTest::Test1() const {
   std::unique_ptr<courgette::AssemblyProgram> prog1 = MakeProgramA();
   std::unique_ptr<courgette::AssemblyProgram> prog2 = MakeProgramB();
@@ -109,7 +122,6 @@ void AdjustmentMethodTest::Test1() const {
   EXPECT_TRUE(s5 == s6);  // Adjustment did change B into A
 }
 
-
 TEST_F(AdjustmentMethodTest, All) {
   Test1();
 }
diff --git a/courgette/assembly_program.cc b/courgette/assembly_program.cc
index 5344cff95f848..f20ed67ea3fa3 100644
--- a/courgette/assembly_program.cc
+++ b/courgette/assembly_program.cc
@@ -111,11 +111,6 @@ AssemblyProgram::AssemblyProgram(ExecutableType kind)
   : kind_(kind), image_base_(0) {
 }
 
-static void DeleteContainedLabels(const RVAToLabel& labels) {
-  for (RVAToLabel::const_iterator p = labels.begin();  p != labels.end();  ++p)
-    UncheckedDelete(p->second);
-}
-
 AssemblyProgram::~AssemblyProgram() {
   for (size_t i = 0;  i < instructions_.size();  ++i) {
     Instruction* instruction = instructions_[i];
@@ -126,8 +121,6 @@ AssemblyProgram::~AssemblyProgram() {
     for (size_t i = 0;  i < 256;  ++i)
       UncheckedDelete(byte_instruction_cache_[i]);
   }
-  DeleteContainedLabels(rel32_labels_);
-  DeleteContainedLabels(abs32_labels_);
 }
 
 CheckBool AssemblyProgram::EmitPeRelocsInstruction() {
@@ -178,27 +171,50 @@ CheckBool AssemblyProgram::EmitAbs64(Label* label) {
       ScopedInstruction(UncheckedNew<InstructionWithLabel>(ABS64, label)));
 }
 
-Label* AssemblyProgram::FindOrMakeAbs32Label(RVA rva) {
-  return FindLabel(rva, &abs32_labels_);
+void AssemblyProgram::PrecomputeLabels(RvaVisitor* abs32_visitor,
+                                       RvaVisitor* rel32_visitor) {
+  abs32_label_manager_.Read(abs32_visitor);
+  rel32_label_manager_.Read(rel32_visitor);
+  TrimLabels();
 }
 
-Label* AssemblyProgram::FindOrMakeRel32Label(RVA rva) {
-  return FindLabel(rva, &rel32_labels_);
-}
+// Chosen empirically to give the best reduction in payload size for
+// an update from daisy_3701.98.0 to daisy_4206.0.0.
+const int AssemblyProgram::kLabelLowerLimit = 5;
 
-void AssemblyProgram::DefaultAssignIndexes() {
-  DefaultAssignIndexes(&abs32_labels_);
-  DefaultAssignIndexes(&rel32_labels_);
+void AssemblyProgram::TrimLabels() {
+  // For now only trim for ARM binaries.
+  if (kind() != EXE_ELF_32_ARM)
+    return;
+
+  int lower_limit = kLabelLowerLimit;
+
+  VLOG(1) << "TrimLabels: threshold " << lower_limit;
+
+  rel32_label_manager_.RemoveUnderusedLabels(lower_limit);
 }
 
 void AssemblyProgram::UnassignIndexes() {
-  UnassignIndexes(&abs32_labels_);
-  UnassignIndexes(&rel32_labels_);
+  abs32_label_manager_.UnassignIndexes();
+  rel32_label_manager_.UnassignIndexes();
+}
+
+void AssemblyProgram::DefaultAssignIndexes() {
+  abs32_label_manager_.DefaultAssignIndexes();
+  rel32_label_manager_.DefaultAssignIndexes();
 }
 
 void AssemblyProgram::AssignRemainingIndexes() {
-  AssignRemainingIndexes(&abs32_labels_);
-  AssignRemainingIndexes(&rel32_labels_);
+  abs32_label_manager_.AssignRemainingIndexes();
+  rel32_label_manager_.AssignRemainingIndexes();
+}
+
+Label* AssemblyProgram::FindAbs32Label(RVA rva) {
+  return abs32_label_manager_.Find(rva);
+}
+
+Label* AssemblyProgram::FindRel32Label(RVA rva) {
+  return rel32_label_manager_.Find(rva);
 }
 
 Label* AssemblyProgram::InstructionAbs32Label(
@@ -238,17 +254,6 @@ CheckBool AssemblyProgram::EmitShared(Instruction* instruction) {
   return instruction && instructions_.push_back(instruction);
 }
 
-Label* AssemblyProgram::FindLabel(RVA rva, RVAToLabel* labels) {
-  Label*& slot = (*labels)[rva];
-  if (slot == NULL) {
-    slot = UncheckedNew<Label>(rva);
-    if (slot == NULL)
-      return NULL;
-  }
-  slot->count_++;
-  return slot;
-}
-
 void AssemblyProgram::UnassignIndexes(RVAToLabel* labels) {
   for (RVAToLabel::iterator p = labels->begin();  p != labels->end();  ++p) {
     Label* current = p->second;
@@ -367,7 +372,7 @@ std::unique_ptr<EncodedProgram> AssemblyProgram::Encode() const {
 
   encoded->set_image_base(image_base_);
 
-  if (!encoded->DefineLabels(abs32_labels_, rel32_labels_))
+  if (!encoded->ImportLabels(abs32_label_manager_, rel32_label_manager_))
     return nullptr;
 
   for (size_t i = 0;  i < instructions_.size();  ++i) {
@@ -470,61 +475,6 @@ Instruction* AssemblyProgram::GetByteInstruction(uint8_t byte) {
   return byte_instruction_cache_[byte];
 }
 
-// Chosen empirically to give the best reduction in payload size for
-// an update from daisy_3701.98.0 to daisy_4206.0.0.
-const int AssemblyProgram::kLabelLowerLimit = 5;
-
-CheckBool AssemblyProgram::TrimLabels() {
-  // For now only trim for ARM binaries.
-  if (kind() != EXE_ELF_32_ARM)
-    return true;
-
-  int lower_limit = kLabelLowerLimit;
-
-  VLOG(1) << "TrimLabels: threshold " << lower_limit;
-
-  // Walk through the list of instructions, replacing trimmed labels
-  // with the original machine instruction.
-  for (size_t i = 0; i < instructions_.size(); ++i) {
-    Instruction* instruction = instructions_[i];
-    switch (instruction->op()) {
-      case REL32ARM: {
-        Label* label =
-            static_cast<InstructionWithLabelARM*>(instruction)->label();
-        if (label->count_ <= lower_limit) {
-          const uint8_t* arm_op =
-              static_cast<InstructionWithLabelARM*>(instruction)->arm_op();
-          uint16_t op_size =
-              static_cast<InstructionWithLabelARM*>(instruction)->op_size();
-
-          if (op_size < 1)
-            return false;
-          UncheckedDelete(instruction);
-          instructions_[i] = UncheckedNew<BytesInstruction>(arm_op, op_size);
-          if (!instructions_[i])
-            return false;
-        }
-        break;
-      }
-      default:
-        break;
-    }
-  }
-
-  // Remove and deallocate underused Labels.
-  RVAToLabel::iterator it = rel32_labels_.begin();
-  while (it != rel32_labels_.end()) {
-    if (it->second->count_ <= lower_limit) {
-      UncheckedDelete(it->second);
-      rel32_labels_.erase(it++);
-    } else {
-      ++it;
-    }
-  }
-
-  return true;
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 
 Status Encode(const AssemblyProgram& program,
diff --git a/courgette/assembly_program.h b/courgette/assembly_program.h
index 2d157dd480d6c..96d6dafe35676 100644
--- a/courgette/assembly_program.h
+++ b/courgette/assembly_program.h
@@ -123,16 +123,23 @@ class AssemblyProgram {
   // Generates 8-byte absolute reference to address of 'label'.
   CheckBool EmitAbs64(Label* label) WARN_UNUSED_RESULT;
 
-  // Looks up a label or creates a new one.  Might return NULL.
-  Label* FindOrMakeAbs32Label(RVA rva);
+  // Traverses RVAs in |abs32_visitor| and |rel32_visitor| to precompute Labels.
+  void PrecomputeLabels(RvaVisitor* abs32_visitor, RvaVisitor* rel32_visitor);
 
-  // Looks up a label or creates a new one.  Might return NULL.
-  Label* FindOrMakeRel32Label(RVA rva);
+  // Removes underused Labels. Thresholds used (0 = no trimming) is
+  // architecture-dependent.
+  void TrimLabels();
 
-  void DefaultAssignIndexes();
   void UnassignIndexes();
+  void DefaultAssignIndexes();
   void AssignRemainingIndexes();
 
+  // Looks up abs32 label. Returns null if none found.
+  Label* FindAbs32Label(RVA rva);
+
+  // Looks up rel32 label. Returns null if none found.
+  Label* FindRel32Label(RVA rva);
+
   std::unique_ptr<EncodedProgram> Encode() const;
 
   // Accessor for instruction list.
@@ -152,10 +159,6 @@ class AssemblyProgram {
   // otherwise returns NULL.
   Label* InstructionRel32Label(const Instruction* instruction) const;
 
-  // Removes underused Labels. Thresholds used (may be 0, i.e., no trimming) is
-  // dependent on architecture. Returns true on success, and false otherwise.
-  CheckBool TrimLabels();
-
  private:
   using ScopedInstruction =
       std::unique_ptr<Instruction, UncheckedDeleter<Instruction>>;
@@ -183,11 +186,10 @@ class AssemblyProgram {
 
   InstructionVector instructions_;  // All the instructions in program.
 
-  // These are lookup maps to find the label associated with a given address.
-  // We have separate label spaces for addresses referenced by rel32 labels and
-  // abs32 labels.  This is somewhat arbitrary.
-  RVAToLabel rel32_labels_;
-  RVAToLabel abs32_labels_;
+  // Storage and lookup of Labels associated with target addresses. We use
+  // separate abs32 and rel32 labels.
+  LabelManager abs32_label_manager_;
+  LabelManager rel32_label_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(AssemblyProgram);
 };
diff --git a/courgette/courgette.h b/courgette/courgette.h
index 84ab7cddd3d9d..5f3dbb8367a81 100644
--- a/courgette/courgette.h
+++ b/courgette/courgette.h
@@ -48,7 +48,6 @@ enum Status {
   C_DISASSEMBLY_FAILED = 25,      //
   C_ASSEMBLY_FAILED = 26,         //
   C_ADJUSTMENT_FAILED = 27,       //
-  C_TRIM_FAILED = 28,             // TrimLabels failed
 };
 
 // What type of executable is something
diff --git a/courgette/disassembler.cc b/courgette/disassembler.cc
index 9b58ba0815736..baa3b1f8c7803 100644
--- a/courgette/disassembler.cc
+++ b/courgette/disassembler.cc
@@ -4,10 +4,35 @@
 
 #include "courgette/disassembler.h"
 
+#include <memory>
+
 #include "base/logging.h"
+#include "courgette/assembly_program.h"
 
 namespace courgette {
 
+Disassembler::RvaVisitor_Abs32::RvaVisitor_Abs32(
+    const std::vector<RVA>& rva_locations,
+    const AddressTranslator& translator)
+    : VectorRvaVisitor<RVA>(rva_locations), translator_(translator) {
+}
+
+RVA Disassembler::RvaVisitor_Abs32::Get() const {
+  // For Abs32 targets, get target RVA from architecture-dependent functions.
+  return translator_.PointerToTargetRVA(translator_.RVAToPointer(*it_));
+}
+
+Disassembler::RvaVisitor_Rel32::RvaVisitor_Rel32(
+    const std::vector<RVA>& rva_locations,
+    const AddressTranslator& translator)
+    : VectorRvaVisitor<RVA>(rva_locations), translator_(translator) {
+}
+
+RVA Disassembler::RvaVisitor_Rel32::Get() const {
+  // For Rel32 targets, only handle 32-bit offsets.
+  return *it_ + 4 + Read32LittleEndian(translator_.RVAToPointer(*it_));
+}
+
 Disassembler::Disassembler(const void* start, size_t length)
     : failure_reason_("uninitialized") {
   start_ = reinterpret_cast<const uint8_t*>(start);
@@ -40,6 +65,12 @@ bool Disassembler::Bad(const char* reason) {
   return false;
 }
 
+void Disassembler::PrecomputeLabels(AssemblyProgram* program) {
+  std::unique_ptr<RvaVisitor> abs32_visitor(CreateAbs32TargetRvaVisitor());
+  std::unique_ptr<RvaVisitor> rel32_visitor(CreateRel32TargetRvaVisitor());
+  program->PrecomputeLabels(abs32_visitor.get(), rel32_visitor.get());
+}
+
 void Disassembler::ReduceLength(size_t reduced_length) {
   CHECK_LE(reduced_length, length_);
   length_ = reduced_length;
diff --git a/courgette/disassembler.h b/courgette/disassembler.h
index 7c57099570d66..d84b11406a520 100644
--- a/courgette/disassembler.h
+++ b/courgette/disassembler.h
@@ -8,6 +8,8 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <vector>
+
 #include "base/macros.h"
 #include "courgette/courgette.h"
 #include "courgette/image_utils.h"
@@ -18,6 +20,38 @@ class AssemblyProgram;
 
 class Disassembler : public AddressTranslator {
  public:
+  // Visitor/adaptor to translate RVA to target RVA for abs32.
+  class RvaVisitor_Abs32 : public VectorRvaVisitor<RVA> {
+   public:
+    RvaVisitor_Abs32(const std::vector<RVA>& rva_locations,
+                     const AddressTranslator& translator);
+    ~RvaVisitor_Abs32() override { }
+
+    // VectorRvaVisitor<RVA> interfaces.
+    RVA Get() const override;
+
+   private:
+    const AddressTranslator& translator_;
+
+    DISALLOW_COPY_AND_ASSIGN(RvaVisitor_Abs32);
+  };
+
+  // Visitor/adaptor to translate RVA to target RVA for rel32.
+  class RvaVisitor_Rel32 : public VectorRvaVisitor<RVA> {
+   public:
+    RvaVisitor_Rel32(const std::vector<RVA>& rva_locations,
+                     const AddressTranslator& translator);
+    ~RvaVisitor_Rel32() override { }
+
+    // VectorRvaVisitor<RVA> interfaces.
+    RVA Get() const override;
+
+   private:
+    const AddressTranslator& translator_;
+
+    DISALLOW_COPY_AND_ASSIGN(RvaVisitor_Rel32);
+  };
+
   virtual ~Disassembler();
 
   // AddressTranslator interfaces.
@@ -29,6 +63,17 @@ class Disassembler : public AddressTranslator {
 
   virtual ExecutableType kind() const = 0;
 
+  // Returns a caller-owned new RvaVisitor to iterate through abs32 target RVAs.
+  virtual RvaVisitor* CreateAbs32TargetRvaVisitor() = 0;
+
+  // Returns a caller-owned new RvaVisitor to iterate through rel32 target RVAs.
+  virtual RvaVisitor* CreateRel32TargetRvaVisitor() = 0;
+
+  // Removes unused rel32 locations (architecture-specific). This is needed
+  // because we may remove rel32 Labels along the way. As a result the matching
+  // matching rel32 addresses become unused. Removing them saves space.
+  virtual void RemoveUnusedRel32Locations(AssemblyProgram* program) = 0;
+
   // Returns true if the buffer appears to be a valid executable of the expected
   // type, and false otherwise. This needs not be called before Disassemble().
   virtual bool ParseHeader() = 0;
@@ -57,6 +102,9 @@ class Disassembler : public AddressTranslator {
     return offset <= length() && elements <= (length() - offset) / element_size;
   }
 
+  // Computes and stores all Labels before scanning program bytes.
+  void PrecomputeLabels(AssemblyProgram* program);
+
   // Reduce the length of the image in memory. Does not actually free
   // (or realloc) any memory. Usually only called via ParseHeader().
   void ReduceLength(size_t reduced_length);
diff --git a/courgette/disassembler_elf_32.cc b/courgette/disassembler_elf_32.cc
index d9d5043b2fdba..febcdeabef504 100644
--- a/courgette/disassembler_elf_32.cc
+++ b/courgette/disassembler_elf_32.cc
@@ -5,6 +5,7 @@
 #include "courgette/disassembler_elf_32.h"
 
 #include <algorithm>
+#include <iterator>
 #include <utility>
 
 #include "base/logging.h"
@@ -33,6 +34,15 @@ std::vector<Elf32_Half> GetSectionHeaderFileOffsetOrder(
 
 }  // namespace
 
+DisassemblerElf32::Elf32RvaVisitor_Rel32::Elf32RvaVisitor_Rel32(
+    const std::vector<std::unique_ptr<TypedRVA>>& rva_locations)
+    : VectorRvaVisitor<std::unique_ptr<TypedRVA>>(rva_locations) {
+}
+
+RVA DisassemblerElf32::Elf32RvaVisitor_Rel32::Get() const {
+  return (*it_)->rva() + (*it_)->relative_target();
+}
+
 DisassemblerElf32::DisassemblerElf32(const void* start, size_t length)
     : Disassembler(start, length),
       header_(nullptr),
@@ -164,12 +174,16 @@ bool DisassemblerElf32::Disassemble(AssemblyProgram* target) {
   if (!ParseAbs32Relocs())
     return false;
 
-  if (!ParseRel32RelocsFromSections())
+  if (!ParseRel32RelocsFromSections())  // Does not sort rel32 locations.
     return false;
 
+  PrecomputeLabels(target);
+  RemoveUnusedRel32Locations(target);
+
   if (!ParseFile(target))
     return false;
 
+  // Finally sort rel32 locations.
   std::sort(rel32_locations_.begin(),
             rel32_locations_.end(),
             TypedRVA::IsLessThanByRVA);
@@ -180,6 +194,28 @@ bool DisassemblerElf32::Disassemble(AssemblyProgram* target) {
   return true;
 }
 
+CheckBool DisassemblerElf32::IsValidTargetRVA(RVA rva) const {
+  if (rva == kUnassignedRVA)
+    return false;
+
+  // |rva| is valid if it's contained in any program segment.
+  for (Elf32_Half segment_id = 0; segment_id < ProgramSegmentHeaderCount();
+       ++segment_id) {
+    const Elf32_Phdr* segment_header = ProgramSegmentHeader(segment_id);
+
+    if (segment_header->p_type != PT_LOAD)
+      continue;
+
+    Elf32_Addr begin = segment_header->p_vaddr;
+    Elf32_Addr end = segment_header->p_vaddr + segment_header->p_memsz;
+
+    if (rva >= begin && rva < end)
+      return true;
+  }
+
+  return false;
+}
+
 bool DisassemblerElf32::UpdateLength() {
   Elf32_Off result = 0;
 
@@ -238,32 +274,11 @@ CheckBool DisassemblerElf32::SectionName(const Elf32_Shdr& shdr,
   return true;
 }
 
-CheckBool DisassemblerElf32::IsValidTargetRVA(RVA rva) const {
-  if (rva == kUnassignedRVA)
-    return false;
-
-  // It's valid if it's contained in any program segment
-  for (Elf32_Half segment_id = 0; segment_id < ProgramSegmentHeaderCount();
-       ++segment_id) {
-    const Elf32_Phdr* segment_header = ProgramSegmentHeader(segment_id);
-
-    if (segment_header->p_type != PT_LOAD)
-      continue;
-
-    Elf32_Addr begin = segment_header->p_vaddr;
-    Elf32_Addr end = segment_header->p_vaddr + segment_header->p_memsz;
-
-    if (rva >= begin && rva < end)
-      return true;
-  }
-
-  return false;
-}
-
 CheckBool DisassemblerElf32::RVAsToFileOffsets(
     const std::vector<RVA>& rvas,
     std::vector<FileOffset>* file_offsets) {
   file_offsets->clear();
+  file_offsets->reserve(rvas.size());
   for (RVA rva : rvas) {
     FileOffset file_offset = RVAToFileOffset(rva);
     if (file_offset == kNoFileOffset)
@@ -284,6 +299,32 @@ CheckBool DisassemblerElf32::RVAsToFileOffsets(
   return true;
 }
 
+RvaVisitor* DisassemblerElf32::CreateAbs32TargetRvaVisitor() {
+  return new RvaVisitor_Abs32(abs32_locations_, *this);
+}
+
+RvaVisitor* DisassemblerElf32::CreateRel32TargetRvaVisitor() {
+  return new Elf32RvaVisitor_Rel32(rel32_locations_);
+}
+
+void DisassemblerElf32::RemoveUnusedRel32Locations(AssemblyProgram* program) {
+  auto tail_it = rel32_locations_.begin();
+  for (auto head_it = rel32_locations_.begin();
+       head_it != rel32_locations_.end(); ++head_it) {
+    RVA target_rva = (*head_it)->rva() + (*head_it)->relative_target();
+    if (program->FindRel32Label(target_rva) == nullptr) {
+      // If address does not match a Label (because it was removed), deallocate.
+      (*head_it).reset(nullptr);
+    } else {
+      // Else squeeze nullptr to end to compactify.
+      if (tail_it != head_it)
+        (*tail_it).swap(*head_it);
+      ++tail_it;
+    }
+  }
+  rel32_locations_.resize(std::distance(rel32_locations_.begin(), tail_it));
+}
+
 CheckBool DisassemblerElf32::ParseFile(AssemblyProgram* program) {
   // Walk all the bytes in the file, whether or not in a section.
   FileOffset file_offset = 0;
@@ -425,7 +466,9 @@ CheckBool DisassemblerElf32::ParseProgbitsSection(
       RVA target_rva = PointerToTargetRVA(FileOffsetToPointer(file_offset));
       DCHECK_NE(kNoRVA, target_rva);
 
-      if (!program->EmitAbs32(program->FindOrMakeAbs32Label(target_rva)))
+      Label* label = program->FindAbs32Label(target_rva);
+      CHECK(label);
+      if (!program->EmitAbs32(label))
         return false;
       file_offset += sizeof(RVA);
       ++(*current_abs_offset);
@@ -435,12 +478,17 @@ CheckBool DisassemblerElf32::ParseProgbitsSection(
     if (*current_rel != end_rel &&
         file_offset == (**current_rel)->file_offset()) {
       uint32_t relative_target = (**current_rel)->relative_target();
+      CHECK_EQ(RVA(origin + (file_offset - origin_offset)),
+               (**current_rel)->rva());
       // This cast is for 64 bit systems, and is only safe because we
       // are working on 32 bit executables.
       RVA target_rva = (RVA)(origin + (file_offset - origin_offset) +
                              relative_target);
 
-      if (!(**current_rel)->EmitInstruction(program, target_rva))
+      Label* label = program->FindRel32Label(target_rva);
+      CHECK(label);
+
+      if (!(**current_rel)->EmitInstruction(program, label))
         return false;
       file_offset += (**current_rel)->op_size();
       ++(*current_rel);
diff --git a/courgette/disassembler_elf_32.h b/courgette/disassembler_elf_32.h
index cf3e900f0df22..6bcf1b888b80c 100644
--- a/courgette/disassembler_elf_32.h
+++ b/courgette/disassembler_elf_32.h
@@ -9,6 +9,7 @@
 #include <stdint.h>
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "base/macros.h"
@@ -51,9 +52,9 @@ class DisassemblerElf32 : public Disassembler {
     // Computes the relative jump's offset from the op in p.
     virtual CheckBool ComputeRelativeTarget(const uint8_t* op_pointer) = 0;
 
-    // Emits the courgette instruction corresponding to the RVA type.
+    // Emits the assembly instruction corresponding to |label|.
     virtual CheckBool EmitInstruction(AssemblyProgram* program,
-                                      RVA target_rva) = 0;
+                                      Label* label) = 0;
 
     // Returns the size of the instruction containing the RVA.
     virtual uint16_t op_size() const = 0;
@@ -76,6 +77,22 @@ class DisassemblerElf32 : public Disassembler {
     FileOffset file_offset_ = kNoFileOffset;
   };
 
+  // Visitor/adaptor to translate RVA to target RVA. This is the ELF
+  // counterpart to RvaVisitor_Rel32 that uses TypedRVA.
+  class Elf32RvaVisitor_Rel32 :
+  public VectorRvaVisitor<std::unique_ptr<TypedRVA>> {
+   public:
+    Elf32RvaVisitor_Rel32(
+        const std::vector<std::unique_ptr<TypedRVA>>& rva_locations);
+    ~Elf32RvaVisitor_Rel32() override { }
+
+    // VectorRvaVisitor<TypedRVA*> interfaces.
+    RVA Get() const override;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Elf32RvaVisitor_Rel32);
+  };
+
  public:
   DisassemblerElf32(const void* start, size_t length);
 
@@ -91,6 +108,12 @@ class DisassemblerElf32 : public Disassembler {
 
   virtual e_machine_values ElfEM() const = 0;
 
+  CheckBool IsValidTargetRVA(RVA rva) const WARN_UNUSED_RESULT;
+
+  // Converts an ELF relocation instruction into an RVA.
+  virtual CheckBool RelToRVA(Elf32_Rel rel, RVA* result)
+    const WARN_UNUSED_RESULT = 0;
+
   // Public for unittests only
   std::vector<RVA>& Abs32Locations() { return abs32_locations_; }
   std::vector<std::unique_ptr<TypedRVA>>& Rel32Locations() {
@@ -132,12 +155,6 @@ class DisassemblerElf32 : public Disassembler {
 
   // Misc address space helpers
 
-  CheckBool IsValidTargetRVA(RVA rva) const WARN_UNUSED_RESULT;
-
-  // Converts an ELF relocation instruction into an RVA.
-  virtual CheckBool RelToRVA(Elf32_Rel rel, RVA* result)
-    const WARN_UNUSED_RESULT = 0;
-
   CheckBool RVAsToFileOffsets(const std::vector<RVA>& rvas,
                               std::vector<FileOffset>* file_offsets);
 
@@ -153,6 +170,11 @@ class DisassemblerElf32 : public Disassembler {
   virtual CheckBool ParseRel32RelocsFromSection(const Elf32_Shdr* section)
       WARN_UNUSED_RESULT = 0;
 
+  // Disassembler interfaces.
+  RvaVisitor* CreateAbs32TargetRvaVisitor() override;
+  RvaVisitor* CreateRel32TargetRvaVisitor() override;
+  void RemoveUnusedRel32Locations(AssemblyProgram* program) override;
+
   CheckBool ParseFile(AssemblyProgram* target) WARN_UNUSED_RESULT;
 
   CheckBool ParseProgbitsSection(
diff --git a/courgette/disassembler_elf_32_arm.cc b/courgette/disassembler_elf_32_arm.cc
index f1fbc7af4fa90..3ea1008c47aba 100644
--- a/courgette/disassembler_elf_32_arm.cc
+++ b/courgette/disassembler_elf_32_arm.cc
@@ -300,11 +300,8 @@ CheckBool DisassemblerElf32ARM::TypedRVAARM::ComputeRelativeTarget(
 
 CheckBool DisassemblerElf32ARM::TypedRVAARM::EmitInstruction(
     AssemblyProgram* program,
-    RVA target_rva) {
-  return program->EmitRel32ARM(c_op(),
-                               program->FindOrMakeRel32Label(target_rva),
-                               arm_op_,
-                               op_size());
+    Label* label) {
+  return program->EmitRel32ARM(c_op(), label, arm_op_, op_size());
 }
 
 DisassemblerElf32ARM::DisassemblerElf32ARM(const void* start, size_t length)
diff --git a/courgette/disassembler_elf_32_arm.h b/courgette/disassembler_elf_32_arm.h
index 5dc6897f47c03..83f5dc6a237b8 100644
--- a/courgette/disassembler_elf_32_arm.h
+++ b/courgette/disassembler_elf_32_arm.h
@@ -36,7 +36,7 @@ class DisassemblerElf32ARM : public DisassemblerElf32 {
     // TypedRVA interfaces.
     CheckBool ComputeRelativeTarget(const uint8_t* op_pointer) override;
     CheckBool EmitInstruction(AssemblyProgram* program,
-                              RVA target_rva) override;
+                              Label* label) override;
     uint16_t op_size() const override;
 
     uint16_t c_op() const { return c_op_; }
diff --git a/courgette/disassembler_elf_32_x86.cc b/courgette/disassembler_elf_32_x86.cc
index 67aa5c3505f8e..ee6e732114961 100644
--- a/courgette/disassembler_elf_32_x86.cc
+++ b/courgette/disassembler_elf_32_x86.cc
@@ -22,8 +22,8 @@ CheckBool DisassemblerElf32X86::TypedRVAX86::ComputeRelativeTarget(
 
 CheckBool DisassemblerElf32X86::TypedRVAX86::EmitInstruction(
     AssemblyProgram* program,
-    RVA target_rva) {
-  return program->EmitRel32(program->FindOrMakeRel32Label(target_rva));
+    Label* label) {
+  return program->EmitRel32(label);
 }
 
 uint16_t DisassemblerElf32X86::TypedRVAX86::op_size() const {
diff --git a/courgette/disassembler_elf_32_x86.h b/courgette/disassembler_elf_32_x86.h
index 63be75544f3a5..706e6fee33b10 100644
--- a/courgette/disassembler_elf_32_x86.h
+++ b/courgette/disassembler_elf_32_x86.h
@@ -28,7 +28,7 @@ class DisassemblerElf32X86 : public DisassemblerElf32 {
     // TypedRVA interfaces.
     CheckBool ComputeRelativeTarget(const uint8_t* op_pointer) override;
     CheckBool EmitInstruction(AssemblyProgram* program,
-                              RVA target_rva) override;
+                              Label* label) override;
     uint16_t op_size() const override;
   };
 
diff --git a/courgette/disassembler_win32_x64.cc b/courgette/disassembler_win32_x64.cc
index ffa6c36a7aade..2db35a39d8a10 100644
--- a/courgette/disassembler_win32_x64.cc
+++ b/courgette/disassembler_win32_x64.cc
@@ -252,6 +252,9 @@ bool DisassemblerWin32X64::Disassemble(AssemblyProgram* target) {
 
   ParseRel32RelocsFromSections();
 
+  PrecomputeLabels(target);
+  RemoveUnusedRel32Locations(target);
+
   if (!ParseFile(target))
     return false;
 
@@ -357,6 +360,26 @@ std::string DisassemblerWin32X64::SectionName(const Section* section) {
   return name;
 }
 
+RvaVisitor* DisassemblerWin32X64::CreateAbs32TargetRvaVisitor() {
+  return new RvaVisitor_Abs32(abs32_locations_, *this);
+}
+
+RvaVisitor* DisassemblerWin32X64::CreateRel32TargetRvaVisitor() {
+  return new RvaVisitor_Rel32(rel32_locations_, *this);
+}
+
+void DisassemblerWin32X64::RemoveUnusedRel32Locations(
+    AssemblyProgram* program) {
+  auto cond = [this, program](RVA rva) -> bool {
+    // + 4 since offset is relative to start of next instruction.
+    RVA target_rva = rva + 4 + Read32LittleEndian(RVAToPointer(rva));
+    return program->FindRel32Label(target_rva) == nullptr;
+  };
+  rel32_locations_.erase(
+      std::remove_if(rel32_locations_.begin(), rel32_locations_.end(), cond),
+      rel32_locations_.end());
+}
+
 CheckBool DisassemblerWin32X64::ParseFile(AssemblyProgram* program) {
   // Walk all the bytes in the file, whether or not in a section.
   FileOffset file_offset = 0;
@@ -525,12 +548,13 @@ void DisassemblerWin32X64::ParseRel32RelocsFromSection(const Section* section) {
       if (abs32_pos != abs32_locations_.end()) {
         if (*abs32_pos < rel32_rva + 4) {
           // Beginning of abs32 reloc is before end of rel32 reloc so they
-          // overlap.  Skip four bytes past the abs32 reloc.
+          // overlap. Skip four bytes past the abs32 reloc.
           p += (*abs32_pos + 4) - current_rva;
           continue;
         }
       }
 
+      // + 4 since offset is relative to start of next instruction.
       RVA target_rva = rel32_rva + 4 + Read32LittleEndian(rel32);
       // To be valid, rel32 target must be within image, and within this
       // section.
@@ -614,7 +638,9 @@ CheckBool DisassemblerWin32X64::ParseFileRegion(const Section* section,
       DCHECK_NE(kNoRVA, target_rva);
       // TODO(sra): target could be Label+offset.  It is not clear how to guess
       // which it might be.  We assume offset==0.
-      if (!program->EmitAbs64(program->FindOrMakeAbs32Label(target_rva)))
+      Label* label = program->FindAbs32Label(target_rva);
+      DCHECK(label);
+      if (!program->EmitAbs64(label))
         return false;
       p += 8;
       continue;
@@ -624,8 +650,11 @@ CheckBool DisassemblerWin32X64::ParseFileRegion(const Section* section,
       ++rel32_pos;
 
     if (rel32_pos != rel32_locations_.end() && *rel32_pos == current_rva) {
+      // + 4 since offset is relative to start of next instruction.
       RVA target_rva = current_rva + 4 + Read32LittleEndian(p);
-      if (!program->EmitRel32(program->FindOrMakeRel32Label(target_rva)))
+      Label* label = program->FindRel32Label(target_rva);
+      DCHECK(label);
+      if (!program->EmitRel32(label))
         return false;
       p += 4;
       continue;
diff --git a/courgette/disassembler_win32_x64.h b/courgette/disassembler_win32_x64.h
index 60a2259fc0399..49ca45207b79c 100644
--- a/courgette/disassembler_win32_x64.h
+++ b/courgette/disassembler_win32_x64.h
@@ -54,6 +54,11 @@ class DisassemblerWin32X64 : public Disassembler {
   static std::string SectionName(const Section* section);
 
  protected:
+  // Disassembler interfaces.
+  RvaVisitor* CreateAbs32TargetRvaVisitor() override;
+  RvaVisitor* CreateRel32TargetRvaVisitor() override;
+  void RemoveUnusedRel32Locations(AssemblyProgram* program) override;
+
   CheckBool ParseFile(AssemblyProgram* target) WARN_UNUSED_RESULT;
   bool ParseAbs32Relocs();
   void ParseRel32RelocsFromSections();
diff --git a/courgette/disassembler_win32_x86.cc b/courgette/disassembler_win32_x86.cc
index 974e864033d43..fc3f921d7296c 100644
--- a/courgette/disassembler_win32_x86.cc
+++ b/courgette/disassembler_win32_x86.cc
@@ -252,6 +252,9 @@ bool DisassemblerWin32X86::Disassemble(AssemblyProgram* target) {
 
   ParseRel32RelocsFromSections();
 
+  PrecomputeLabels(target);
+  RemoveUnusedRel32Locations(target);
+
   if (!ParseFile(target))
     return false;
 
@@ -362,6 +365,26 @@ std::string DisassemblerWin32X86::SectionName(const Section* section) {
   return name;
 }
 
+RvaVisitor* DisassemblerWin32X86::CreateAbs32TargetRvaVisitor() {
+  return new RvaVisitor_Abs32(abs32_locations_, *this);
+}
+
+RvaVisitor* DisassemblerWin32X86::CreateRel32TargetRvaVisitor() {
+  return new RvaVisitor_Rel32(rel32_locations_, *this);
+}
+
+void DisassemblerWin32X86::RemoveUnusedRel32Locations(
+    AssemblyProgram* program) {
+  auto cond = [this, program](RVA rva) -> bool {
+    // + 4 since offset is relative to start of next instruction.
+    RVA target_rva = rva + 4 + Read32LittleEndian(RVAToPointer(rva));
+    return program->FindRel32Label(target_rva) == nullptr;
+  };
+  rel32_locations_.erase(
+      std::remove_if(rel32_locations_.begin(), rel32_locations_.end(), cond),
+      rel32_locations_.end());
+}
+
 CheckBool DisassemblerWin32X86::ParseFile(AssemblyProgram* program) {
   // Walk all the bytes in the file, whether or not in a section.
   FileOffset file_offset = 0;
@@ -543,7 +566,9 @@ CheckBool DisassemblerWin32X86::ParseFileRegion(const Section* section,
       DCHECK_NE(kNoRVA, target_rva);
       // TODO(sra): target could be Label+offset.  It is not clear how to guess
       // which it might be.  We assume offset==0.
-      if (!program->EmitAbs32(program->FindOrMakeAbs32Label(target_rva)))
+      Label* label = program->FindAbs32Label(target_rva);
+      DCHECK(label);
+      if (!program->EmitAbs32(label))
         return false;
       p += 4;
       continue;
@@ -553,8 +578,11 @@ CheckBool DisassemblerWin32X86::ParseFileRegion(const Section* section,
       ++rel32_pos;
 
     if (rel32_pos != rel32_locations_.end() && *rel32_pos == current_rva) {
+      // + 4 since offset is relative to start of next instruction.
       RVA target_rva = current_rva + 4 + Read32LittleEndian(p);
-      if (!program->EmitRel32(program->FindOrMakeRel32Label(target_rva)))
+      Label* label = program->FindRel32Label(target_rva);
+      DCHECK(label);
+      if (!program->EmitRel32(label))
         return false;
       p += 4;
       continue;
diff --git a/courgette/disassembler_win32_x86.h b/courgette/disassembler_win32_x86.h
index 597f841ff8141..a01f0403f25dd 100644
--- a/courgette/disassembler_win32_x86.h
+++ b/courgette/disassembler_win32_x86.h
@@ -54,6 +54,11 @@ class DisassemblerWin32X86 : public Disassembler {
   static std::string SectionName(const Section* section);
 
  protected:
+  // Disassembler interfaces.
+  RvaVisitor* CreateAbs32TargetRvaVisitor() override;
+  RvaVisitor* CreateRel32TargetRvaVisitor() override;
+  void RemoveUnusedRel32Locations(AssemblyProgram* program) override;
+
   CheckBool ParseFile(AssemblyProgram* target) WARN_UNUSED_RESULT;
   bool ParseAbs32Relocs();
   void ParseRel32RelocsFromSections();
diff --git a/courgette/encoded_program.cc b/courgette/encoded_program.cc
index e14b37efeba46..129728a79860b 100644
--- a/courgette/encoded_program.cc
+++ b/courgette/encoded_program.cc
@@ -137,38 +137,15 @@ bool ReadVectorU8(V* items, SourceStream* buffer) {
 EncodedProgram::EncodedProgram() {}
 EncodedProgram::~EncodedProgram() {}
 
-CheckBool EncodedProgram::DefineLabels(const RVAToLabel& abs32_labels,
-                                       const RVAToLabel& rel32_labels) {
-  // which == 0 => abs32; which == 1 => rel32.
-  for (int which = 0; which < 2; ++which) {
-    const RVAToLabel& labels = which == 0 ? abs32_labels : rel32_labels;
-    RvaVector& rvas = which == 0 ? abs32_rva_ : rel32_rva_;
-
-    if (!rvas.resize(LabelManager::GetIndexBound(labels), kUnassignedRVA))
-      return false;
-
-    // For each Label, write its RVA to assigned index.
-    for (const auto& rva_and_label : labels) {
-      const Label& label = *rva_and_label.second;
-      DCHECK_EQ(rva_and_label.first, label.rva_);
-      DCHECK_NE(label.index_, Label::kNoIndex);
-      DCHECK_EQ(rvas[label.index_], kUnassignedRVA)
-          << "DefineLabels() double assigned " << label.index_;
-      rvas[label.index_] = label.rva_;
-    }
-
-    // Replace all unassigned slots with the value at the previous index so they
-    // delta-encode to zero. (There might be better values than zero. The way to
-    // get that is have the higher level assembly program assign the unassigned
-    // slots.)
-    RVA previous = 0;
-    for (RVA& rva : rvas) {
-      if (rva == kUnassignedRVA)
-        rva = previous;
-      else
-        previous = rva;
-    }
+CheckBool EncodedProgram::ImportLabels(
+    const LabelManager& abs32_label_manager,
+    const LabelManager& rel32_label_manager) {
+  if (!WriteRvasToList(abs32_label_manager, &abs32_rva_) ||
+      !WriteRvasToList(rel32_label_manager, &rel32_rva_)) {
+    return false;
   }
+  FillUnassignedRvaSlots(&abs32_rva_);
+  FillUnassignedRvaSlots(&rel32_rva_);
   return true;
 }
 
@@ -733,6 +710,43 @@ class RelocBlock {
   RelocBlockPOD pod;
 };
 
+// static
+// Updates |rvas| so |rvas[label.index_] == label.rva_| for each |label| in
+// |label_manager|, assuming |label.index_| is properly assigned. Takes care of
+// |rvas| resizing. Unused slots in |rvas| are assigned |kUnassignedRVA|.
+// Returns true on success, and false otherwise.
+CheckBool EncodedProgram::WriteRvasToList(const LabelManager& label_manager,
+                                          RvaVector* rvas) {
+  rvas->clear();
+  int index_bound = LabelManager::GetLabelIndexBound(label_manager.Labels());
+  if (!rvas->resize(index_bound, kUnassignedRVA))
+    return false;
+
+  // For each Label, write its RVA to assigned index.
+  for (const Label& label : label_manager.Labels()) {
+    DCHECK_NE(label.index_, Label::kNoIndex);
+    DCHECK_EQ((*rvas)[label.index_], kUnassignedRVA)
+        << "ExportToList() double assigned " << label.index_;
+    (*rvas)[label.index_] = label.rva_;
+  }
+  return true;
+}
+
+// static
+// Replaces all unassigned slots in |rvas| with the value at the previous index
+// so they delta-encode to zero. (There might be better values than zero. The
+// way to get that is have the higher level assembly program assign the
+// unassigned slots.)
+void EncodedProgram::FillUnassignedRvaSlots(RvaVector* rvas) {
+  RVA previous = 0;
+  for (RVA& rva : *rvas) {
+    if (rva == kUnassignedRVA)
+      rva = previous;
+    else
+      previous = rva;
+  }
+}
+
 CheckBool EncodedProgram::GeneratePeRelocations(SinkStream* buffer,
                                                 uint8_t type) {
   std::sort(abs32_relocs_.begin(), abs32_relocs_.end());
diff --git a/courgette/encoded_program.h b/courgette/encoded_program.h
index d0ce945f34f9f..814a45cf35306 100644
--- a/courgette/encoded_program.h
+++ b/courgette/encoded_program.h
@@ -50,10 +50,11 @@ class EncodedProgram {
   // (1) The image base can be specified at any time.
   void set_image_base(uint64_t base) { image_base_ = base; }
 
-  // (2) Address tables and indexes defined first.
-  CheckBool DefineLabels(const RVAToLabel& abs32_labels,
-                         const RVAToLabel& rel32_labels) WARN_UNUSED_RESULT;
+  // (2) Address tables and indexes imported first.
 
+  CheckBool ImportLabels(const LabelManager& abs32_label_manager,
+                         const LabelManager& rel32_label_manager)
+      WARN_UNUSED_RESULT;
 
   // (3) Add instructions in the order needed to generate bytes of file.
   // NOTE: If any of these methods ever fail, the EncodedProgram instance
@@ -114,6 +115,14 @@ class EncodedProgram {
   typedef NoThrowBuffer<OP> OPVector;
 
   void DebuggingSummary();
+
+  // Helper for ImportLabels().
+  static CheckBool WriteRvasToList(const LabelManager& label_manager,
+                                   RvaVector* rvas);
+
+  // Helper for ImportLabels().
+  static void FillUnassignedRvaSlots(RvaVector* rvas);
+
   CheckBool GeneratePeRelocations(SinkStream* buffer,
                                   uint8_t type) WARN_UNUSED_RESULT;
   CheckBool GenerateElfRelocations(Elf32_Word pending_elf_relocation_table,
diff --git a/courgette/encoded_program_unittest.cc b/courgette/encoded_program_unittest.cc
index 52f8d88679ecd..e8e461481c144 100644
--- a/courgette/encoded_program_unittest.cc
+++ b/courgette/encoded_program_unittest.cc
@@ -21,55 +21,32 @@ namespace courgette {
 namespace {
 
 // Helper class to instantiate RVAToLabel while managing allocation.
-class RVAToLabelMaker {
+class TestLabelManager : public LabelManager {
  public:
-  RVAToLabelMaker() {}
-  ~RVAToLabelMaker() {}
-
-  // Adds a Label for storage. Must be called before Make(), since |labels_map_|
-  // has pointers into |labels_|, and |labels_.push_back()| can invalidate them.
-  void Add(int index, RVA rva) {
-    DCHECK(label_map_.empty());
+  void RawAddLabel(int index, RVA rva) {
     labels_.push_back(Label(rva, index));  // Don't care about |count_|.
   }
-
-  // Initializes |label_map_| and returns a pointer to it.
-  RVAToLabel* Make() {
-    for (size_t i = 0; i < labels_.size(); ++i) {
-      DCHECK_EQ(0U, label_map_.count(labels_[i].rva_));
-      label_map_[labels_[i].rva_] = &labels_[i];
-    }
-    return &label_map_;
-  }
-
-  const std::vector<Label>& GetLabels() const { return labels_; }
-
- private:
-  std::vector<Label> labels_;  // Stores all Labels.
-  RVAToLabel label_map_;  // Has pointers to |labels_| elements.
 };
 
 // Creates a simple new program with given addresses. The orders of elements
 // in |abs32_specs| and |rel32_specs| are important.
 std::unique_ptr<EncodedProgram> CreateTestProgram(
-    RVAToLabelMaker* abs32_maker,
-    RVAToLabelMaker* rel32_maker) {
-  RVAToLabel* abs32_labels = abs32_maker->Make();
-  RVAToLabel* rel32_labels = rel32_maker->Make();
-
+    const TestLabelManager& abs32_label_manager,
+    const TestLabelManager& rel32_label_manager) {
   std::unique_ptr<EncodedProgram> program(new EncodedProgram());
 
   uint32_t base = 0x00900000;
   program->set_image_base(base);
 
-  EXPECT_TRUE(program->DefineLabels(*abs32_labels, *rel32_labels));
+  EXPECT_TRUE(program->ImportLabels(abs32_label_manager, rel32_label_manager));
 
   EXPECT_TRUE(program->AddOrigin(0));  // Start at base.
 
-  // Arbitrary: Add instructions in the order they're defined in |*_maker|.
-  for (const Label& label : abs32_maker->GetLabels())
+  // Add instructions. Since we're using TestLabelManager, Labels are sorted in
+  // the order they're added via Add().
+  for (const Label& label : abs32_label_manager.Labels())
     EXPECT_TRUE(program->AddAbs32(label.index_));
-  for (const Label& label : rel32_maker->GetLabels())
+  for (const Label& label : rel32_label_manager.Labels())
     EXPECT_TRUE(program->AddRel32(label.index_));
 
   return program;
@@ -91,14 +68,14 @@ bool CompareSink(const uint8_t expected[],
 // check that the bits produced are as expected.
 TEST(EncodedProgramTest, Test) {
   // ABS32 index 7 <-- base + 4.
-  RVAToLabelMaker abs32_label_maker;
-  abs32_label_maker.Add(7, 4);
+  TestLabelManager abs32_label_manager;
+  abs32_label_manager.RawAddLabel(7, 4);
   // REL32 index 5 <-- base + 0.
-  RVAToLabelMaker rel32_label_maker;
-  rel32_label_maker.Add(5, 0);
+  TestLabelManager rel32_label_manager;
+  rel32_label_manager.RawAddLabel(5, 0);
 
   std::unique_ptr<EncodedProgram> program(
-      CreateTestProgram(&abs32_label_maker, &rel32_label_maker));
+      CreateTestProgram(abs32_label_manager, rel32_label_manager));
 
   // Serialize and deserialize.
   SinkStreamSet sinks;
@@ -139,18 +116,18 @@ TEST(EncodedProgramTest, Test) {
 // contents of the address streams.
 TEST(EncodedProgramTest, TestWriteAddress) {
   // Absolute addresses by index: [_, _, _, 2, _, 23, _, 11].
-  RVAToLabelMaker abs32_label_maker;
-  abs32_label_maker.Add(7, 11);
-  abs32_label_maker.Add(3, 2);
-  abs32_label_maker.Add(5, 23);
+  TestLabelManager abs32_label_manager;
+  abs32_label_manager.RawAddLabel(7, 11);
+  abs32_label_manager.RawAddLabel(3, 2);
+  abs32_label_manager.RawAddLabel(5, 23);
   // Relative addresses by index: [16, 7, _, 32].
-  RVAToLabelMaker rel32_label_maker;
-  rel32_label_maker.Add(0, 16);
-  rel32_label_maker.Add(3, 32);
-  rel32_label_maker.Add(1, 7);
+  TestLabelManager rel32_label_manager;
+  rel32_label_manager.RawAddLabel(0, 16);
+  rel32_label_manager.RawAddLabel(3, 32);
+  rel32_label_manager.RawAddLabel(1, 7);
 
   std::unique_ptr<EncodedProgram> program(
-      CreateTestProgram(&abs32_label_maker, &rel32_label_maker));
+      CreateTestProgram(abs32_label_manager, rel32_label_manager));
 
   SinkStreamSet sinks;
   EXPECT_TRUE(program->WriteTo(&sinks));
diff --git a/courgette/image_utils.h b/courgette/image_utils.h
index 643b9fb23873c..19a04d52e1f71 100644
--- a/courgette/image_utils.h
+++ b/courgette/image_utils.h
@@ -101,6 +101,8 @@ class Label {
 // to the matching RVA target on demand without extra storage.
 class RvaVisitor {
  public:
+  virtual ~RvaVisitor() { }
+
   // Returns the number of remaining RVAs to visit.
   virtual size_t Remaining() const = 0;
 
@@ -118,7 +120,8 @@ class VectorRvaVisitor : public RvaVisitor {
  public:
   // Assumes |v| does not change for the lifetime of this instance.
   explicit VectorRvaVisitor(const std::vector<T>& v)
-      : it_(v.begin()), end_(v.end()) {}
+      : it_(v.begin()), end_(v.end()) { }
+  ~VectorRvaVisitor() override { }
 
   // RvaVisitor interfaces.
   size_t Remaining() const override { return std::distance(it_, end_); }
@@ -134,7 +137,8 @@ class VectorRvaVisitor : public RvaVisitor {
 class TrivialRvaVisitor : public VectorRvaVisitor<RVA> {
  public:
   explicit TrivialRvaVisitor(const std::vector<RVA>& rvas)
-      : VectorRvaVisitor<RVA>(rvas) {}
+      : VectorRvaVisitor<RVA>(rvas) { }
+  ~TrivialRvaVisitor() override { }
 
   // VectorRvaVisitor<RVA> interfaces.
   RVA Get() const override { return *it_; }
diff --git a/courgette/label_manager.cc b/courgette/label_manager.cc
index a109d8bda9fe9..51e0d5de43c94 100644
--- a/courgette/label_manager.cc
+++ b/courgette/label_manager.cc
@@ -95,17 +95,6 @@ LabelManager::LabelManager() {}
 
 LabelManager::~LabelManager() {}
 
-// static
-int LabelManager::GetIndexBound(const RVAToLabel& labels_map) {
-  int max_index = -1;
-  for (const auto& rva_and_label : labels_map) {
-    const Label& label = *rva_and_label.second;
-    if (label.index_ != Label::kNoIndex)
-      max_index = std::max(max_index, label.index_);
-  }
-  return max_index + 1;
-}
-
 // static
 int LabelManager::GetLabelIndexBound(const LabelVector& labels) {
   int max_index = -1;
diff --git a/courgette/label_manager.h b/courgette/label_manager.h
index d60a2746da22b..4b2524053280c 100644
--- a/courgette/label_manager.h
+++ b/courgette/label_manager.h
@@ -80,10 +80,6 @@ class LabelManager {
   LabelManager();
   ~LabelManager();
 
-  // Returns an exclusive upper bound for all existing indexes in |labels_map|.
-  // TODO(huangs): Remove once all callers are gone.
-  static int GetIndexBound(const RVAToLabel& labels_map);
-
   // Returns an exclusive upper bound for all assigned indexes in |labels|.
   static int GetLabelIndexBound(const LabelVector& labels);
 
diff --git a/courgette/program_detector.cc b/courgette/program_detector.cc
index 80dbd2d852a49..60b295fef7a82 100644
--- a/courgette/program_detector.cc
+++ b/courgette/program_detector.cc
@@ -78,9 +78,6 @@ Status ParseDetectedExecutable(const void* buffer,
   if (!disassembler->Disassemble(program.get()))
     return C_DISASSEMBLY_FAILED;
 
-  if (!program->TrimLabels())
-    return C_TRIM_FAILED;
-
   *output = std::move(program);
   return C_OK;
 }
diff --git a/courgette/rel32_finder_win32_x86.cc b/courgette/rel32_finder_win32_x86.cc
index 0ed492f2e5f48..61ff9696d5ac1 100644
--- a/courgette/rel32_finder_win32_x86.cc
+++ b/courgette/rel32_finder_win32_x86.cc
@@ -87,12 +87,13 @@ void Rel32FinderWin32X86_Basic::Find(const uint8_t* start_pointer,
       if (abs32_pos != abs32_locations.end()) {
         if (*abs32_pos < rel32_rva + 4) {
           // Beginning of abs32 reloc is before end of rel32 reloc so they
-          // overlap.  Skip four bytes past the abs32 reloc.
+          // overlap. Skip four bytes past the abs32 reloc.
           p += (*abs32_pos + 4) - current_rva;
           continue;
         }
       }
 
+      // + 4 since offset is relative to start of next instruction.
       RVA target_rva = rel32_rva + 4 + Read32LittleEndian(rel32);
       // Valid, rel32 target must be within image, and within this section.
       // Subsumes |target_rva| != |kUnassignedRVA|.