[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

121

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 ↗

(On Diff #518928)

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

121

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 ↗

(On Diff #518928)

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");
+  if (RegNo >= N)
+    return MCDisassembler::Fail;
   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.");
+  if (RegNo > 30 || (RegNo & 1))
+    return MCDisassembler::Fail;
   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-invalid.txt

+# RUN: llvm-mc --disassemble %s -mcpu=pwr10 -triple \
+# RUN:   powerpc64-unknown-linux-gnu < %s 2>&1 | FileCheck %s
+
+# Regsiter for DSUBQ cannot be greater than 30.
+# CHECK-NOT: dsubq 0, 6, 31
+# CHECK: warning: invalid instruction encoding
+0xfc 0x06 0xfc 0x04