[MC] Consume EndOfStatement in .cfi_{sections,endproc}

[scott.linder]

Summary

Previously these directives were always interpreted as having an extra
blank line after them.

[MaskRay]

Previously these directives were always interpreted as having an extra blank line after them.

EndOfStatement was not consumed and the next AsmParser::parseStatement would consume EndOfStatement and add a blank line.

llvm/test/MC/AsmParser/directive-parse-err.s

1 ↗

(On Diff #309377)

Please merge this into a .cfi_* parsing test. You can leverage -defsym ERR=1 to check errors. See MC/ELF/reloc-directive.s for an example

6 ↗

(On Diff #309377)

[[@LINE+1]] is deprecated. Use [[#@LINE+1]]

llvm/test/MC/AsmParser/round-trip.s

17

One pair .cfi_startproc of should be sufficient.

[scott.linder]

llvm/test/MC/AsmParser/directive-parse-err.s

1 ↗

(On Diff #309377)

To clarify, is there an "omnibus" cfi parsing test under MC/AsmParser? I see a few specific examples, but not just cfi.s. Did you have a particular test in mind?

6 ↗

(On Diff #309377)

Sorry, I keep forgetting this when I look at existing tests as templates. I'll update this in the next patch!

llvm/test/MC/AsmParser/round-trip.s

17

Agreed, I'm not sure why I added the repeated pairs, I'll update this in the next patch.

[MaskRay]

llvm/test/MC/AsmParser/directive-parse-err.s

1 ↗

(On Diff #309377)

They don't have to be under MC/AsmParser. You may use MC/ELF where there are a few cfi*.s

[scott.linder]

Merge new test into existing cfi.s test, remove gratuitous directives in the
round trip test. Replace use of deprecated FileCheck syntax.

[scott.linder]

llvm/test/MC/AsmParser/directive-parse-err.s

1 ↗

(On Diff #309377)

Ok, it just seemed like tests under ELF/ were for the binary result, not necessarily for the behavior of the parser directly.

[MaskRay]

Thanks!

llvm/test/MC/ELF/cfi.s

440

I think it is more common to place RUN lines above all the CHECK lines.

MC/ELF does have some parser tests (-filetype=asm). I agree that the organization of tests is a bit unclear but I think MC/ELF for these .cfi_* is fine because they are indeed specific to certain binary formats.

llvm/lib/MC/MCParser/AsmParser.cpp

if (parseIdentifier(Name))
       return TokError("Expected an identifier");
 
     if (Name == ".eh_frame")
       EH = true;
     else if (Name == ".debug_frame")
       Debug = true;
   }
 
+  if (parseToken(AsmToken::EndOfStatement))
+    return addErrorSuffix(" in '.cfi_sections' directive");
+
   getStreamer().emitCFISections(EH, Debug);
   return false;
 }
 
 /// parseDirectiveCFIStartProc
 /// ::= .cfi_startproc [simple]
 bool AsmParser::parseDirectiveCFIStartProc() {
   StringRef Simple;
bool AsmParser::parseDirectiveCFIStartProc() {
   // correct context information.
   getStreamer().emitCFIStartProc(!Simple.empty(), Lexer.getLoc());
   return false;
 }
 
 /// parseDirectiveCFIEndProc
 /// ::= .cfi_endproc
 bool AsmParser::parseDirectiveCFIEndProc() {
+  if (parseToken(AsmToken::EndOfStatement))
+    return addErrorSuffix(" in '.cfi_endproc' directive");
   getStreamer().emitCFIEndProc();
   return false;
 }
 
 /// parse register name or number.
 bool AsmParser::parseRegisterOrRegisterNumber(int64_t &Register,
                                               SMLoc DirectiveLoc) {
   unsigned RegNo;

llvm/test/MC/AsmParser/round-trip.s

+# RUN: llvm-mc -preserve-comments -triple i386-unknown-unknown %s >%t-1.s
+# RUN: llvm-mc -preserve-comments -triple i386-unknown-unknown %t-1.s >%t-2.s
+# RUN: diff %t-1.s %t-2.s
+
+# Test that various assembly round-trips when run through MC; the first
+# transition from hand-written to "canonical" output may introduce some small
+# differences, so we don't include the initial input in the comparison.
+
+.text
+
+# Some of these CFI instructions didn't consume the end of statement
+# consistently, which led to extra empty lines in the output.
+.cfi_sections .debug_frame
+.cfi_startproc
+.cfi_endproc

llvm/test/MC/ELF/cfi.s

 // RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | llvm-readobj -S --sr --sd - | FileCheck %s
+// RUN: not llvm-mc -triple=x86_64 -o - -defsym=ERR=1 %s 2>&1 | FileCheck %s --check-prefix=ERR
 
 f1:
         .cfi_startproc
   .cfi_lsda 0x3, bar
         nop
         .cfi_endproc
 
 f2:

 // CHECK-NEXT:       0x66E R_X86_64_PC16 foo 0x0
 // CHECK-NEXT:       0x680 R_X86_64_PC32 .text 0x1F
 // CHECK-NEXT:       0x69E R_X86_64_PC32 foo 0x0
 // CHECK-NEXT:       0x6B0 R_X86_64_PC32 .text 0x20
 // CHECK-NEXT:       0x6CE R_X86_64_PC64 foo 0x0
 // CHECK-NEXT:       0x6E4 R_X86_64_PC32 .text 0x21
 // CHECK-NEXT:     ]
 // CHECK:        }
+
+.ifdef ERR
+// ERR: [[#@LINE+1]]:15: error: Expected an identifier
+.cfi_sections $
+// ERR: [[#@LINE+1]]:28: error: unexpected token in '.cfi_sections' directive
+.cfi_sections .debug_frame $
+// ERR: [[#@LINE+1]]:39: error: unexpected token in '.cfi_sections' directive
+.cfi_sections .debug_frame, .eh_frame $
+
+// ERR: [[#@LINE+1]]:16: error: unexpected token in '.cfi_startproc' directive
+.cfi_startproc $
+// ERR: [[#@LINE+1]]:23: error: unexpected token in '.cfi_startproc' directive
+.cfi_startproc simple $
+
+// ERR: [[#@LINE+1]]:14: error: unexpected token in '.cfi_endproc' directive
+.cfi_endproc $
+.endif