Explicitly clear the MCInst operand list when calling the target disassembler. Otherwise, repeatedly passing in the same MCInst to getInstruction() will append the current instruction's operands to the previous instruction's operands.

[dirty]

Summary

getInstruction() in the target disassembler will explicitly clear the MCInst operand list. Some targets were already doing this, but not all. Without this change, repeatedly passing in the same MCInst to getInstruction() will append the current instruction's operands to the previous instruction's operands.

[uweigand]

Wouldn't it be preferable to do the MI.clear() *within* decodeInstruction? It seems odd to define the interface so that every caller must call clear() itself ...

[dirty]

In D11665#215891, @uweigand wrote:

Wouldn't it be preferable to do the MI.clear() *within* decodeInstruction? It seems odd to define the interface so that every caller must call clear() itself ...

That sounds reasonable. Not every implementation of getInstruction() calls decodeInstruction(), but I can explicitly call MI.clear() in those cases. The only issue is that decodeInstruction() is an auto generated routine.

I'll update my patch.

[dirty]

Revert previous patch implementation. Now, decodeInstruction() calls MI.clear().

[uweigand]

In general, this looks good to me. As a stylistic preference, I'd rather place the .clear() calls closer to the initial .setOpcode() calls to keep the sequences that fill in the MCInst close together; see below.

lib/Target/X86/Disassembler/X86Disassembler.cpp

151 ↗

