This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by apazos on Aug 15 2018, 10:29 AM.

Details

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.

Diff Detail

Event Timeline

apazos created this revision.Aug 15 2018, 10:29 AM
apazos updated this revision to Diff 162116.Aug 22 2018, 5:29 PM
apazos retitled this revision from [TableGen, RISCV] Fixed SmallVector.h Assertion `idx < size()' failed to [RISCV] Fixed SmallVector.h Assertion `idx < size()' failed.
apazos edited the summary of this revision. (Show Details)
sabuasal added inline comments.Aug 22 2018, 6:53 PM
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
694

Handle the case *when* the error..?

asb added a reviewer: asb.Aug 23 2018, 4:44 AM
asb added a comment.Aug 23 2018, 5:03 AM

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

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

apazos removed a reviewer: asb.Aug 24 2018, 5:41 PM
apazos removed subscribers: rkruppe, jfb.

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

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 }, }
apazos updated this revision to Diff 162522.Aug 24 2018, 6:04 PM
asb added a reviewer: asb.Aug 30 2018, 6:15 AM

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
696

There's a stray // on this line

asb accepted this revision.Aug 30 2018, 6:15 AM
This revision is now accepted and ready to land.Aug 30 2018, 6:15 AM
apazos updated this revision to Diff 163361.Aug 30 2018, 10:06 AM
apazos retitled this revision from [RISCV] Fixed SmallVector.h Assertion `idx < size()' failed to [RISCV] Fixed SmallVector.h Assertion `idx < size()'.
apazos edited the summary of this revision. (Show Details)
apazos added a subscriber: llvm-commits.

Added a FIXME comment about jal behavior that needs fixing.

apazos updated this revision to Diff 163363.Aug 30 2018, 10:08 AM

Missed the extra //

apazos updated this revision to Diff 163393.Aug 30 2018, 12:42 PM

A typo fix.

This revision was automatically updated to reflect the committed changes.