[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.

test/MC/RISCV/rv32i-fuzzed-invalid.s

+# REQUIRES: asserts
+# RUN: not llvm-mc -triple riscv32 < %s 2>&1 | FileCheck %s
+#
+# Fuzzed test cases produced by a LLVM MC Assembler
+# Protocol Buffer Fuzzer for the RISC-V assembly language.
+#
+
+jal x13 # CHECK: error: immediate must be a multiple of 2 bytes in the range [-1048576, 1048574]

utils/TableGen/AsmMatcherEmitter.cpp

if (ReportMultipleNearMisses) {
     OS << "          }\n";
     OS << "        } else {\n";
     OS << "          DEBUG_WITH_TYPE(\"asm-matcher\", dbgs() << \"but formal operand not required\\n\");\n";
     OS << "          break;\n";
     OS << "        }\n";
     OS << "        continue;\n";
   } else {
     OS << "        OperandsValid = (Formal == InvalidMatchClass) || isSubclass(Formal, OptionalMatchClass);\n";
-    OS << "        if (!OperandsValid) ErrorInfo = ActualIdx;\n";
+    OS << "        if (!OperandsValid)\n";
+    OS << "          break;\n";
     if (HasOptionalOperands) {
       OS << "        OptionalOperandsMask.set(FormalIdx, " << MaxNumOperands
          << ");\n";
     }
     OS << "        break;\n";
   }
   OS << "      }\n";
   OS << "      MCParsedAsmOperand &Actual = *Operands[ActualIdx];\n";