(On Diff #31167)

I think this can be pushed down into translateInstruction().

utils/TableGen/FixedLenDecoderEmitter.cpp

2093 ↗

(On Diff #31167)

And this can be pushed down into the MCD::OPC_Decode case. (In the MCD::OPC_TryDecode case, we're using a temporary anyway.)

[dirty]

• Integrate uweigand's feedback: move MI.clear() into MCD::OPC_Decode case and call MI.clear() inside of translateInstruction() for the X86 target.

[uweigand]

Looks good to me, thanks.

[kparzysz]

LGTM

[dirty]

Anyone else have comments or should I go ahead and commit this?

[hfinkel]

LGTM too, please go ahead.

[dirty]

Ping.

llvm/trunk/lib/Target/ARM/Disassembler/ARMDisassembler.cpp

DecodeStatus Result =
       decodeInstruction(DecoderTableARM32, MI, Insn, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 4;
     return checkDecodedInstruction(MI, Size, Address, OS, CS, Insn, Result);
   }
 
   // VFP and NEON instructions, similarly, are shared between ARM
   // and Thumb modes.
-  MI.clear();
   Result = decodeInstruction(DecoderTableVFP32, MI, Insn, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 4;
     return Result;
   }
 
-  MI.clear();
   Result = decodeInstruction(DecoderTableVFPV832, MI, Insn, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 4;
     return Result;
   }
 
-  MI.clear();
   Result =
       decodeInstruction(DecoderTableNEONData32, MI, Insn, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 4;
     // Add a fake predicate operand, because we share these instruction
     // definitions with Thumb2 where these instructions are predicable.
     if (!DecodePredicateOperand(MI, 0xE, Address, this))
       return MCDisassembler::Fail;
     return Result;
   }
 
-  MI.clear();
   Result = decodeInstruction(DecoderTableNEONLoadStore32, MI, Insn, Address,
                              this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 4;
     // Add a fake predicate operand, because we share these instruction
     // definitions with Thumb2 where these instructions are predicable.
     if (!DecodePredicateOperand(MI, 0xE, Address, this))
       return MCDisassembler::Fail;
     return Result;
   }
 
-  MI.clear();
   Result =
       decodeInstruction(DecoderTableNEONDup32, MI, Insn, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 4;
     // Add a fake predicate operand, because we share these instruction
     // definitions with Thumb2 where these instructions are predicable.
     if (!DecodePredicateOperand(MI, 0xE, Address, this))
       return MCDisassembler::Fail;
     return Result;
   }
 
-  MI.clear();
   Result =
       decodeInstruction(DecoderTablev8NEON32, MI, Insn, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 4;
     return Result;
   }
 
-  MI.clear();
   Result =
       decodeInstruction(DecoderTablev8Crypto32, MI, Insn, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 4;
     return Result;
   }
 
-  MI.clear();
   Size = 0;
   return MCDisassembler::Fail;
 }
 
 namespace llvm {
 extern const MCInstrDesc ARMInsts[];
 }
 
DecodeStatus ThumbDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
   DecodeStatus Result =
       decodeInstruction(DecoderTableThumb16, MI, Insn16, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 2;
     Check(Result, AddThumbPredicate(MI));
     return Result;
   }
 
-  MI.clear();
   Result = decodeInstruction(DecoderTableThumbSBit16, MI, Insn16, Address, this,
                              STI);
   if (Result) {
     Size = 2;
     bool InITBlock = ITBlock.instrInITBlock();
     Check(Result, AddThumbPredicate(MI));
     AddThumb1SBit(MI, InITBlock);
     return Result;
   }
 
-  MI.clear();
   Result =
       decodeInstruction(DecoderTableThumb216, MI, Insn16, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 2;
 
     // Nested IT blocks are UNPREDICTABLE.  Must be checked before we add
     // the Thumb predicate.
     if (MI.getOpcode() == ARM::t2IT && ITBlock.instrInITBlock())
DecodeStatus ThumbDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
   // We want to read exactly 4 bytes of data.
   if (Bytes.size() < 4) {
     Size = 0;
     return MCDisassembler::Fail;
   }
 
   uint32_t Insn32 =
       (Bytes[3] << 8) | (Bytes[2] << 0) | (Bytes[1] << 24) | (Bytes[0] << 16);
-  MI.clear();
   Result =
       decodeInstruction(DecoderTableThumb32, MI, Insn32, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 4;
     bool InITBlock = ITBlock.instrInITBlock();
     Check(Result, AddThumbPredicate(MI));
     AddThumb1SBit(MI, InITBlock);
     return Result;
   }
 
-  MI.clear();
   Result =
       decodeInstruction(DecoderTableThumb232, MI, Insn32, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 4;
     Check(Result, AddThumbPredicate(MI));
     return Result;
   }
 
   if (fieldFromInstruction(Insn32, 28, 4) == 0xE) {
-    MI.clear();
     Result =
         decodeInstruction(DecoderTableVFP32, MI, Insn32, Address, this, STI);
     if (Result != MCDisassembler::Fail) {
       Size = 4;
       UpdateThumbVFPPredicate(MI);
       return Result;
     }
   }
 
-  MI.clear();
   Result =
       decodeInstruction(DecoderTableVFPV832, MI, Insn32, Address, this, STI);
   if (Result != MCDisassembler::Fail) {
     Size = 4;
     return Result;
   }
 
   if (fieldFromInstruction(Insn32, 28, 4) == 0xE) {
-    MI.clear();
     Result = decodeInstruction(DecoderTableNEONDup32, MI, Insn32, Address, this,
                                STI);
     if (Result != MCDisassembler::Fail) {
       Size = 4;
       Check(Result, AddThumbPredicate(MI));
       return Result;
     }
   }
 
   if (fieldFromInstruction(Insn32, 24, 8) == 0xF9) {
-    MI.clear();
     uint32_t NEONLdStInsn = Insn32;
     NEONLdStInsn &= 0xF0FFFFFF;
     NEONLdStInsn |= 0x04000000;
     Result = decodeInstruction(DecoderTableNEONLoadStore32, MI, NEONLdStInsn,
                                Address, this, STI);
     if (Result != MCDisassembler::Fail) {
       Size = 4;
       Check(Result, AddThumbPredicate(MI));
       return Result;
     }
   }
 
   if (fieldFromInstruction(Insn32, 24, 4) == 0xF) {
-    MI.clear();
     uint32_t NEONDataInsn = Insn32;
     NEONDataInsn &= 0xF0FFFFFF; // Clear bits 27-24
     NEONDataInsn |= (NEONDataInsn & 0x10000000) >> 4; // Move bit 28 to bit 24
     NEONDataInsn |= 0x12000000; // Set bits 28 and 25
     Result = decodeInstruction(DecoderTableNEONData32, MI, NEONDataInsn,
                                Address, this, STI);
     if (Result != MCDisassembler::Fail) {
       Size = 4;
       Check(Result, AddThumbPredicate(MI));
       return Result;
     }
 
-    MI.clear();
     uint32_t NEONCryptoInsn = Insn32;
     NEONCryptoInsn &= 0xF0FFFFFF; // Clear bits 27-24
     NEONCryptoInsn |= (NEONCryptoInsn & 0x10000000) >> 4; // Move bit 28 to bit 24
     NEONCryptoInsn |= 0x12000000; // Set bits 28 and 25
     Result = decodeInstruction(DecoderTablev8Crypto32, MI, NEONCryptoInsn,
                                Address, this, STI);
     if (Result != MCDisassembler::Fail) {
       Size = 4;
       return Result;
     }
 
-    MI.clear();
     uint32_t NEONv8Insn = Insn32;
     NEONv8Insn &= 0xF3FFFFFF; // Clear bits 27-26
     Result = decodeInstruction(DecoderTablev8NEON32, MI, NEONv8Insn, Address,
                                this, STI);
     if (Result != MCDisassembler::Fail) {
       Size = 4;
       return Result;
     }
   }
 
-  MI.clear();
   Size = 0;
   return MCDisassembler::Fail;
 }
 
 
 extern "C" void LLVMInitializeARMDisassembler() {
   TargetRegistry::RegisterMCDisassembler(TheARMLETarget,
                                          createARMDisassembler);

llvm/trunk/lib/Target/Hexagon/Disassembler/HexagonDisassembler.cpp

DecodeStatus HexagonDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
                                                  uint64_t Address,
                                                  raw_ostream &os,
                                                  raw_ostream &cs) const {
   DecodeStatus Result = DecodeStatus::Success;
   bool Complete = false;
   Size = 0;
 
   *CurrentBundle = &MI;
+  MI.clear();
   MI.setOpcode(Hexagon::BUNDLE);
   MI.addOperand(MCOperand::createImm(0));
   while (Result == Success && Complete == false) {
     if (Bytes.size() < HEXAGON_INSTR_SIZE)
       return MCDisassembler::Fail;
     MCInst *Inst = new (getContext()) MCInst;
     Result = getSingleInstruction(*Inst, MI, Bytes, Address, os, cs, Complete);
     MI.addOperand(MCOperand::createInst(Inst));

llvm/trunk/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp

DecodeStatus PPCDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
   uint32_t Inst = IsLittleEndian ? support::endian::read32le(Bytes.data())
                                  : support::endian::read32be(Bytes.data());
 
   if (STI.getFeatureBits()[PPC::FeatureQPX]) {
     DecodeStatus result =
       decodeInstruction(DecoderTableQPX32, MI, Inst, Address, this, STI);
     if (result != MCDisassembler::Fail)
       return result;
-
-    MI.clear();
   }
 
   return decodeInstruction(DecoderTable32, MI, Inst, Address, this, STI);
 }
 

llvm/trunk/lib/Target/X86/Disassembler/X86Disassembler.cpp

 static bool translateInstruction(MCInst &mcInst,
                                 InternalInstruction &insn,
                                 const MCDisassembler *Dis) {
   if (!insn.spec) {
     debug("Instruction has no specification");
     return true;
   }
 
+  mcInst.clear();
   mcInst.setOpcode(insn.instructionID);
   // If when reading the prefix bytes we determined the overlapping 0xf2 or 0xf3
   // prefix bytes should be disassembled as xrelease and xacquire then set the
   // opcode to those instead of the rep and repne opcodes.
   if (insn.xAcquireRelease) {
     if(mcInst.getOpcode() == X86::REP_PREFIX)
       mcInst.setOpcode(X86::XRELEASE_PREFIX);
     else if(mcInst.getOpcode() == X86::REPNE_PREFIX)

llvm/trunk/utils/TableGen/FixedLenDecoderEmitter.cpp

OS << "template<typename InsnType>\n"
      << "    case MCD::OPC_Decode: {\n"
      << "      unsigned Len;\n"
      << "      // Decode the Opcode value.\n"
      << "      unsigned Opc = decodeULEB128(++Ptr, &Len);\n"
      << "      Ptr += Len;\n"
      << "      unsigned DecodeIdx = decodeULEB128(Ptr, &Len);\n"
      << "      Ptr += Len;\n"
      << "\n"
+     << "      MI.clear();\n"
      << "      MI.setOpcode(Opc);\n"
      << "      bool DecodeComplete;\n"
      << "      S = decodeToMCInst(S, DecodeIdx, insn, MI, Address, DisAsm, DecodeComplete);\n"
      << "      assert(DecodeComplete);\n"
      << "\n"
      << "      DEBUG(dbgs() << Loc << \": OPC_Decode: opcode \" << Opc\n"
      << "                   << \", using decoder \" << DecodeIdx << \": \"\n"
      << "                   << (S != MCDisassembler::Fail ? \"PASS\" : \"FAIL\") << \"\\n\");\n"