Make disassembler tests abort on unimplemented opcode

This ensures that tests actually abort on unimplemented opcodes instead
of just printing them as "Unimplemented Instruction". If used to
disassemble a code region though, we want to ignore unimplemented
opcodes to keep printing remaining valid instructions.

The tests were previously fixed by Deepti in
8fa509d3, but this got partly reverted
on the "Address" refactoring in
2459046c.

R=titzer@chromium.org

Change-Id: I802dda2b0f45ee77c4f9b244ed984b1c4679bac3
Reviewed-on: https://chromium-review.googlesource.com/1146649
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54726}

src/arm/disasm-arm.cc

@@ -2693,13 +2693,6 @@ const char* NameConverter::NameInCode(byte* addr) const {
 
 //------------------------------------------------------------------------------
 
-Disassembler::Disassembler(const NameConverter& converter)
-    : converter_(converter) {}
-
-
-Disassembler::~Disassembler() {}
-
-
 int Disassembler::InstructionDecode(v8::internal::Vector<char> buffer,
                                     byte* instruction) {
   v8::internal::Decoder d(converter_, buffer);
@@ -2711,10 +2704,10 @@ int Disassembler::ConstantPoolSizeAt(byte* instruction) {
   return v8::internal::Decoder::ConstantPoolSizeAt(instruction);
 }
 
-
-void Disassembler::Disassemble(FILE* f, byte* begin, byte* end) {
+void Disassembler::Disassemble(FILE* f, byte* begin, byte* end,
+                               UnimplementedOpcodeAction unimplemented_action) {
   NameConverter converter;
-  Disassembler d(converter);
+  Disassembler d(converter, unimplemented_action);
   for (byte* pc = begin; pc < end;) {
     v8::internal::EmbeddedVector<char, 128> buffer;
     buffer[0] = '\0';
@@ -2725,7 +2718,6 @@ void Disassembler::Disassemble(FILE* f, byte* begin, byte* end) {
   }
 }
 
-
 }  // namespace disasm
 
 #endif  // V8_TARGET_ARCH_ARM

src/arm64/disasm-arm64.cc

@@ -4106,15 +4106,9 @@ class BufferDisassembler : public v8::internal::DisassemblingDecoder {
   v8::internal::Vector<char> out_buffer_;
 };
 
-Disassembler::Disassembler(const NameConverter& converter)
-    : converter_(converter) {}
-
-
-Disassembler::~Disassembler() { USE(converter_); }
-
-
 int Disassembler::InstructionDecode(v8::internal::Vector<char> buffer,
                                     byte* instr) {
+  USE(converter_);  // avoid unused field warning
   v8::internal::Decoder<v8::internal::DispatchingDecoderVisitor> decoder;
   BufferDisassembler disasm(buffer);
   decoder.AppendVisitor(&disasm);
@@ -4129,8 +4123,8 @@ int Disassembler::ConstantPoolSizeAt(byte* instr) {
       reinterpret_cast<v8::internal::Instruction*>(instr));
 }
 
-
-void Disassembler::Disassemble(FILE* file, byte* start, byte* end) {
+void Disassembler::Disassemble(FILE* file, byte* start, byte* end,
+                               UnimplementedOpcodeAction) {
   v8::internal::Decoder<v8::internal::DispatchingDecoderVisitor> decoder;
   v8::internal::PrintDisassembler disasm(file);
   decoder.AppendVisitor(&disasm);

src/disasm.h

@@ -32,32 +32,39 @@ class NameConverter {
 // A generic Disassembler interface
 class Disassembler {
  public:
+  enum UnimplementedOpcodeAction : int8_t {
+    kContinueOnUnimplementedOpcode,
+    kAbortOnUnimplementedOpcode
+  };
+
   // Caller deallocates converter.
-  explicit Disassembler(const NameConverter& converter);
+  explicit Disassembler(const NameConverter& converter,
+                        UnimplementedOpcodeAction unimplemented_opcode_action =
+                            kAbortOnUnimplementedOpcode)
+      : converter_(converter),
+        unimplemented_opcode_action_(unimplemented_opcode_action) {}
 
-  virtual ~Disassembler();
+  UnimplementedOpcodeAction unimplemented_opcode_action() const {
+    return unimplemented_opcode_action_;
+  }
 
   // Writes one disassembled instruction into 'buffer' (0-terminated).
   // Returns the length of the disassembled machine instruction in bytes.
   int InstructionDecode(v8::internal::Vector<char> buffer, byte* instruction);
 
-  // Disassemblers on ia32/x64 need a separate method for testing, as
-  // instruction decode method above continues on unimplemented opcodes, and
-  // does not test the disassemblers. Basic functionality of the method remains
-  // the same.
-  int InstructionDecodeForTesting(v8::internal::Vector<char> buffer,
-                                  byte* instruction);
-
   // Returns -1 if instruction does not mark the beginning of a constant pool,
   // or the number of entries in the constant pool beginning here.
   int ConstantPoolSizeAt(byte* instruction);
 
   // Write disassembly into specified file 'f' using specified NameConverter
   // (see constructor).
-  static void Disassemble(FILE* f, byte* begin, byte* end);
+  static void Disassemble(FILE* f, byte* begin, byte* end,
+                          UnimplementedOpcodeAction unimplemented_action =
+                              kAbortOnUnimplementedOpcode);
 
  private:
   const NameConverter& converter_;
+  const UnimplementedOpcodeAction unimplemented_opcode_action_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(Disassembler);
 };

src/disassembler.cc

@@ -175,7 +175,8 @@ static int DecodeIt(Isolate* isolate, ExternalReferenceEncoder* ref_encoder,
   v8::internal::EmbeddedVector<char, kOutBufferSize> out_buffer;
   StringBuilder out(out_buffer.start(), out_buffer.length());
   byte* pc = begin;
-  disasm::Disassembler d(converter);
+  disasm::Disassembler d(converter,
+                         disasm::Disassembler::kContinueOnUnimplementedOpcode);
   RelocIterator* it = nullptr;
   if (!converter.code().is_null()) {
     it = new RelocIterator(converter.code());

src/disassembler.h

@@ -16,6 +16,8 @@ class Disassembler : public AllStatic {
   // Decode instructions in the the interval [begin, end) and print the
   // code into os. Returns the number of bytes disassembled or 1 if no
   // instruction could be decoded.
+  // Does not abort on unimplemented opcodes, but prints them as 'Unimplemented
+  // Instruction'.
   // the code object is used for name resolution and may be null.
   // TODO(titzer): accept a {WasmCodeManager*} if {isolate} is null
   static int Decode(Isolate* isolate, std::ostream* os, byte* begin, byte* end,

src/ia32/disasm-ia32.cc

@@ -228,15 +228,16 @@ void InstructionTable::AddJumpConditionalShort() {
 // The IA32 disassembler implementation.
 class DisassemblerIA32 {
  public:
-  DisassemblerIA32(const NameConverter& converter,
-                   bool abort_on_unimplemented = true)
+  DisassemblerIA32(
+      const NameConverter& converter,
+      Disassembler::UnimplementedOpcodeAction unimplemented_opcode_action)
       : converter_(converter),
         vex_byte0_(0),
         vex_byte1_(0),
         vex_byte2_(0),
         instruction_table_(InstructionTable::get_instance()),
         tmp_buffer_pos_(0),
-        abort_on_unimplemented_(abort_on_unimplemented) {
+        unimplemented_opcode_action_(unimplemented_opcode_action) {
     tmp_buffer_[0] = '\0';
   }
 
@@ -254,7 +255,7 @@ class DisassemblerIA32 {
   InstructionTable* instruction_table_;
   v8::internal::EmbeddedVector<char, 128> tmp_buffer_;
   unsigned int tmp_buffer_pos_;
-  bool abort_on_unimplemented_;
+  Disassembler::UnimplementedOpcodeAction unimplemented_opcode_action_;
 
   enum {
     eax = 0,
@@ -392,8 +393,9 @@ class DisassemblerIA32 {
   PRINTF_FORMAT(2, 3) void AppendToBuffer(const char* format, ...);
 
   void UnimplementedInstruction() {
-    if (abort_on_unimplemented_) {
-      UNIMPLEMENTED();
+    if (unimplemented_opcode_action_ ==
+        Disassembler::kAbortOnUnimplementedOpcode) {
+      FATAL("Unimplemented instruction in disassembler");
     } else {
       AppendToBuffer("'Unimplemented Instruction'");
     }
@@ -2628,32 +2630,20 @@ const char* NameConverter::NameInCode(byte* addr) const {
 
 //------------------------------------------------------------------------------
 
-Disassembler::Disassembler(const NameConverter& converter)
-    : converter_(converter) {}
-
-
-Disassembler::~Disassembler() {}
-
-
 int Disassembler::InstructionDecode(v8::internal::Vector<char> buffer,
                                     byte* instruction) {
-  DisassemblerIA32 d(converter_, false /*do not crash if unimplemented*/);
-  return d.InstructionDecode(buffer, instruction);
-}
-
-int Disassembler::InstructionDecodeForTesting(v8::internal::Vector<char> buffer,
-                                              byte* instruction) {
-  DisassemblerIA32 d(converter_, true /*crash if unimplemented*/);
+  DisassemblerIA32 d(converter_, unimplemented_opcode_action());
   return d.InstructionDecode(buffer, instruction);
 }
 
 // The IA-32 assembler does not currently use constant pools.
 int Disassembler::ConstantPoolSizeAt(byte* instruction) { return -1; }
 
-
-/*static*/ void Disassembler::Disassemble(FILE* f, byte* begin, byte* end) {
+// static
+void Disassembler::Disassemble(FILE* f, byte* begin, byte* end,
+                               UnimplementedOpcodeAction unimplemented_action) {
   NameConverter converter;
-  Disassembler d(converter);
+  Disassembler d(converter, unimplemented_action);
   for (byte* pc = begin; pc < end;) {
     v8::internal::EmbeddedVector<char, 128> buffer;
     buffer[0] = '\0';

src/mips/disasm-mips.cc

@@ -2752,13 +2752,6 @@ const char* NameConverter::NameInCode(byte* addr) const {
 
 //------------------------------------------------------------------------------
 
-Disassembler::Disassembler(const NameConverter& converter)
-    : converter_(converter) {}
-
-
-Disassembler::~Disassembler() {}
-
-
 int Disassembler::InstructionDecode(v8::internal::Vector<char> buffer,
                                     byte* instruction) {
   v8::internal::Decoder d(converter_, buffer);
@@ -2771,10 +2764,10 @@ int Disassembler::ConstantPoolSizeAt(byte* instruction) {
   return -1;
 }
 
-
-void Disassembler::Disassemble(FILE* f, byte* begin, byte* end) {
+void Disassembler::Disassemble(FILE* f, byte* begin, byte* end,
+                               UnimplementedOpcodeAction unimplemented_action) {
   NameConverter converter;
-  Disassembler d(converter);
+  Disassembler d(converter, unimplemented_action);
   for (byte* pc = begin; pc < end;) {
     v8::internal::EmbeddedVector<char, 128> buffer;
     buffer[0] = '\0';

src/mips64/disasm-mips64.cc

@@ -3071,13 +3071,6 @@ const char* NameConverter::NameInCode(byte* addr) const {
 
 //------------------------------------------------------------------------------
 
-Disassembler::Disassembler(const NameConverter& converter)
-    : converter_(converter) {}
-
-
-Disassembler::~Disassembler() {}
-
-
 int Disassembler::InstructionDecode(v8::internal::Vector<char> buffer,
                                     byte* instruction) {
   v8::internal::Decoder d(converter_, buffer);
@@ -3090,10 +3083,10 @@ int Disassembler::ConstantPoolSizeAt(byte* instruction) {
   return -1;
 }
 
-
-void Disassembler::Disassemble(FILE* f, byte* begin, byte* end) {
+void Disassembler::Disassemble(FILE* f, byte* begin, byte* end,
+                               UnimplementedOpcodeAction unimplemented_action) {
   NameConverter converter;
-  Disassembler d(converter);
+  Disassembler d(converter, unimplemented_action);
   for (byte* pc = begin; pc < end;) {
     v8::internal::EmbeddedVector<char, 128> buffer;
     buffer[0] = '\0';

src/ppc/disasm-ppc.cc

@@ -1512,13 +1512,6 @@ const char* NameConverter::NameInCode(byte* addr) const {
 
 //------------------------------------------------------------------------------
 
-Disassembler::Disassembler(const NameConverter& converter)
-    : converter_(converter) {}
-
-
-Disassembler::~Disassembler() {}
-
-
 int Disassembler::InstructionDecode(v8::internal::Vector<char> buffer,
                                     byte* instruction) {
   v8::internal::Decoder d(converter_, buffer);
@@ -1529,10 +1522,10 @@ int Disassembler::InstructionDecode(v8::internal::Vector<char> buffer,
 // The PPC assembler does not currently use constant pools.
 int Disassembler::ConstantPoolSizeAt(byte* instruction) { return -1; }
 
-
-void Disassembler::Disassemble(FILE* f, byte* begin, byte* end) {
+void Disassembler::Disassemble(FILE* f, byte* begin, byte* end,
+                               UnimplementedOpcodeAction unimplemented_action) {
   NameConverter converter;
-  Disassembler d(converter);
+  Disassembler d(converter, unimplemented_action);
   for (byte* pc = begin; pc < end;) {
     v8::internal::EmbeddedVector<char, 128> buffer;
     buffer[0] = '\0';

src/s390/disasm-s390.cc

@@ -960,11 +960,6 @@ const char* NameConverter::NameInCode(byte* addr) const {
 
 //------------------------------------------------------------------------------
 
-Disassembler::Disassembler(const NameConverter& converter)
-    : converter_(converter) {}
-
-Disassembler::~Disassembler() {}
-
 int Disassembler::InstructionDecode(v8::internal::Vector<char> buffer,
                                     byte* instruction) {
   v8::internal::Decoder d(converter_, buffer);
@@ -974,9 +969,10 @@ int Disassembler::InstructionDecode(v8::internal::Vector<char> buffer,
 // The S390 assembler does not currently use constant pools.
 int Disassembler::ConstantPoolSizeAt(byte* instruction) { return -1; }
 
-void Disassembler::Disassemble(FILE* f, byte* begin, byte* end) {
+void Disassembler::Disassemble(FILE* f, byte* begin, byte* end,
+                               UnimplementedOpcodeAction unimplemented_action) {
   NameConverter converter;
-  Disassembler d(converter);
+  Disassembler d(converter, unimplemented_action);
   for (byte* pc = begin; pc < end;) {
     v8::internal::EmbeddedVector<char, 128> buffer;
     buffer[0] = '\0';

src/x64/disasm-x64.cc

@@ -277,23 +277,17 @@ static const InstructionDesc cmov_instructions[16] = {
 //------------------------------------------------------------------------------
 // DisassemblerX64 implementation.
 
-enum UnimplementedOpcodeAction {
-  CONTINUE_ON_UNIMPLEMENTED_OPCODE,
-  ABORT_ON_UNIMPLEMENTED_OPCODE
-};
-
 
 // A new DisassemblerX64 object is created to disassemble each instruction.
 // The object can only disassemble a single instruction.
 class DisassemblerX64 {
  public:
   DisassemblerX64(const NameConverter& converter,
-                  UnimplementedOpcodeAction unimplemented_action =
-                      ABORT_ON_UNIMPLEMENTED_OPCODE)
+                  Disassembler::UnimplementedOpcodeAction unimplemented_action)
       : converter_(converter),
         tmp_buffer_pos_(0),
         abort_on_unimplemented_(unimplemented_action ==
-                                ABORT_ON_UNIMPLEMENTED_OPCODE),
+                                Disassembler::kAbortOnUnimplementedOpcode),
         rex_(0),
         operand_size_(0),
         group_1_prefix_(0),
@@ -305,9 +299,6 @@ class DisassemblerX64 {
     tmp_buffer_[0] = '\0';
   }
 
-  virtual ~DisassemblerX64() {
-  }
-
   // Writes one disassembled instruction into 'buffer' (0-terminated).
   // Returns the length of the disassembled machine instruction in bytes.
   int InstructionDecode(v8::internal::Vector<char> buffer, byte* instruction);
@@ -2820,21 +2811,9 @@ const char* NameConverter::NameInCode(byte* addr) const {
 
 //------------------------------------------------------------------------------
 
-Disassembler::Disassembler(const NameConverter& converter)
-    : converter_(converter) { }
-
-Disassembler::~Disassembler() { }
-
-
 int Disassembler::InstructionDecode(v8::internal::Vector<char> buffer,
                                     byte* instruction) {
-  DisassemblerX64 d(converter_, CONTINUE_ON_UNIMPLEMENTED_OPCODE);
-  return d.InstructionDecode(buffer, instruction);
-}
-
-int Disassembler::InstructionDecodeForTesting(v8::internal::Vector<char> buffer,
-                                              byte* instruction) {
-  DisassemblerX64 d(converter_, ABORT_ON_UNIMPLEMENTED_OPCODE);
+  DisassemblerX64 d(converter_, unimplemented_opcode_action());
   return d.InstructionDecode(buffer, instruction);
 }
 
@@ -2843,10 +2822,10 @@ int Disassembler::ConstantPoolSizeAt(byte* instruction) {
   return -1;
 }
 
-
-void Disassembler::Disassemble(FILE* f, byte* begin, byte* end) {
+void Disassembler::Disassemble(FILE* f, byte* begin, byte* end,
+                               UnimplementedOpcodeAction unimplemented_action) {
   NameConverter converter;
-  Disassembler d(converter);
+  Disassembler d(converter, unimplemented_action);
   for (byte* pc = begin; pc < end;) {
     v8::internal::EmbeddedVector<char, 128> buffer;
     buffer[0] = '\0';