[PowerPC] Remove asserts from the disassembler.

[stefanp]

Summary

My previous patch had added a couple of asserts to the disassembler.
The problem with this is that the disassembler is not just used for the
text section it is also used to disassemble the data section of an
object where the bytes do not necessarily represent instructions. If the
data in the data section happens to look like an illegal instruction
then llvm-objdump will assert on data because it is finding an illegal
instruction that is not actually an instruction at all.

[shchenz]

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

118

Little confused. From the function name and its parameter seems like this function should only be called for instructions. Is it possible that there is a wrong call to this function? For example, for data, we should not decode fpr RC as there is no RC for data at all?

[amyk]

Could we also update the description of the patch to specify what My previous patch means (as in, which patch)?

llvm/test/MC/Disassembler/PowerPC/ppc64-encoding-dfp.txt

27

Might be a silly question, but I have a clarification question.
Is the goal here, we want the disassembler to still continue decoding an instruction (even if it is invalid/illegal), but cannot have it assert? Does anything special need to happen after we have decoded an illegal instruction?

[barannikov88]

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

118

The removed assertion should have been changed to if (...) return DecodeStatus::Fail; so that random data is not decoded into invalid instructions.
The added test should be a negative one.

[nemanjai]

Please note that this assert can trip even when disassembling with --disassemble and not just -disassemble-all. Namely, the offending word can appear in the text section (for example, the AIX traceback table, etc.).
This is very similar to https://reviews.llvm.org/rGc86f8d4276ae

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

118–119

Lets not just remove these. Please change it to something like:

if (RegNo > 30 || (RegNo & 1))
  return MCDisassembler::Fail;

[nemanjai]

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

86

While we're at it, we can turn this one into a disassembly failure rather than an assert.

351

While we're at it, we can turn this one into a disassembly failure rather than an assert.

[stefanp]

Added the conditions so that we return DecodeStatus::Fail instead of asserting.

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

118

It is not an incorrect call to the function. The disassembler will call this whenever it tries to disassemble anything that looks like it might be an instruction. As other people in the comments mentioned I need to stop the assert and instead just return a failure to disassemble.

llvm/test/MC/Disassembler/PowerPC/ppc64-encoding-dfp.txt

27

No, we don't want to decode an incorrect instruction. The initial issue was that we would decode an instruction that was part of the data section or (as Nemanja pointed out) part of the AIX traceback table and those bytes just happened to look like an instruction but an invalid instruction. When I had the asserts in there the disassembler would assert and we don't want that.

[nemanjai]

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

351

Did you just miss this one or is there a reason you didn't address this comment?

[stefanp]

Updated one assert that I missed.

[stefanp]

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

351

Sorry, I did miss this one.

[nemanjai]

LGTM. Thanks for fixing these.

[stefanp]

Rebased to top of main.

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

 }
 
 // FIXME: These can be generated by TableGen from the existing register
 // encoding values!
 
 template <std::size_t N>
 static DecodeStatus decodeRegisterClass(MCInst &Inst, uint64_t RegNo,
                                         const MCPhysReg (&Regs)[N]) {
   assert(RegNo < N && "Invalid register number");
   Inst.addOperand(MCOperand::createReg(Regs[RegNo]));
   return MCDisassembler::Success;
 }
 
 static DecodeStatus DecodeCRRCRegisterClass(MCInst &Inst, uint64_t RegNo,
                                             uint64_t Address,
                                             const MCDisassembler *Decoder) {
   return decodeRegisterClass(Inst, RegNo, CRRegs);
static DecodeStatus DecodeF8RCRegisterClass(MCInst &Inst, uint64_t RegNo,
                                             uint64_t Address,
                                             const MCDisassembler *Decoder) {
   return decodeRegisterClass(Inst, RegNo, FRegs);
 }
 
 static DecodeStatus DecodeFpRCRegisterClass(MCInst &Inst, uint64_t RegNo,
                                             uint64_t Address,
                                             const MCDisassembler *Decoder) {
-  assert(RegNo <= 30 && "Expecting a register number no more than 30.");
-  assert((RegNo & 1) == 0 && "Expecting an even register number.");
   return decodeRegisterClass(Inst, RegNo >> 1, FpRegs);
 }
 
 static DecodeStatus DecodeVFRCRegisterClass(MCInst &Inst, uint64_t RegNo,
                                             uint64_t Address,
                                             const MCDisassembler *Decoder) {
   return decodeRegisterClass(Inst, RegNo, VFRegs);
 }
 

 }
 
 static DecodeStatus decodeCRBitMOperand(MCInst &Inst, uint64_t Imm,
                                         int64_t Address,
                                         const MCDisassembler *Decoder) {
   // The cr bit encoding is 0x80 >> cr_reg_num.
 
   unsigned Zeros = llvm::countr_zero(Imm);
   assert(Zeros < 8 && "Invalid CR bit value");
 
   Inst.addOperand(MCOperand::createReg(CRRegs[7 - Zeros]));
   return MCDisassembler::Success;
 }
 
 #include "PPCGenDisassemblerTables.inc"
 
 DecodeStatus PPCDisassembler::getInstruction(MCInst &MI, uint64_t &Size,

llvm/test/MC/Disassembler/PowerPC/ppc64-encoding-dfp.txt

 # CHECK: dsub. 2, 3, 4
 0xec 0x43 0x24 0x05
 
 # CHECK: dsubq 2, 6, 4
 0xfc 0x46 0x24 0x04
 
 # CHECK: dsubq. 2, 6, 4
 0xfc 0x46 0x24 0x05
+
+# This is actually the encoding for an invalid instruction.
+# However, we check it here because the disassembler shouldn't check if an
+# instruction is valid when decoding it because llvm-objdump uses the decoder
+# for data sections as well as text sections and we do not want to assert in
+# cases where we are decoding a data section that happens to match an invalid
+# instruction.
+# CHECK: dsubq 0, 6, 30
+0xfc 0x06 0xfc 0x04