This is an archive of the discontinued LLVM Phabricator instance.

Differential D50808

[MC, RISCV] Fixed StringRef Assertion `Index < Length && "Invalid index!"'
ClosedPublic

Authored by apazos on Aug 15 2018, 1:48 PM.

Details

Reviewers
rnk
Commits
rG353f67a741da: [MC, RISCV] Fixed StringRef Assertion `Index < Length && "Invalid index!"'
rL340678: [MC, RISCV] Fixed StringRef Assertion `Index < Length && "Invalid index!"'
Summary

Handle the case IDVal is an empty string.

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, 1:48 PM
Herald added subscribers: PkmX, rogfer01, shiva0217, kito-cheng. · View Herald TranscriptAug 15 2018, 1:48 PM
bcain added a subscriber: bcain.Aug 15 2018, 3:01 PM
apazos added a subscriber: llvm-commits.Aug 16 2018, 2:46 PM
apazos added a subscriber: grosbach.Aug 22 2018, 6:45 PM
asb added a subscriber: asb.Aug 23 2018, 7:19 AM
asb added inline comments.Aug 23 2018, 7:21 AM
test/Assembler/empty-string.s
2

This test shouldn't actually require asserts should it?

apazos updated this revision to Diff 162516.Aug 24 2018, 5:20 PM

Thanks Alex for reviewing. Adding some more reviewers who might +1 before I merge

apazos added subscribers: peter.smith, pcc, niravd, rnk.Aug 24 2018, 5:27 PM
rnk accepted this revision.Aug 24 2018, 5:33 PM

lgtm

lib/MC/MCParser/AsmParser.cpp
1845

I'd probably spell it IDVal.startswith(".") for readability.

This revision is now accepted and ready to land.Aug 24 2018, 5:33 PM
apazos added inline comments.Aug 24 2018, 6:04 PM
lib/MC/MCParser/AsmParser.cpp
1845

good suggestion, will update the patch and merge. Thanks, Reid.

apazos updated this revision to Diff 162526.Aug 24 2018, 6:20 PM
Closed by commit rL340678: [MC, RISCV] Fixed StringRef Assertion `Index < Length && "Invalid index!"' (authored by apazos). · Explain WhyAug 24 2018, 6:36 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
MC/
MCParser/
2 lines
test/
Assembler/
3 lines

Diff 162526

lib/MC/MCParser/AsmParser.cpp

Show First 20 Lines • Show All 1,836 Lines • ▼ Show 20 Linesbool AsmParser::parseStatement(ParseStatementInfo &Info,
if (areMacrosEnabled()) if (areMacrosEnabled())
if (const MCAsmMacro *M = getContext().lookupMacro(IDVal)) { if (const MCAsmMacro *M = getContext().lookupMacro(IDVal)) {
return handleMacroEntry(M, IDLoc); return handleMacroEntry(M, IDLoc);
} }
// Otherwise, we have a normal instruction or directive. // Otherwise, we have a normal instruction or directive.
// Directives start with "." // Directives start with "."
if (IDVal[0] == '.' && IDVal != ".") { if (IDVal.startswith(".") && IDVal != ".") {
rnkUnsubmitted
Not Done
ReplyInline Actions

I'd probably spell it IDVal.startswith(".") for readability.

rnk: I'd probably spell it `IDVal.startswith(".")` for readability.
apazosAuthorUnsubmitted
Not Done
ReplyInline Actions

good suggestion, will update the patch and merge. Thanks, Reid.

apazos: good suggestion, will update the patch and merge. Thanks, Reid.
// There are several entities interested in parsing directives: // There are several entities interested in parsing directives:
// //
// 1. The target-specific assembly parser. Some directives are target // 1. The target-specific assembly parser. Some directives are target
// specific or may potentially behave differently on certain targets. // specific or may potentially behave differently on certain targets.
// 2. Asm parser extensions. For example, platform-specific parsers // 2. Asm parser extensions. For example, platform-specific parsers
// (like the ELF parser) register themselves as extensions. // (like the ELF parser) register themselves as extensions.
// 3. The generic directive parser implemented by this class. These are // 3. The generic directive parser implemented by this class. These are
// all the directives that behave in a target and platform independent // all the directives that behave in a target and platform independent
▲ Show 20 Lines • Show All 4,046 Lines • Show Last 20 Lines

test/Assembler/empty-string.s

  • This file was added.
// REQUIRES: riscv-registered-target
// RUN: not llvm-mc -triple riscv32-unknown-linux-gnu < %s 2>&1 | FileCheck %s
asbUnsubmitted
Not Done
ReplyInline Actions

This test shouldn't actually require asserts should it?

asb: This test shouldn't actually require `asserts` should it?
"" # CHECK: error: unrecognized instruction mnemonic