[Courgette] Refactor: Manage AssemblyProgram and EncodedProgram with …

…scoped_ptr.

Previously naked pointers AssemblyProgram and EncodedProgram are used over the
place, and are deallocated using Delete{AssemblyProgram, EncodedProgram}().
In this CL we use scoped_ptr to manage the life cycles of these objects.

- Removed DeleteAssemblyProgram() and DeleteEncodedProgram() and replaced calls
  with e.g., program.reset(nullptr); if the manual deallocation is a peak
  memory optimization.
- Moved Encode() and ReadEncodedProgram() to the .h files matching the .cc files.
- Extracted DetectExecutableType() and ParseDetectedExecutable() from
  disassembly.* to new files program_detector*c, since Disassembly is really an
  implementation that caller's don't care about.

Committed: https://crrev.com/0a9cbf1781a114b35a4e0f4a834f2d24ade2e917
Cr-Commit-Position: refs/heads/master@{#372212}

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

Cr-Commit-Position: refs/heads/master@{#372436}

courgette/BUILD.gn

@@ -42,6 +42,8 @@ static_library("courgette_lib") {
     "memory_allocator.h",
     "patch_generator_x86_32.h",
     "patcher_x86_32.h",
+    "program_detector.cc",
+    "program_detector.h",
     "region.h",
     "rel32_finder_win32_x86.cc",
     "rel32_finder_win32_x86.h",

courgette/adjustment_method_unittest.cc

@@ -3,11 +3,13 @@
 // found in the LICENSE file.
 
 #include <string>
+#include <utility>
 
+#include "base/memory/scoped_ptr.h"
 #include "base/strings/string_util.h"
-
 #include "courgette/assembly_program.h"
 #include "courgette/courgette.h"
+#include "courgette/encoded_program.h"
 #include "courgette/streams.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
@@ -23,11 +25,11 @@ class AdjustmentMethodTest : public testing::Test {
   void TearDown() {
   }
 
-  // Returns one of two similar a simple programs.  They differ only in the
-  // label assignment, so that it is possible to make them look identical.
-  courgette::AssemblyProgram* MakeProgram(int kind) const {
-    courgette::AssemblyProgram* prog =
-      new courgette::AssemblyProgram(courgette::EXE_WIN_32_X86);
+  // Returns one of two similar simple programs. These differ only in Label
+  // assignment, so it is possible to make them look identical.
+  scoped_ptr<courgette::AssemblyProgram> MakeProgram(int kind) const {
+    scoped_ptr<courgette::AssemblyProgram> prog(
+      new courgette::AssemblyProgram(courgette::EXE_WIN_32_X86));
     prog->set_image_base(0x00400000);
 
     courgette::Label* labelA = prog->FindOrMakeAbs32Label(0x00410000);
@@ -52,24 +54,29 @@ class AdjustmentMethodTest : public testing::Test {
     return prog;
   }
 
-  courgette::AssemblyProgram* MakeProgramA() const { return MakeProgram(0); }
-  courgette::AssemblyProgram* MakeProgramB() const { return MakeProgram(1); }
+  scoped_ptr<courgette::AssemblyProgram> MakeProgramA() const {
+    return MakeProgram(0);
+  }
+  scoped_ptr<courgette::AssemblyProgram> MakeProgramB() const {
+    return MakeProgram(1);
+  }
 
   // Returns a string that is the serialized version of |program|.
   // Deletes |program|.
-  std::string Serialize(courgette::AssemblyProgram *program) const {
-    courgette::EncodedProgram* encoded = NULL;
+  std::string Serialize(scoped_ptr<courgette::AssemblyProgram> program) const {
+    scoped_ptr<courgette::EncodedProgram> encoded;
 
-    const courgette::Status encode_status = Encode(program, &encoded);
+    const courgette::Status encode_status = Encode(*program, &encoded);
     EXPECT_EQ(courgette::C_OK, encode_status);
 
-    DeleteAssemblyProgram(program);
+    program.reset();
 
     courgette::SinkStreamSet sinks;
-    const courgette::Status write_status = WriteEncodedProgram(encoded, &sinks);
+    const courgette::Status write_status =
+        WriteEncodedProgram(encoded.get(), &sinks);
     EXPECT_EQ(courgette::C_OK, write_status);
 
-    DeleteEncodedProgram(encoded);
+    encoded.reset();
 
     courgette::SinkStream sink;
     bool can_collect = sinks.CopyTo(&sink);
@@ -82,20 +89,20 @@ class AdjustmentMethodTest : public testing::Test {
 
 
 void AdjustmentMethodTest::Test1() const {
-  courgette::AssemblyProgram* prog1 = MakeProgramA();
-  courgette::AssemblyProgram* prog2 = MakeProgramB();
-  std::string s1 = Serialize(prog1);
-  std::string s2 = Serialize(prog2);
+  scoped_ptr<courgette::AssemblyProgram> prog1 = MakeProgramA();
+  scoped_ptr<courgette::AssemblyProgram> prog2 = MakeProgramB();
+  std::string s1 = Serialize(std::move(prog1));
+  std::string s2 = Serialize(std::move(prog2));
 
   // Don't use EXPECT_EQ because strings are unprintable.
   EXPECT_FALSE(s1 == s2);  // Unadjusted A and B differ.
 
-  courgette::AssemblyProgram* prog5 = MakeProgramA();
-  courgette::AssemblyProgram* prog6 = MakeProgramB();
-  courgette::Status can_adjust = Adjust(*prog5, prog6);
+  scoped_ptr<courgette::AssemblyProgram> prog5 = MakeProgramA();
+  scoped_ptr<courgette::AssemblyProgram> prog6 = MakeProgramB();
+  courgette::Status can_adjust = Adjust(*prog5, prog6.get());
   EXPECT_EQ(courgette::C_OK, can_adjust);
-  std::string s5 = Serialize(prog5);
-  std::string s6 = Serialize(prog6);
+  std::string s5 = Serialize(std::move(prog5));
+  std::string s6 = Serialize(std::move(prog6));
 
   EXPECT_TRUE(s1 == s5);  // Adjustment did not change A (prog5)
   EXPECT_TRUE(s5 == s6);  // Adjustment did change B into A

courgette/assembly_program.cc

@@ -7,16 +7,12 @@
 #include <memory.h>
 #include <stddef.h>
 #include <stdint.h>
-#include <algorithm>
-#include <map>
-#include <set>
-#include <sstream>
+
+#include <utility>
 #include <vector>
 
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/memory/scoped_ptr.h"
-
 #include "courgette/courgette.h"
 #include "courgette/encoded_program.h"
 
@@ -365,13 +361,13 @@ void AssemblyProgram::AssignRemainingIndexes(RVAToLabel* labels) {
           << "  infill " << fill_infill_count;
 }
 
-EncodedProgram* AssemblyProgram::Encode() const {
+scoped_ptr<EncodedProgram> AssemblyProgram::Encode() const {
   scoped_ptr<EncodedProgram> encoded(new EncodedProgram());
 
   encoded->set_image_base(image_base_);
 
   if (!encoded->DefineLabels(abs32_labels_, rel32_labels_))
-    return NULL;
+    return nullptr;
 
   for (size_t i = 0;  i < instructions_.size();  ++i) {
     Instruction* instruction = instructions_[i];
@@ -380,13 +376,13 @@ EncodedProgram* AssemblyProgram::Encode() const {
       case ORIGIN: {
         OriginInstruction* org = static_cast<OriginInstruction*>(instruction);
         if (!encoded->AddOrigin(org->origin_rva()))
-          return NULL;
+          return nullptr;
         break;
       }
       case DEFBYTE: {
         uint8_t b = static_cast<ByteInstruction*>(instruction)->byte_value();
         if (!encoded->AddCopy(1, &b))
-          return NULL;
+          return nullptr;
         break;
       }
       case DEFBYTES: {
@@ -395,13 +391,13 @@ EncodedProgram* AssemblyProgram::Encode() const {
         size_t len = static_cast<BytesInstruction*>(instruction)->len();
 
         if (!encoded->AddCopy(len, byte_values))
-          return NULL;
+          return nullptr;
         break;
       }
       case REL32: {
         Label* label = static_cast<InstructionWithLabel*>(instruction)->label();
         if (!encoded->AddRel32(label->index_))
-          return NULL;
+          return nullptr;
         break;
       }
       case REL32ARM: {
@@ -410,34 +406,34 @@ EncodedProgram* AssemblyProgram::Encode() const {
         uint16_t compressed_op =
             static_cast<InstructionWithLabelARM*>(instruction)->compressed_op();
         if (!encoded->AddRel32ARM(compressed_op, label->index_))
-          return NULL;
+          return nullptr;
         break;
       }
       case ABS32: {
         Label* label = static_cast<InstructionWithLabel*>(instruction)->label();
         if (!encoded->AddAbs32(label->index_))
-          return NULL;
+          return nullptr;
         break;
       }
       case ABS64: {
         Label* label = static_cast<InstructionWithLabel*>(instruction)->label();
         if (!encoded->AddAbs64(label->index_))
-          return NULL;
+          return nullptr;
         break;
       }
       case MAKEPERELOCS: {
         if (!encoded->AddPeMakeRelocs(kind_))
-          return NULL;
+          return nullptr;
         break;
       }
       case MAKEELFRELOCS: {
         if (!encoded->AddElfMakeRelocs())
-          return NULL;
+          return nullptr;
         break;
       }
       case MAKEELFARMRELOCS: {
         if (!encoded->AddElfARMMakeRelocs())
-          return NULL;
+          return nullptr;
         break;
       }
       default: {
@@ -446,7 +442,7 @@ EncodedProgram* AssemblyProgram::Encode() const {
     }
   }
 
-  return encoded.release();
+  return encoded;
 }
 
 Instruction* AssemblyProgram::GetByteInstruction(uint8_t byte) {
@@ -530,15 +526,13 @@ CheckBool AssemblyProgram::TrimLabels() {
 
 ////////////////////////////////////////////////////////////////////////////////
 
-Status Encode(AssemblyProgram* program, EncodedProgram** output) {
-  *output = NULL;
-  EncodedProgram *encoded = program->Encode();
-  if (encoded) {
-    *output = encoded;
-    return C_OK;
-  } else {
-    return C_GENERAL_ERROR;
-  }
+Status Encode(const AssemblyProgram& program,
+              scoped_ptr<EncodedProgram>* output) {
+  // Explicitly release any memory associated with the output before encoding.
+  output->reset();
+
+  *output = program.Encode();
+  return (*output) ? C_OK : C_GENERAL_ERROR;
 }
 
 }  // namespace courgette

courgette/assembly_program.h

@@ -14,7 +14,7 @@
 
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
-#include "courgette/disassembler.h"
+#include "courgette/courgette.h"
 #include "courgette/image_utils.h"
 #include "courgette/label_manager.h"
 #include "courgette/memory_allocator.h"
@@ -132,7 +132,7 @@ class AssemblyProgram {
   void UnassignIndexes();
   void AssignRemainingIndexes();
 
-  EncodedProgram* Encode() const;
+  scoped_ptr<EncodedProgram> Encode() const;
 
   // Accessor for instruction list.
   const InstructionVector& instructions() const {
@@ -191,5 +191,12 @@ class AssemblyProgram {
   DISALLOW_COPY_AND_ASSIGN(AssemblyProgram);
 };
 
+// Converts |program| into encoded form, returning it as |*output|.
+// Returns C_OK if succeeded, otherwise returns an error status and sets
+// |*output| to null.
+Status Encode(const AssemblyProgram& program,
+              scoped_ptr<EncodedProgram>* output);
+
 }  // namespace courgette
+
 #endif  // COURGETTE_ASSEMBLY_PROGRAM_H_

courgette/courgette.gyp

@@ -40,6 +40,8 @@
       'label_manager.h',
       'memory_allocator.cc',
       'memory_allocator.h',
+      'program_detector.cc',
+      'program_detector.h',
       'region.h',
       'rel32_finder_win32_x86.cc',
       'rel32_finder_win32_x86.h',

courgette/courgette.h

@@ -90,30 +90,6 @@ Status ApplyEnsemblePatch(const base::FilePath::CharType* old_file_name,
 Status GenerateEnsemblePatch(SourceStream* old, SourceStream* target,
                              SinkStream* patch);
 
-// Detects the type of an executable file, and it's length. The length
-// may be slightly smaller than some executables (like ELF), but will include
-// all bytes the courgette algorithm has special benefit for.
-// On success:
-//   Fill in type and detected_length, and return C_OK.
-// On failure:
-//   Fill in type with UNKNOWN, detected_length with 0, and
-//   return C_INPUT_NOT_RECOGNIZED
-Status DetectExecutableType(const void* buffer, size_t length,
-                            ExecutableType* type,
-                            size_t* detected_length);
-
-// Attempts to detect the type of executable, and parse it with the
-// appropriate tools, storing the pointer to the AssemblyProgram in |*output|.
-// Returns C_OK if successful, otherwise returns an error status and sets
-// |*output| to NULL.
-Status ParseDetectedExecutable(const void* buffer, size_t length,
-                               AssemblyProgram** output);
-
-// Converts |program| into encoded form, returning it as |*output|.
-// Returns C_OK if succeeded, otherwise returns an error status and
-// sets |*output| to NULL
-Status Encode(AssemblyProgram* program, EncodedProgram** output);
-
 // Serializes |encoded| into the stream set.
 // Returns C_OK if succeeded, otherwise returns an error status.
 Status WriteEncodedProgram(EncodedProgram* encoded, SinkStreamSet* sink);
@@ -123,20 +99,10 @@ Status WriteEncodedProgram(EncodedProgram* encoded, SinkStreamSet* sink);
 // |buffer| in an undefined state.
 Status Assemble(EncodedProgram* encoded, SinkStream* buffer);
 
-// Deserializes program from the stream set.
-// Returns C_OK if succeeded, otherwise returns an error status and
-// sets |*output| to NULL
-Status ReadEncodedProgram(SourceStreamSet* source, EncodedProgram** output);
-
-// Used to free an AssemblyProgram returned by other APIs.
-void DeleteAssemblyProgram(AssemblyProgram* program);
-
-// Used to free an EncodedProgram returned by other APIs.
-void DeleteEncodedProgram(EncodedProgram* encoded);
-
 // Adjusts |program| to look more like |model|.
 //
 Status Adjust(const AssemblyProgram& model, AssemblyProgram *program);
 
 }  // namespace courgette
+
 #endif  // COURGETTE_COURGETTE_H_