[Courgette] Simple AssemblyProgram and Disassembler cleanups.

We perform mundane cleanups to prepare for next step in AssemblyProgram reduction: - Delete dead code (static functions) in AssemblyProgram that were left over from LabelManager usage. - Simplify image_base assignment for AssemblyProgram. - Source: Disassembler adds virtual function image_base() (PE file reads and stores it; ELF just returns 0). - Sink: AssemblyProgram takes it on construction (replaces mutator). - Disassembler::Disassemble() overrides: Rename |target| to |program|. BUG=660980 Review-Url: https://codereview.chromium.org/2583373002 Cr-Commit-Position: refs/heads/master@{#443609}
cbchan · Jan 13, 2017 · 8563622 · 8563622
1 parent 534dc4d
commit 8563622
Show file tree

Hide file tree

Showing 10 changed files with 40 additions and 175 deletions.
diff --git a/courgette/adjustment_method_unittest.cc b/courgette/adjustment_method_unittest.cc
@@ -8,7 +8,7 @@
 #include <vector>
 
 #include "base/bind.h"
-#include "base/strings/string_util.h"
+#include "base/memory/ptr_util.h"
 #include "courgette/assembly_program.h"
 #include "courgette/courgette.h"
 #include "courgette/encoded_program.h"
@@ -35,8 +35,7 @@ class AdjustmentMethodTest : public testing::Test {
   // Returns one of two similar simple programs. These differ only in Label
   // assignment, so it is possible to make them look identical.
   std::unique_ptr<AssemblyProgram> MakeProgram(int kind) const {
-    std::unique_ptr<AssemblyProgram> prog(new AssemblyProgram(EXE_WIN_32_X86));
-    prog->set_image_base(0x00400000);
+    auto prog = base::MakeUnique<AssemblyProgram>(EXE_WIN_32_X86, 0x00400000);
 
     RVA kRvaA = 0x00410000;
     RVA kRvaB = 0x00410004;

diff --git a/courgette/assembly_program.cc b/courgette/assembly_program.cc
@@ -4,16 +4,8 @@
 
 #include "courgette/assembly_program.h"
 
-#include <memory.h>
-#include <stddef.h>
-#include <stdint.h>
-
-#include <memory>
-#include <utility>
-#include <vector>
-
+#include "base/callback.h"
 #include "base/logging.h"
-#include "base/macros.h"
 #include "courgette/courgette.h"
 #include "courgette/encoded_program.h"
 
@@ -194,9 +186,8 @@ class InstructionStoreReceptor : public InstructionReceptor {
 
 /******** AssemblyProgram ********/
 
-AssemblyProgram::AssemblyProgram(ExecutableType kind)
-  : kind_(kind), image_base_(0) {
-}
+AssemblyProgram::AssemblyProgram(ExecutableType kind, uint64_t image_base)
+    : kind_(kind), image_base_(image_base) {}
 
 AssemblyProgram::~AssemblyProgram() {
   for (size_t i = 0;  i < instructions_.size();  ++i) {
@@ -341,115 +332,6 @@ CheckBool AssemblyProgram::EmitShared(Instruction* instruction) {
   return instruction && instructions_.push_back(instruction);
 }
 
-void AssemblyProgram::UnassignIndexes(RVAToLabel* labels) {
-  for (RVAToLabel::iterator p = labels->begin();  p != labels->end();  ++p) {
-    Label* current = p->second;
-    current->index_ = Label::kNoIndex;
-  }
-}
-
-// DefaultAssignIndexes takes a set of labels and assigns indexes in increasing
-// address order.
-void AssemblyProgram::DefaultAssignIndexes(RVAToLabel* labels) {
-  int index = 0;
-  for (RVAToLabel::iterator p = labels->begin();  p != labels->end();  ++p) {
-    Label* current = p->second;
-    if (current->index_ != Label::kNoIndex)
-      NOTREACHED();
-    current->index_ = index;
-    ++index;
-  }
-}
-
-// AssignRemainingIndexes assigns indexes to any addresses (labels) that are not
-// yet assigned an index.
-void AssemblyProgram::AssignRemainingIndexes(RVAToLabel* labels) {
-  // An address table compresses best when each index is associated with an
-  // address that is slight larger than the previous index.
-
-  // First see which indexes have not been used.  The 'available' vector could
-  // grow even bigger, but the number of addresses is a better starting size
-  // than empty.
-  std::vector<bool> available(labels->size(), true);
-  int used = 0;
-
-  for (RVAToLabel::iterator p = labels->begin();  p != labels->end();  ++p) {
-    int index = p->second->index_;
-    if (index != Label::kNoIndex) {
-      while (static_cast<size_t>(index) >= available.size())
-        available.push_back(true);
-      available.at(index) = false;
-      ++used;
-    }
-  }
-
-  VLOG(1) << used << " of " << labels->size() << " labels pre-assigned";
-
-  // Are there any unused labels that happen to be adjacent following a used
-  // label?
-  int fill_forward_count = 0;
-  Label* prev = 0;
-  for (RVAToLabel::iterator p = labels->begin();  p != labels->end();  ++p) {
-    Label* current = p->second;
-    if (current->index_ == Label::kNoIndex) {
-      int index = 0;
-      if (prev  &&  prev->index_ != Label::kNoIndex)
-        index = prev->index_ + 1;
-      if (index < static_cast<int>(available.size()) && available.at(index)) {
-        current->index_ = index;
-        available.at(index) = false;
-        ++fill_forward_count;
-      }
-    }
-    prev = current;
-  }
-
-  // Are there any unused labels that happen to be adjacent preceeding a used
-  // label?
-  int fill_backward_count = 0;
-  prev = 0;
-  for (RVAToLabel::reverse_iterator p = labels->rbegin();
-       p != labels->rend();
-       ++p) {
-    Label* current = p->second;
-    if (current->index_ == Label::kNoIndex) {
-      int prev_index;
-      if (prev)
-        prev_index = prev->index_;
-      else
-        prev_index = static_cast<uint32_t>(available.size());
-      if (prev_index != 0  &&
-          prev_index != Label::kNoIndex  &&
-          available.at(prev_index - 1)) {
-        current->index_ = prev_index - 1;
-        available.at(current->index_) = false;
-        ++fill_backward_count;
-      }
-    }
-    prev = current;
-  }
-
-  // Fill in any remaining indexes
-  int fill_infill_count = 0;
-  int index = 0;
-  for (RVAToLabel::iterator p = labels->begin();  p != labels->end();  ++p) {
-    Label* current = p->second;
-    if (current->index_ == Label::kNoIndex) {
-      while (!available.at(index)) {
-        ++index;
-      }
-      current->index_ = index;
-      available.at(index) = false;
-      ++index;
-      ++fill_infill_count;
-    }
-  }
-
-  VLOG(1) << "  fill forward " << fill_forward_count
-          << "  backward " << fill_backward_count
-          << "  infill " << fill_infill_count;
-}
-
 std::unique_ptr<EncodedProgram> AssemblyProgram::Encode() const {
   std::unique_ptr<EncodedProgram> encoded(new EncodedProgram());
 

diff --git a/courgette/assembly_program.h b/courgette/assembly_program.h
@@ -10,10 +10,8 @@
 
 #include <map>
 #include <memory>
-#include <set>
-#include <vector>
 
-#include "base/bind.h"
+#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/free_deleter.h"
 #include "courgette/courgette.h"
@@ -132,13 +130,11 @@ class AssemblyProgram {
   using InstructionGenerator =
       base::Callback<CheckBool(AssemblyProgram*, InstructionReceptor*)>;
 
-  explicit AssemblyProgram(ExecutableType kind);
+  AssemblyProgram(ExecutableType kind, uint64_t image_base);
   ~AssemblyProgram();
 
   ExecutableType kind() const { return kind_; }
 
-  void set_image_base(uint64_t image_base) { image_base_ = image_base; }
-
   // Traverses RVAs in |abs32_visitor| and |rel32_visitor| to precompute Labels.
   void PrecomputeLabels(RvaVisitor* abs32_visitor, RvaVisitor* rel32_visitor);
 
@@ -212,8 +208,6 @@ class AssemblyProgram {
   using ScopedInstruction =
       std::unique_ptr<Instruction, UncheckedDeleter<Instruction>>;
 
-  ExecutableType kind_;
-
   CheckBool Emit(ScopedInstruction instruction) WARN_UNUSED_RESULT;
   CheckBool EmitShared(Instruction* instruction) WARN_UNUSED_RESULT;
 
@@ -222,16 +216,13 @@ class AssemblyProgram {
   // Looks up a label or creates a new one.  Might return NULL.
   Label* FindLabel(RVA rva, RVAToLabel* labels);
 
-  // Helper methods for the public versions.
-  static void UnassignIndexes(RVAToLabel* labels);
-  static void DefaultAssignIndexes(RVAToLabel* labels);
-  static void AssignRemainingIndexes(RVAToLabel* labels);
-
   // Sharing instructions that emit a single byte saves a lot of space.
   Instruction* GetByteInstruction(uint8_t byte);
-  std::unique_ptr<Instruction* [], base::FreeDeleter> byte_instruction_cache_;
 
-  uint64_t image_base_;  // Desired or mandated base address of image.
+  const ExecutableType kind_;
+  const uint64_t image_base_;  // Desired or mandated base address of image.
+
+  std::unique_ptr<Instruction* [], base::FreeDeleter> byte_instruction_cache_;
 
   InstructionVector instructions_;  // All the instructions in program.
 

diff --git a/courgette/disassembler.h b/courgette/disassembler.h
@@ -5,7 +5,6 @@
 #ifndef COURGETTE_DISASSEMBLER_H_
 #define COURGETTE_DISASSEMBLER_H_
 
-#include <stddef.h>
 #include <stdint.h>
 
 #include <vector>
@@ -55,14 +54,18 @@ class Disassembler : public AddressTranslator {
   virtual ~Disassembler();
 
   // AddressTranslator interfaces.
-  virtual RVA FileOffsetToRVA(FileOffset file_offset) const override = 0;
-  virtual FileOffset RVAToFileOffset(RVA rva) const override = 0;
+  RVA FileOffsetToRVA(FileOffset file_offset) const override = 0;
+  FileOffset RVAToFileOffset(RVA rva) const override = 0;
   const uint8_t* FileOffsetToPointer(FileOffset file_offset) const override;
   const uint8_t* RVAToPointer(RVA rva) const override;
-  RVA PointerToTargetRVA(const uint8_t* p) const = 0;
+  RVA PointerToTargetRVA(const uint8_t* p) const override = 0;
 
   virtual ExecutableType kind() const = 0;
 
+  // Returns the preferred image base address. Using uint64_t to accommodate the
+  // general case of 64-bit architectures.
+  virtual uint64_t image_base() const = 0;
+
   // Returns a caller-owned new RvaVisitor to iterate through abs32 target RVAs.
   virtual RvaVisitor* CreateAbs32TargetRvaVisitor() = 0;
 

diff --git a/courgette/disassembler_elf_32.cc b/courgette/disassembler_elf_32.cc
@@ -6,7 +6,6 @@
 
 #include <algorithm>
 #include <iterator>
-#include <utility>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ -164,23 +163,20 @@ bool DisassemblerElf32::ParseHeader() {
   return Good();
 }
 
-bool DisassemblerElf32::Disassemble(AssemblyProgram* target) {
+bool DisassemblerElf32::Disassemble(AssemblyProgram* program) {
   if (!ok())
     return false;
 
-  // The Image Base is always 0 for ELF Executables
-  target->set_image_base(0);
-
   if (!ParseAbs32Relocs())
     return false;
 
   if (!ParseRel32RelocsFromSections())  // Does not sort rel32 locations.
     return false;
 
-  PrecomputeLabels(target);
-  RemoveUnusedRel32Locations(target);
+  PrecomputeLabels(program);
+  RemoveUnusedRel32Locations(program);
 
-  if (!target->GenerateInstructions(
+  if (!program->GenerateInstructions(
           base::Bind(&DisassemblerElf32::ParseFile, base::Unretained(this)))) {
     return false;
   }
@@ -192,7 +188,7 @@ bool DisassemblerElf32::Disassemble(AssemblyProgram* target) {
   DCHECK(rel32_locations_.empty() ||
          rel32_locations_.back()->rva() != kUnassignedRVA);
 
-  target->DefaultAssignIndexes();
+  program->DefaultAssignIndexes();
   return true;
 }
 

diff --git a/courgette/disassembler_elf_32.h b/courgette/disassembler_elf_32.h
@@ -104,8 +104,9 @@ class DisassemblerElf32 : public Disassembler {
   FileOffset RVAToFileOffset(RVA rva) const override;
   RVA PointerToTargetRVA(const uint8_t* p) const override;
   ExecutableType kind() const override = 0;
+  uint64_t image_base() const override { return 0; }
   bool ParseHeader() override;
-  bool Disassemble(AssemblyProgram* target) override;
+  bool Disassemble(AssemblyProgram* program) override;
 
   virtual e_machine_values ElfEM() const = 0;
 

diff --git a/courgette/disassembler_elf_32_x86_unittest.cc b/courgette/disassembler_elf_32_x86_unittest.cc
@@ -14,6 +14,7 @@
 #include <string>
 #include <vector>
 
+#include "base/memory/ptr_util.h"
 #include "courgette/assembly_program.h"
 #include "courgette/base_test_unittest.h"
 #include "courgette/image_utils.h"
@@ -69,9 +70,8 @@ void DisassemblerElf32X86Test::TestExe(const char* file_name,
                                        size_t expected_rel_count) const {
   std::string file1 = FileContents(file_name);
 
-  std::unique_ptr<TestDisassemblerElf32X86> disassembler(
-      new TestDisassemblerElf32X86(
-          reinterpret_cast<const uint8_t*>(file1.c_str()), file1.length()));
+  auto disassembler = base::MakeUnique<TestDisassemblerElf32X86>(
+      reinterpret_cast<const uint8_t*>(file1.c_str()), file1.length());
 
   bool can_parse_header = disassembler->ParseHeader();
   EXPECT_TRUE(can_parse_header);
@@ -89,7 +89,7 @@ void DisassemblerElf32X86Test::TestExe(const char* file_name,
   EXPECT_EQ('L', offset_p[2]);
   EXPECT_EQ('F', offset_p[3]);
 
-  std::unique_ptr<AssemblyProgram> program(new AssemblyProgram(EXE_ELF_32_X86));
+  auto program = base::MakeUnique<AssemblyProgram>(EXE_ELF_32_X86, 0);
 
   EXPECT_TRUE(disassembler->Disassemble(program.get()));
 

diff --git a/courgette/disassembler_win32.cc b/courgette/disassembler_win32.cc
@@ -4,9 +4,6 @@
 
 #include "courgette/disassembler_win32.h"
 
-#include <stddef.h>
-#include <stdint.h>
-
 #include <algorithm>
 
 #include "base/bind.h"
@@ -224,26 +221,24 @@ bool DisassemblerWin32::ParseHeader() {
   return Good();
 }
 
-bool DisassemblerWin32::Disassemble(AssemblyProgram* target) {
+bool DisassemblerWin32::Disassemble(AssemblyProgram* program) {
   if (!ok())
     return false;
 
-  target->set_image_base(image_base());
-
   if (!ParseAbs32Relocs())
     return false;
 
   ParseRel32RelocsFromSections();
 
-  PrecomputeLabels(target);
-  RemoveUnusedRel32Locations(target);
+  PrecomputeLabels(program);
+  RemoveUnusedRel32Locations(program);
 
-  if (!target->GenerateInstructions(
+  if (!program->GenerateInstructions(
           base::Bind(&DisassemblerWin32::ParseFile, base::Unretained(this)))) {
     return false;
   }
 
-  target->DefaultAssignIndexes();
+  program->DefaultAssignIndexes();
   return true;
 }