[PATCH] D149711: [PowerPC] Remove asserts from the disassembler.

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp:118
                                             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);
 }
----------------
nemanjai wrote:
> barannikov88 wrote:
> > shchenz wrote:
> > > 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?
> > 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.
> > 
> Lets not just remove these. Please change it to something like:
> ```
>   if (RegNo > 30 || (RegNo & 1))
>     return MCDisassembler::Fail;
> ```
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.


================
Comment at: llvm/test/MC/Disassembler/PowerPC/ppc64-encoding-dfp.txt:27
+
+# This is actually the encoding for an invalid instruction.
+# However, we check it here because the disassembler shouldn't check if an
----------------
amyk wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149711/new/

https://reviews.llvm.org/D149711