Fix Mips Parser error reporting

[niravd]

Summary

[mips] On error, ParseDirective should always return false to signify that the
directive was understood.

[dsanders]

LGTM with a few small changes.

Could you format this the way clang-format would? In particular there should be a space after the '//' in comments and before the '{' on the if-statements.

Could you also add a test case? Something that checks we don't emit the 'unknown directive' error for one of the incorrect directives is sufficient.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

4884 ↗

(On Diff #56186)

If they're going to have the same name then I agree that they should be consistent in behaviour. From their usage it looks like we ought to have both behaviours available so I think it's best to rename one of them. We should leave that for another patch though.

6039–6041 ↗

(On Diff #56186)

I disagree with this comment as it's written since most errors are reported because we didn't (fully) understand the directive. However, the caller of ParseDirective() phrases it as "It will return 'true' if it isn't interested in this directive." which I read as "it recognized the first token of the directive and decided to handle it". I think this is what you mean and I agree we don't comply with this at the moment.

I'm thinking it might be better phrased as something like:

This returns false if this function recognizes the directive regardless of whether it is successfully handled or reports an error. Otherwise it returns true to give the generic parser a chance at recognizing it.

[niravd]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

4884 ↗

(On Diff #56186)

Either that or merge them into a single. Regardless, I've added a bit more text to be a bit clearer.

[niravd]

Add test case and cleanup

llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp

if (getLexer().isNot(AsmToken::EndOfStatement)) {
     SMLoc Loc = getLexer().getLoc();
     Parser.eatToEndOfStatement();
     return Error(Loc, "unexpected token in argument list");
   }
   Parser.Lex(); // Consume the EndOfStatement.
   return false;
 }
 
+// FIXME: Given that these have the same name, these should both be
+// consistent on affecting the Parser.
 bool MipsAsmParser::reportParseError(Twine ErrorMsg) {
   MCAsmParser &Parser = getParser();
   SMLoc Loc = getLexer().getLoc();
   Parser.eatToEndOfStatement();
   return Error(Loc, ErrorMsg);
 }
 
 bool MipsAsmParser::reportParseError(SMLoc Loc, Twine ErrorMsg) {
bool MipsAsmParser::parseDirectiveCPSetup() {
   unsigned FuncReg;
   unsigned Save;
   bool SaveIsReg = true;
 
   SmallVector<std::unique_ptr<MCParsedAsmOperand>, 1> TmpReg;
   OperandMatchResultTy ResTy = parseAnyRegister(TmpReg);
   if (ResTy == MatchOperand_NoMatch) {
     reportParseError("expected register containing function address");
-    Parser.eatToEndOfStatement();
     return false;
   }
 
   MipsOperand &FuncRegOpnd = static_cast<MipsOperand &>(*TmpReg[0]);
   if (!FuncRegOpnd.isGPRAsmReg()) {
     reportParseError(FuncRegOpnd.getStartLoc(), "invalid register");
     Parser.eatToEndOfStatement();
     return false;
if (Lexer.is(AsmToken::Integer)) {
 
     return true;
   }
 
   return false;
 }
 
 bool MipsAsmParser::ParseDirective(AsmToken DirectiveID) {
+  // This returns false if this function recognizes the directive
+  // regardless of whether it is successfully handles or reports an
+  // error. Otherwise it returns true to give the generic parser a
+  // chance at recognizing it.
+
   MCAsmParser &Parser = getParser();
   StringRef IDVal = DirectiveID.getString();
 
-  if (IDVal == ".cpload")
-    return parseDirectiveCpLoad(DirectiveID.getLoc());
-  if (IDVal == ".cprestore")
-    return parseDirectiveCpRestore(DirectiveID.getLoc());
+  if (IDVal == ".cpload") {
+    parseDirectiveCpLoad(DirectiveID.getLoc());
+    return false;
+  }
+  if (IDVal == ".cprestore") {
+    parseDirectiveCpRestore(DirectiveID.getLoc());
+    return false;
+  }
   if (IDVal == ".dword") {
     parseDataDirective(8, DirectiveID.getLoc());
     return false;
   }
   if (IDVal == ".ent") {
     StringRef SymbolName;
 
     if (Parser.parseIdentifier(SymbolName)) {
if (IDVal == ".frame") {
 
     getTargetStreamer().emitFrame(StackReg, FrameSizeVal,
                                   ReturnRegOpnd.getGPR32Reg());
     IsCpRestoreSet = false;
     return false;
   }
 
   if (IDVal == ".set") {
-    return parseDirectiveSet();
+    parseDirectiveSet();
+    return false;
   }
 
   if (IDVal == ".mask" || IDVal == ".fmask") {
     // .mask bitmask, frame_offset
     // bitmask: One bit for each register used.
     // frame_offset: Offset from Canonical Frame Address ($sp on entry) where
     //               first register is expected to be saved.
     // Examples:
if (IDVal == ".word") {
     return false;
   }
 
   if (IDVal == ".hword") {
     parseDataDirective(2, DirectiveID.getLoc());
     return false;
   }
 
-  if (IDVal == ".option")
-    return parseDirectiveOption();
+  if (IDVal == ".option") {
+    parseDirectiveOption();
+    return false;
+  }
 
   if (IDVal == ".abicalls") {
     getTargetStreamer().emitDirectiveAbiCalls();
     if (Parser.getTok().isNot(AsmToken::EndOfStatement)) {
       Error(Parser.getTok().getLoc(), 
             "unexpected token, expected end of statement");
       // Clear line
       Parser.eatToEndOfStatement();
     }
     return false;
   }
 
-  if (IDVal == ".cpsetup")
-    return parseDirectiveCPSetup();
-
-  if (IDVal == ".cpreturn")
-    return parseDirectiveCPReturn();
-
-  if (IDVal == ".module")
-    return parseDirectiveModule();
-
-  if (IDVal == ".llvm_internal_mips_reallow_module_directive")
-    return parseInternalDirectiveReallowModule();
-
-  if (IDVal == ".insn")
-    return parseInsnDirective();
-
-  if (IDVal == ".sbss")
-    return parseSSectionDirective(IDVal, ELF::SHT_NOBITS);
-  if (IDVal == ".sdata")
-    return parseSSectionDirective(IDVal, ELF::SHT_PROGBITS);
+  if (IDVal == ".cpsetup") {
+    parseDirectiveCPSetup();
+    return false;
+  }
+  if (IDVal == ".cpreturn") {
+    parseDirectiveCPReturn();
+    return false;
+  }
+  if (IDVal == ".module") {
+    parseDirectiveModule();
+    return false;
+  }
+  if (IDVal == ".llvm_internal_mips_reallow_module_directive") {
+    parseInternalDirectiveReallowModule();
+    return false;
+  }
+  if (IDVal == ".insn") {
+    parseInsnDirective();
+    return false;
+  }
+  if (IDVal == ".sbss") {
+    parseSSectionDirective(IDVal, ELF::SHT_NOBITS);
+    return false;
+  }
+  if (IDVal == ".sdata") {
+    parseSSectionDirective(IDVal, ELF::SHT_PROGBITS);
+    return false;
+  }
 
   return true;
 }
 
 bool MipsAsmParser::parseInternalDirectiveReallowModule() {
   // If this is not the end of the statement, report an error.
   if (getLexer().isNot(AsmToken::EndOfStatement)) {
     reportParseError("unexpected token, expected end of statement");

llvm/trunk/test/MC/Mips/set-push-pop-directives-bad.s

 # RUN: not llvm-mc %s -triple=mipsel-unknown-linux -mcpu=mips32r2 2>%t1
 # RUN:   FileCheck %s < %t1
 
+# Check only one error per statement.
+
         .text
         .set pop
 # CHECK: :[[@LINE-1]]:14: error: .set pop with no .set push
+# CHECK-NOT: :[[@LINE-1]]:
         .set push
         .set pop
         .set pop
 # CHECK: :[[@LINE-1]]:14: error: .set pop with no .set push
+# CHECK-NOT: :[[@LINE-1]]:
         .set push foo
 # CHECK: :[[@LINE-1]]:19: error: unexpected token, expected end of statement
+# CHECK-NOT: :[[@LINE-1]]:
         .set pop bar
 # CHECK: :[[@LINE-1]]:18: error: unexpected token, expected end of statement
-
+# CHECK-NOT: :[[@LINE-1]]:
         .set hardfloat
         .set push
         .set softfloat
         add.s $f2, $f2, $f2
 # CHECK: :[[@LINE-1]]:9: error: instruction requires a CPU feature not currently enabled
+# CHECK-NOT: :[[@LINE-1]]:
         .set pop
         add.s $f2, $f2, $f2
 # CHECK-NOT: :[[@LINE-1]]:9: error: instruction requires a CPU feature not currently enabled
+# CHECK-NOT: :[[@LINE-1]]: