This is an archive of the discontinued LLVM Phabricator instance.

[MC] Defer asm errors to post-statement failure
ClosedPublic

Authored by niravd on Aug 30 2016, 11:29 AM.

Details

Summary

Allow errors to be deferred and emitted as part of clean up to simplify
and shorten Assembly parser code. This will allow error messages to be
emitted in helper functions and be modified by the caller which has
better context.

As part of this many minor cleanups to the Parser:

  • Unify parser cleanup on error
  • Add Workaround for incorrect return values in ParseDirective instances
  • Tighten checks on error-signifying return values for parser functions and fix in-tree TargetParsers to be more consistent with the changes.
  • Fix AArch64 test cases checking for spurious error messages that are now fixed.

These changes should be backwards compatible with current Target Parsers
so long as the error status are correctly returned in appropriate functions.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd updated this revision to Diff 69733.Aug 30 2016, 11:29 AM
niravd retitled this revision from to [MC] Defer asm errors to post-statement failure.
niravd updated this object.
niravd added reviewers: rnk, majnemer.
niravd added a subscriber: llvm-commits.
rnk edited edge metadata.Aug 30 2016, 1:10 PM

Can you provide some motivating examples for how this improves diagnostic quality by eliminating duplicates? Clang's diagnostics aren't buffered, and our recovery is still acceptable.

include/llvm/MC/MCParser/MCAsmParser.h
72 ↗(On Diff #69733)

Just struct MCPendingError would be more C++-y

74 ↗(On Diff #69733)

Right now sizeof(Msg) is 256+3*sizeof(void*), and you're putting MCPendingError in a vector. You should drop this down to 64 or use std::string.

75 ↗(On Diff #69733)

This ArrayRef seems like a good way to create dangling references to local ranges arrays. Does any code actually use this functionality? Maybe we can delete it?

91 ↗(On Diff #69733)

Maybe move ShowParsedOperands down here if you want to be in the same bitfield, or just make this bool.

In D24047#529392, @rnk wrote:

Can you provide some motivating examples for how this improves diagnostic quality by eliminating duplicates? Clang's diagnostics aren't buffered, and our recovery is still acceptable.

The deferred errors is motivated by sharing in various error messages. For instance, most of the directive errors have a suffix of the form "in directive X". Deferring let's us cut down on all the strings and append a suffix once in ParseDirective (or similar place) or error.

Most of the error duplicate reduction happens not because of the buffering, but because of the improved checking of the return value from ParseInstruction or ParseDirective which causes us to continue to not run through the instruction and emit additional errors. The changes in the neon-diagnostic case is a good example: parsing failed, but we return success.

niravd updated this revision to Diff 70061.Sep 1 2016, 1:09 PM
niravd marked 4 inline comments as done.
niravd edited edge metadata.

Cleanup Ranges and other fixes from rnk.

niravd added inline comments.Sep 1 2016, 1:35 PM
include/llvm/MC/MCParser/MCAsmParser.h
75 ↗(On Diff #69733)

It looks like at most we use one SMRange anywhere in the Parser, but we definitely use the Range. Looking at the history it's doesn't look like started out with support for multiple SMRange values but never actually used this feature.

I've changed all references to ArrayRef<SMRange> in the Parsers to a single SMRange.

niravd updated this object.Sep 6 2016, 9:12 AM
rnk accepted this revision.Sep 7 2016, 3:08 PM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Sep 7 2016, 3:08 PM
This revision was automatically updated to reflect the committed changes.