Backport the changes from the readability review.

Review URL: http://codereview.chromium.org/8939

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@700 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/constants-arm.h

@@ -33,62 +33,74 @@ namespace assembler { namespace arm {
 // Defines constants and accessor classes to assemble, disassemble and
 // simulate ARM instructions.
 //
+// Section references in the code refer to the "ARM Architecture Reference
+// Manual" from July 2005 (available at http://www.arm.com/miscPDFs/14128.pdf)
+//
 // Constants for specific fields are defined in their respective named enums.
 // General constants are in an anonymous enum in class Instr.
 
 typedef unsigned char byte;
 
+// Values for the condition field as defined in section A3.2
 enum Condition {
   no_condition = -1,
-  EQ =  0,
-  NE =  1,
-  CS =  2,
-  CC =  3,
-  MI =  4,
-  PL =  5,
-  VS =  6,
-  VC =  7,
-  HI =  8,
-  LS =  9,
-  GE = 10,
-  LT = 11,
-  GT = 12,
-  LE = 13,
-  AL = 14,
-  special_condition = 15
+  EQ =  0,  // equal
+  NE =  1,  // not equal
+  CS =  2,  // carry set/unsigned higher or same
+  CC =  3,  // carry clear/unsigned lower
+  MI =  4,  // minus/negative
+  PL =  5,  // plus/positive or zero
+  VS =  6,  // overflow
+  VC =  7,  // no overflow
+  HI =  8,  // unsigned higher
+  LS =  9,  // unsigned lower or same
+  GE = 10,  // signed greater than or equal
+  LT = 11,  // signed less than
+  GT = 12,  // signed greater than
+  LE = 13,  // signed less than or equal
+  AL = 14,  // always (unconditional)
+  special_condition = 15,  // special condition (refer to section A3.2.1)
+  max_condition = 16
 };
 
 
+// Opcodes for Data-processing instructions (instructions with a type 0 and 1)
+// as defined in section A3.4
 enum Opcode {
   no_operand = -1,
-  AND =  0,
-  EOR =  1,
-  SUB =  2,
-  RSB =  3,
-  ADD =  4,
-  ADC =  5,
-  SBC =  6,
-  RSC =  7,
-  TST =  8,
-  TEQ =  9,
-  CMP = 10,
-  CMN = 11,
-  ORR = 12,
-  MOV = 13,
-  BIC = 14,
-  MVN = 15
+  AND =  0,  // Logical AND
+  EOR =  1,  // Logical Exclusive OR
+  SUB =  2,  // Subtract
+  RSB =  3,  // Reverse Subtract
+  ADD =  4,  // Add
+  ADC =  5,  // Add with Carry
+  SBC =  6,  // Subtract with Carry
+  RSC =  7,  // Reverse Subtract with Carry
+  TST =  8,  // Test
+  TEQ =  9,  // Test Equivalence
+  CMP = 10,  // Compare
+  CMN = 11,  // Compare Negated
+  ORR = 12,  // Logical (inclusive) OR
+  MOV = 13,  // Move
+  BIC = 14,  // Bit Clear
+  MVN = 15,  // Move Not
+  max_operand = 16
 };
 
 
+// Shifter types for Data-processing operands as defined in section A5.1.2.
 enum Shift {
   no_shift = -1,
-  LSL = 0,
-  LSR = 1,
-  ASR = 2,
-  ROR = 3
+  LSL = 0,  // Logical shift left
+  LSR = 1,  // Logical shift right
+  ASR = 2,  // Arithmetic shift right
+  ROR = 3,  // Rotate right
+  max_shift = 4
 };
 
 
+// Special Software Interrupt codes when used in the presence of the ARM
+// simulator.
 enum SoftwareInterruptCodes {
   // transition to C code
   call_rt_r5 = 0x10,
@@ -102,7 +114,17 @@ typedef int32_t instr_t;
 
 
 // The class Instr enables access to individual fields defined in the ARM
-// architecture.
+// architecture instruction set encoding as described in figure A3-1.
+//
+// Example: Test whether the instruction at ptr does set the condition code
+// bits.
+//
+// bool InstructionSetsConditionCodes(byte* ptr) {
+//   Instr *instr = Instr::At(ptr);
+//   int type = instr->TypeField();
+//   return ((type == 0) || (type == 1)) && instr->HasS();
+// }
+//
 class Instr {
  public:
   enum {
@@ -110,25 +132,29 @@ class Instr {
     kPCReadOffset = 8
   };
 
-  // Get the raw instruction bits
+  // Get the raw instruction bits.
   inline instr_t InstructionBits() const {
     return *reinterpret_cast<const instr_t*>(this);
   }
 
+  // Set the raw instruction bits to value.
   inline void SetInstructionBits(instr_t value) {
     *reinterpret_cast<instr_t*>(this) = value;
   }
 
+  // Read one particular bit out of the instruction bits.
   inline int Bit(int nr) const {
     return (InstructionBits() >> nr) & 1;
   }
 
+  // Read a bit field out of the instruction bits.
   inline int Bits(int hi, int lo) const {
     return (InstructionBits() >> lo) & ((2 << (hi - lo)) - 1);
   }
 
 
   // Accessors for the different named fields used in the ARM encoding.
+  // The naming of these accessor corresponds to figure A3-1.
   // Generally applicable fields
   inline Condition ConditionField() const {
     return static_cast<Condition>(Bits(31, 28));

src/disasm-ia32.cc

@@ -1095,11 +1095,6 @@ const char* NameConverter::NameInCode(byte* addr) const {
 
 //------------------------------------------------------------------------------
 
-static NameConverter defaultConverter;
-
-Disassembler::Disassembler() : converter_(defaultConverter) {}
-
-
 Disassembler::Disassembler(const NameConverter& converter)
     : converter_(converter) {}
 
@@ -1119,7 +1114,8 @@ int Disassembler::ConstantPoolSizeAt(byte* instruction) { return -1; }
 
 
 /*static*/ void Disassembler::Disassemble(FILE* f, byte* begin, byte* end) {
-  Disassembler d;
+  NameConverter converter;
+  Disassembler d(converter);
   for (byte* pc = begin; pc < end;) {
     v8::internal::EmbeddedVector<char, 128> buffer;
     buffer[0] = '\0';

src/disasm.h

@@ -49,9 +49,6 @@ class NameConverter {
 // A generic Disassembler interface
 class Disassembler {
  public:
-  // Uses default NameConverter.
-  Disassembler();
-
   // Caller deallocates converter.
   explicit Disassembler(const NameConverter& converter);
 
@@ -70,6 +67,8 @@ class Disassembler {
   static void Disassemble(FILE* f, byte* begin, byte* end);
  private:
   const NameConverter& converter_;
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(Disassembler);
 };
 
 }  // namespace disasm

src/globals.h

@@ -25,6 +25,9 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+#ifndef V8_GLOBALS_H_
+#define V8_GLOBALS_H_
+
 // -----------------------------------------------------------------------------
 // Types
 // Windows is missing the stdint.h header file. Instead we define standard
@@ -58,9 +61,6 @@ namespace v8 { namespace internal {
 // defined here because the platform code uses bool, and platform.h is
 // include very early in the main include file.
 
-#ifndef V8_GLOBALS_H_
-#define V8_GLOBALS_H_
-
 #ifdef USE_MYBOOL
 typedef unsigned int __my_bool__;
 #define bool __my_bool__  // use 'indirection' to avoid name clashes

src/simulator-arm.cc

@@ -210,7 +210,8 @@ void Debugger::Debug() {
 
   while (!done) {
     if (last_pc != sim_->get_pc()) {
-      disasm::Disassembler dasm;
+      disasm::NameConverter converter;
+      disasm::Disassembler dasm(converter);
       // use a reasonably large buffer
       v8::internal::EmbeddedVector<char, 256> buffer;
       dasm.InstructionDecode(buffer,
@@ -265,7 +266,8 @@ void Debugger::Debug() {
           PrintF("printobject value\n");
         }
       } else if (strcmp(cmd, "disasm") == 0) {
-        disasm::Disassembler dasm;
+        disasm::NameConverter converter;
+        disasm::Disassembler dasm(converter);
         // use a reasonably large buffer
         v8::internal::EmbeddedVector<char, 256> buffer;
 
@@ -1441,7 +1443,8 @@ void Simulator::InstructionDecode(Instr* instr) {
     return;
   }
   if (::v8::internal::FLAG_trace_sim) {
-    disasm::Disassembler dasm;
+    disasm::NameConverter converter;
+    disasm::Disassembler dasm(converter);
     // use a reasonably large buffer
     v8::internal::EmbeddedVector<char, 256> buffer;
     dasm.InstructionDecode(buffer,

src/v8.h

@@ -38,6 +38,10 @@
 // If both are defined in Google3, then we are building an optimized v8 with
 // assertions enabled.
 #undef NDEBUG
+#elif !defined(DEBUG) && !defined(NDEBUG)
+// If neither is defined in Google3, then we are building a debug v8. Mark it
+// as such.
+#define DEBUG
 #endif
 #endif  // defined(GOOGLE3)
 

test/cctest/test-disasm-arm.cc

@@ -50,7 +50,8 @@ static void InitializeVM() {
 
 
 bool DisassembleAndCompare(byte* pc, const char* compare_string) {
-  disasm::Disassembler disasm;
+  disasm::NameConverter converter;
+  disasm::Disassembler disasm(converter);
   EmbeddedVector<char, 128> disasm_buffer;
 
   disasm.InstructionDecode(disasm_buffer, pc);
@@ -68,6 +69,9 @@ bool DisassembleAndCompare(byte* pc, const char* compare_string) {
 }
 
 
+// Setup V8 to a state where we can at least run the assembler and
+// disassembler. Declare the variables and allocate the data structures used
+// in the rest of the macros.
 #define SETUP() \
   InitializeVM(); \
   Serializer::disable(); \
@@ -77,6 +81,10 @@ bool DisassembleAndCompare(byte* pc, const char* compare_string) {
   bool failure = false;
 
 
+// This macro assembles one instruction using the preallocated assembler and
+// disassembles the generated instruction, comparing the output to the expected
+// value. If the comparison fails an error message is printed, but the test
+// continues to run until the end.
 #define COMPARE(asm_, compare_string) \
   { \
     int pc_offset = assm.pc_offset(); \
@@ -86,8 +94,10 @@ bool DisassembleAndCompare(byte* pc, const char* compare_string) {
   }
 
 
-#define OUTPUT() \
-  if (failure) { \
+// Verify that all invocations of the COMPARE macro passed successfully.
+// Exit with a failure if at least one of the tests failed.
+#define VERIFY_RUN() \
+if (failure) { \
     V8_Fatal(__FILE__, __LINE__, "ARM Disassembler tests failed.\n"); \
   }
 
@@ -239,7 +249,7 @@ TEST(Type0) {
   COMPARE(mvn(r5, Operand(r4), SetCC, cc),
           "31f05004       mvnccs r5, r4");
 
-  OUTPUT();
+  VERIFY_RUN();
 }
 
 
@@ -268,5 +278,5 @@ TEST(Type1) {
   COMPARE(eor(r4, r1, Operand(0x10000000), SetCC, cc),
           "32314201       eorccs r4, r1, #268435456");
 
-  OUTPUT();
+  VERIFY_RUN();
 }