[RISCV] Fixed SmallVector.h Assertion `idx < size()'

[apazos]

Summary

RISCVAsmParser needs to handle the case the error message is of specific type, other than the generic Match_InvalidOperand, and the corresponding
operand is missing.

This bug was uncovered by a LLVM MC Assembler Protocol Buffer Fuzzer for the RISC-V assembly language.

[sabuasal]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

694 ↗

(On Diff #162116)

Handle the case *when* the error..?

[asb]

This patch doesn't really fix the problem with jal a3 though catching these target-specific invalid operand errors seems worth doing.

jal a3 actually shouldn't fail due to not having enough operands if we're trying to match gas. Sadly, RISC-V assembly is horrifically lax (it would be neat to implement a 'strict mode' that disabled many of the aliases that are likely to be mistakes). jal a3 is interpreted by gas as jal ra, a3 where a3 is a symbol.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

697–722 ↗

(On Diff #162116)

I think this can be safely replaced with if (Result > FIRST_TARGET_MATCH_RESULT_TY) { , which is more maintainable.

[apazos]

Alex, I did not understand your comment.
"jal a3" parsing will crash due to ErrorInfo == Operands.size(), which this patch is fixing.

[apazos]

RISCVAsmMatcher tries to match the second jal because a3 happens to match with a register name:

{ 2000 /* jal */, RISCV::JAL, ConvertregX1SImm21Lsb01_0, 0, { MCK_SImm21Lsb0 }, },

{ 2000 /* jal */, RISCV::JAL, Convert__Reg1_0__SImm21Lsb01_1, 0, { MCK_GPR, MCK_SImm21Lsb0 }, }

[asb]

In D50790#1213227, @apazos wrote:

Alex, I did not understand your comment.
"jal a3" parsing will crash due to ErrorInfo == Operands.size(), which this patch is fixing.

Sorry I wasn't clear. What I mean is that the handling of jal a3 is still incorrect as it should be accepted rather than rejected (with a3 interpreted as a symbol rather than a register). I'm working on follow-up patches to correct this and related issues.

This patch LGTM, thanks!

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

714 ↗

(On Diff #162522)

There's a stray // on this line

[apazos]

Added a FIXME comment about jal behavior that needs fixing.

[apazos]

Missed the extra //

[apazos]

A typo fix.

llvm/trunk/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

 
 bool RISCVAsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                              OperandVector &Operands,
                                              MCStreamer &Out,
                                              uint64_t &ErrorInfo,
                                              bool MatchingInlineAsm) {
   MCInst Inst;
 
-  switch (MatchInstructionImpl(Operands, Inst, ErrorInfo, MatchingInlineAsm)) {
+  auto Result =
+    MatchInstructionImpl(Operands, Inst, ErrorInfo, MatchingInlineAsm);
+  switch (Result) {
   default:
     break;
   case Match_Success:
     return processInstruction(Inst, IDLoc, Out);
   case Match_MissingFeature:
     return Error(IDLoc, "instruction use requires an option to be enabled");
   case Match_MnemonicFail:
     return Error(IDLoc, "unrecognized instruction mnemonic");
   case Match_InvalidOperand: {
     SMLoc ErrorLoc = IDLoc;
     if (ErrorInfo != ~0U) {
       if (ErrorInfo >= Operands.size())
         return Error(ErrorLoc, "too few operands for instruction");
 
       ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
       if (ErrorLoc == SMLoc())
         ErrorLoc = IDLoc;
     }
     return Error(ErrorLoc, "invalid operand for instruction");
   }
+  }
+
+  // Handle the case when the error message is of specific type
+  // other than the generic Match_InvalidOperand, and the
+  // corresponding operand is missing.
+  if (Result > FIRST_TARGET_MATCH_RESULT_TY) {
+    SMLoc ErrorLoc = IDLoc;
+    if (ErrorInfo != ~0U && ErrorInfo >= Operands.size())
+        return Error(ErrorLoc, "too few operands for instruction");
+  }
+
+  switch(Result) {
+  default:
+    break;
   case Match_InvalidImmXLen:
     if (isRV64()) {
       SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
       return Error(ErrorLoc, "operand must be a constant 64-bit integer");
     }
     return generateImmOutOfRangeError(Operands, ErrorInfo,
                                       std::numeric_limits<int32_t>::min(),
                                       std::numeric_limits<uint32_t>::max());

llvm/trunk/test/MC/RISCV/rv32i-invalid.s

 sltiu s2, s3, 0x50, 0x60 # CHECK: :[[@LINE]]:21: error: invalid operand for instruction
 
 # Memory operand not formatted correctly
 lw a4, a5, 111 # CHECK: :[[@LINE]]:8: error: immediate must be an integer in the range [-2048, 2047]
 
 # Too few operands
 ori a0, a1 # CHECK: :[[@LINE]]:1: error: too few operands for instruction
 xor s2, s2 # CHECK: :[[@LINE]]:1: error: too few operands for instruction
+# FIXME: Fix jal behavior to interpret a3 as a symbol rather than a register.
+jal a3     # CHECK: :[[@LINE]]:1: error: too few operands for instruction
 
 # Instruction not in the base ISA
 mul a4, ra, s0 # CHECK: :[[@LINE]]:1: error: instruction use requires an option to be enabled
 amomaxu.w s5, s4, (s3) # CHECK: :[[@LINE]]:1: error: instruction use requires an option to be enabled
 fadd.s ft0, ft1, ft2 # CHECK: :[[@LINE]]:1: error: instruction use requires an option to be enabled
 
 # Using floating point registers when integer registers are expected
 addi a2, ft0, 24 # CHECK: :[[@LINE]]:10: error: invalid operand for instruction
 
 # fence.tso accepts no operands
 fence.tso rw, rw # CHECK: :[[@LINE]]:11: error: invalid operand for instruction