This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by scott.linder on Dec 3 2020, 2:57 PM.

Details

Summary

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

Diff Detail

Event Timeline

scott.linder created this revision.Dec 3 2020, 2:57 PM
scott.linder requested review of this revision.Dec 3 2020, 2:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2020, 2:57 PM

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 added inline comments.Dec 4 2020, 7:27 AM
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 added inline comments.Dec 4 2020, 9:18 AM
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

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

scott.linder added inline comments.Dec 4 2020, 12:03 PM
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 accepted this revision.Dec 4 2020, 12:29 PM

Thanks!

llvm/test/MC/ELF/cfi.s
439

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.

This revision is now accepted and ready to land.Dec 4 2020, 12:29 PM