Add -verify-asm-diags mode to llvm-mc

[olista01]

Summary

Similar to clang's -verify option, this parses comments in the source containing expected diagnostics, and checks that the emitted diagnostics match them. It is intended to be used in tests in the same way as the clang option.

I haven't been able to share any code other than the ParseHelper class with the clang implementation, because clang uses it's own source manager and diagnostic classes, which are different from LLVM's.

[grimar]

I know nothing about ARM/llvm-mc, sorry.

[niravd]

I'm not familiar with the historical justification for verify in clang, so this may be obvious, but what do these patches give us? The assembler should be emitting comments in linear order, so I don't see where FileCheck is insufficient or more difficult to write tests with. Is the long term goal to allow checking of included files and modularize testing?

[olista01]

The assembler does not emit errors in source order for errors that are detected after parsing, such as during layout or fixup resolution.

I also think it's easier to write robust tests using this than FileCheck, as you don't have to regex-match the line and column numbers.

[niravd]

The assembler does not emit errors in source order for errors that are detected after parsing, such as during layout or fixup resolution.

Ah, right. And having the error message come out of order in those cases is natural and better for the user so we shouldn't sort the output messages.

Can you make a test example where we specifically get out-of-order messages?

Otherwise LGTM.

[olista01]

Re-ordered the existing test file to show that the order is no longer important for post-parsing errors, and remove the obsolete comment.

[olista01]

Thanks, I'll commit this once the dependencies are all committed.

test/MC/ARM/error-location.s

-@ RUN: not llvm-mc -triple armv7a--none-eabi -filetype obj < %s -o /dev/null 2>&1 | FileCheck %s
-
-@ Note: These errors are not always emitted in the order in which the relevant
-@ source appears, this file is carefully ordered so that that is the case.
+@ RUN: llvm-mc -verify-asm-diags -triple armv7a--none-eabi -filetype obj < %s -o /dev/null
 
   .text
-@ CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: symbol 'undef' can not be undefined in a subtraction expression
-  .word (0-undef)
+  .word (0-undef) // expected-error {{symbol 'undef' can not be undefined in a subtraction expression}}
 
-@ CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: expected relocatable expression
-  .word -undef
+  .word -undef // expected-error {{expected relocatable expression}}
 
-@ CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: No relocation available to represent this relative expression
-  adr r0, #a-undef
+  .set v1, -undef // expected-error@0 {{expression could not be evaluated}}
 
-@ CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: Cannot represent a difference across sections
-  .word x_a - y_a
+  adr r0, #a-undef // expected-error {{No relocation available to represent this relative expression}}
 
-@ CHECK: <unknown>:0: error: expression could not be evaluated
-  .set v1, -undef
+  .word x_a - y_a // expected-error {{Cannot represent a difference across sections}}
 
   .comm common, 4
-@ CHECK: <unknown>:0: error: Common symbol 'common' cannot be used in assignment expr
-  .set v3, common
+  .set v3, common // expected-error@0 {{Common symbol 'common' cannot be used in assignment expr}}
 
-@ CHECK: <unknown>:0: error: symbol 'undef' could not be evaluated in a subtraction expression
-  .set v2, a-undef
+  .set v2, a-undef // expected-error@0 {{symbol 'undef' could not be evaluated in a subtraction expression}}
 
 
 
 w:
   .word 0
   .weak w
 
 
   .section sec_x
 x_a:
   .word 0
 
 
   .section sec_y
 y_a:
   .word 0

test/MC/ARM/verify-diagnostics-all-matching.s

+// RUN: llvm-mc -triple armv7a--none-eabi < %s -verify-asm-diags 2>&1 | FileCheck %s
+
+  wibble // expected-error {{invalid instruction}}
+  wibble @ expected-error {{invalid instruction}}
+  wibble /* expected-error {{invalid instruction}} */
+
+  // A normal comment, not for the diagnostic verifier, even if it contains the
+  // word expected somewhere.
+
+  wibble /* unrelated text is allowed in parsed comments
+            as are line-breaks
+            expected-error {{invalid instruction}} */
+
+  wibble
+  // expected-error@14 {{invalid instruction}}
+
+  wibble
+  // expected-error@-1 {{invalid instruction}}
+
+  // expected-error@+1 {{invalid instruction}}
+  wibble
+
+  foo; bar // expected-error {{invalid instruction}} expected-error {{invalid instruction}}
+
+  foo; bar // expected-error    {{invalid instruction}}
+           // expected-error@-1 {{invalid instruction}}
+
+// CHECK-NOT: error

test/MC/ARM/verify-diagnostics-comment-parsing.s

+// RUN: not llvm-mc -triple armv7a--none-eabi < %s -verify-asm-diags 2>&1 | FileCheck %s
+
+// expected-foo {{text}}
+// CHECK: :[[@LINE-1]]:13: error: unsupported diagnostic severity
+
+// expected-warning {{text}}
+// CHECK-NOT: :[[@LINE-1]]:{{.*}}: error: unsupported diagnostic severity
+
+// expected-note {{text}}
+// CHECK-NOT: :[[@LINE-1]]:{{.*}}: error: unsupported diagnostic severity
+
+// expected-error
+// CHECK: :[[@LINE-1]]:18: error: cannot find start ('{{[{][{]}}') of expected string
+
+// expected-error {{
+// CHECK: :[[@LINE-1]]:21: error: cannot find end ('}}') of expected string

test/MC/ARM/verify-diagnostics-expected-not-seen.s

+// RUN: not llvm-mc -triple armv7a--none-eabi < %s -verify-asm-diags 2>&1 | FileCheck %s
+
+// expected-error {{error text}}
+// CHECK: :[[@LINE-1]]:4: error: 'error' expected but not seen: error text
+
+// expected-error {{multiple errors}} expected-error {{on one line}}
+// CHECK: :[[@LINE-1]]:4: error: 'error' expected but not seen: multiple errors
+// CHECK: :[[@LINE-2]]:39: error: 'error' expected but not seen: on one line

test/MC/ARM/verify-diagnostics-seen-not-expected.s

+// RUN: not llvm-mc -triple armv7a--none-eabi < %s -verify-asm-diags 2>&1 | FileCheck %s
+
+  wibble
+// CHECK: :[[@LINE-1]]:3: error: 'error' seen but not expected: invalid instruction
+
+  foo; bar
+// CHECK-DAG: :[[@LINE-1]]:3: error: 'error' seen but not expected: invalid instruction
+// CHECK-DAG: :[[@LINE-2]]:8: error: 'error' seen but not expected: invalid instruction

tools/llvm-mc/llvm-mc.cpp

 #include "llvm/MC/MCTargetOptionsCommandFlags.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/ParseHelper.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/ToolOutputFile.h"
 
 using namespace llvm;
static cl::opt<bool> SaveTempLabels("save-temp-labels",
                                     cl::desc("Don't discard temporary labels"));
 
 static cl::opt<bool> NoExecStack("no-exec-stack",
                                  cl::desc("File doesn't need an exec stack"));
 
 enum ActionType {
   AC_AsLex,
   AC_Assemble,
+  AC_AssembleVerifyDiags,
   AC_Disassemble,
   AC_MDisassemble,
 };
 
 static cl::opt<ActionType>
 Action(cl::desc("Action to perform:"),
        cl::init(AC_Assemble),
        cl::values(clEnumValN(AC_AsLex, "as-lex",
                              "Lex tokens from a .s file"),
                   clEnumValN(AC_Assemble, "assemble",
                              "Assemble a .s file (default)"),
+                  clEnumValN(AC_AssembleVerifyDiags, "verify-asm-diags",
+                             "Assemble a .s file, and verify diagnostics"),
                   clEnumValN(AC_Disassemble, "disassemble",
                              "Disassemble strings of hex bytes"),
                   clEnumValN(AC_MDisassemble, "mdis",
                              "Marked up disassembly of strings of hex bytes")));
 
 static const Target *GetTarget(const char *ProgName) {
   // Figure out the target triple.
   if (TripleName.empty())
if (Val.getAsInteger(0, Value)) {
       errs() << "error: Value is not an integer: " << Val << "\n";
       return 1;
     }
     Parser.getContext().setSymbolValue(Parser.getStreamer(), Sym, Value);
   }
   return 0;
 }
 
+// Implements the assembly diagnostic verification, similar to the clang
+// -verify option. This doesn't implement the full functionality of clang's
+// checker, in particular it currently only supports diagnostics emitted in the
+// main file, not any include files. It also doesn't know anything about
+// assembler macros.
+// This has callbacks from the source manager when a diagnostic is emitted, and
+// from the lexer when a comment is seen in the source. It uses these to record
+// lists of seen and expected diagnostics, and verifies that the two lists
+// match.
+class VerifyAsmDiagnosticConsumer : public AsmCommentConsumer {
+  // Information stored about a diagnostic, either expected (from a parsed
+  // comment) or seen (from an actually emitted diag).
+  struct PendingDiag {
+    std::string DiagText;
+    SourceMgr::DiagKind Kind;
+    // The line number on which the diagnostic was seen or is expected.
+    int LineNo;
+    // The exact location at which the diagnostic was reported, or the expected
+    // diagnostic was described. This is just used for emitting our own
+    // diagnostics, as we wouldn't expect the expected and actual locations to
+    // match up exactly.
+    SMLoc Loc;
+    // True if this is an expected diagnostic, and we have already seen an
+    // actual diagnostic that matches it.
+    bool Matched;
+
+    // Constructor for an actually emitted diagnostic.
+    PendingDiag(const SMDiagnostic &Diag)
+        : DiagText(Diag.getMessage()), Kind(Diag.getKind()),
+          LineNo(Diag.getLineNo()), Loc(Diag.getLoc()), Matched(false) {}
+
+    // Constructor for an expected diagnostic.
+    PendingDiag(unsigned LineNo, StringRef ExpectedText,
+                SourceMgr::DiagKind Kind, SMLoc Loc)
+        : DiagText(ExpectedText), Kind(Kind), LineNo(LineNo), Loc(Loc),
+          Matched(false) {}
+
+    bool operator<(const PendingDiag &RHS) const { return LineNo < RHS.LineNo; }
+  };
+
+  SourceMgr &SrcMgr;
+  MCAsmParser &Parser;
+  // True if we have emitted an error ourselves (for an unparsable comment, or
+  // a mismatch between expected or seen diagnostics).
+  bool HadError;
+
+  std::vector<PendingDiag> SeenDiags;
+  std::vector<PendingDiag> ExpectedDiags;
+
+public:
+  VerifyAsmDiagnosticConsumer(SourceMgr &SrcMgr,
+                              MCAsmParser &Parser)
+      : SrcMgr(SrcMgr), Parser(Parser), HadError(false) {}
+  ~VerifyAsmDiagnosticConsumer() {}
+
+  // Get the source location pointing to the current position in a comment that
+  // we are parsing.
+  SMLoc getCurrentLoc(SMLoc CommentStartLoc, ParseHelper &PH) {
+    return SMLoc::getFromPointer(CommentStartLoc.getPointer() +
+                                 (PH.C - PH.Begin));
+  }
+
+  // Get the source location pointing to the start of the current token in a
+  // comment that we are parsing.
+  SMLoc getTokenStartLoc(SMLoc CommentStartLoc, ParseHelper &PH) {
+    return SMLoc::getFromPointer(CommentStartLoc.getPointer() +
+                                 (PH.P - PH.Begin));
+  }
+
+  // Report an error of our own, which should be printed to stderr, rather than
+  // being recorded.
+  void ReportError(SMLoc Loc, StringRef Message) {
+    // Temporarily disable our diagnostic callback while emitting our own
+    // diagnostics, and re-enable it afterwards.
+    SrcMgr.setDiagHandler(nullptr);
+    Parser.printError(Loc, Message);
+    SrcMgr.setDiagHandler(
+        &VerifyAsmDiagnosticConsumer::HandleDiagnosticCallback, this);
+    HadError = true;
+  }
+
+  // Report that Diag was either seen but not expected (when Seen==true), or
+  // vice-verse (when Seen==false).
+  void ReportMismatchError(PendingDiag Diag, bool Seen) {
+    SmallString<256> Message;
+    raw_svector_ostream OS(Message);
+
+    OS << "'" << getDiagKindName(Diag.Kind) << "' ";
+    if (Seen)
+      OS << "seen but not expected: ";
+    else
+      OS << "expected but not seen: ";
+    OS << Diag.DiagText;
+
+    ReportError(Diag.Loc, Message);
+  }
+
+  // Diagnostic from the assembler, record it for later checking.  Don't print
+  // it, as we want to avoid cluttering stderr with a mixture of actual
+  // assembler diagnostics and our own errors.
+  void HandleDiagnostic(const SMDiagnostic &Diag) {
+    SeenDiags.push_back(PendingDiag(Diag));
+  }
+
+  // Callback triggered when the assembler lexes a comment. This parses the
+  // text of the comment, and records any diagnostics that we should expect to
+  // see for later processing.
+  void HandleComment(SMLoc Loc, StringRef CommentText) {
+    unsigned LineNo = SrcMgr.FindLineNumber(Loc);
+    ParseHelper PH(CommentText);
+    while (!PH.Done()) {
+      // Search for token: expected
+      if (!PH.Search("expected", true)) {
+        break;
+      }
+      SMLoc DirectiveLoc = getTokenStartLoc(Loc, PH);
+      PH.Advance();
+
+      // Next token: -
+      if (!PH.Next("-")) {
+        continue;
+      }
+      PH.Advance();
+
+      // Parse diagnostic kind.
+      SourceMgr::DiagKind Kind;
+      if (PH.Next("error"))
+        Kind = SourceMgr::DK_Error;
+      else if (PH.Next("warning"))
+        Kind = SourceMgr::DK_Warning;
+      else if (PH.Next("note"))
+        Kind = SourceMgr::DK_Note;
+      else {
+        ReportError(getCurrentLoc(Loc, PH),
+                    "unsupported diagnostic severity");
+        continue;
+      }
+      PH.Advance();
+      PH.SkipWhitespace();
+
+      // Parse optional line number (absolute or offset from current line).
+      if (PH.Next("@")) {
+        int RelativeDir = 0;
+        unsigned ExplicitLineNo;
+        PH.Advance();
+        if (PH.Next("+")) {
+          RelativeDir = 1;
+          PH.Advance();
+        } else if (PH.Next("-")) {
+          RelativeDir = -1;
+          PH.Advance();
+        }
+        if (!PH.Next(ExplicitLineNo)) {
+          ReportError(getCurrentLoc(Loc, PH),
+                      "expected integer line number or offset");
+          continue;
+        }
+        PH.Advance();
+        if (RelativeDir == 0)
+          LineNo = ExplicitLineNo;
+        else
+          LineNo = LineNo + ((int)ExplicitLineNo * RelativeDir);
+      }
+
+      PH.SkipWhitespace();
+
+      // Parse the expected diagnostic text.
+      if (!PH.Next("{{")) {
+        ReportError(getCurrentLoc(Loc, PH),
+                    "cannot find start ('{{') of expected string");
+        continue;
+      }
+      PH.Advance();
+      const char *const ContentBegin = PH.C; // mark content begin
+
+      // Search for token: }}
+      if (!PH.SearchClosingBrace("{{", "}}")) {
+        ReportError(getCurrentLoc(Loc, PH),
+                    "cannot find end ('}}') of expected string");
+        continue;
+      }
+      const char *const ContentEnd = PH.P; // mark content end
+      PH.Advance();
+
+      ExpectedDiags.push_back(PendingDiag(
+          LineNo, StringRef(ContentBegin, ContentEnd - ContentBegin), Kind,
+          DirectiveLoc));
+    }
+  }
+
+  // The actual diagnostic callback called by the SourceMgr, this just converts
+  // the function ptr + context ptr into a member function call.
+  static void HandleDiagnosticCallback(const SMDiagnostic &Diag, void *Context) {
+    ((VerifyAsmDiagnosticConsumer*)Context)->HandleDiagnostic(Diag);
+  }
+
+  static const char *getDiagKindName(SourceMgr::DiagKind Kind) {
+    switch (Kind) {
+      case SourceMgr::DK_Error:   return "error";
+      case SourceMgr::DK_Warning: return "warning";
+      case SourceMgr::DK_Note:    return "note";
+    }
+    llvm_unreachable("unhandled enum value");
+  }
+
+  // Once the assembler has finished, check that the lists of expected and seen
+  // diagnostics agree, and emit errors for any that do not.
+  bool Finalize() {
+    // Sort the seen and expected diagnostics by line number. This doesn't have
+    // to be a stable sort for the checking to be correct, but it ensures we
+    // always emit our errors in a deterministic order.
+    std::stable_sort(SeenDiags.begin(), SeenDiags.end());
+    std::stable_sort(ExpectedDiags.begin(), ExpectedDiags.end());
+
+    auto SeenIt = SeenDiags.begin();
+    auto ExpectedIt = ExpectedDiags.begin();
+    auto SeenEnd = SeenDiags.end();
+    auto ExpectedEnd = ExpectedDiags.end();
+
+    while (SeenIt != SeenEnd || ExpectedIt != ExpectedEnd) {
+      // Find the lowest line number not yet processed, and check all seen and
+      // expected diagnostics on that line.
+      int LineNo =
+          std::min(SeenIt != SeenEnd ? SeenIt->LineNo : INT_MAX,
+                   ExpectedIt != ExpectedEnd ? ExpectedIt->LineNo : INT_MAX);
+      auto SeenNextLine = std::partition(
+          SeenIt, SeenEnd,
+          [LineNo](PendingDiag &D) { return D.LineNo == LineNo; });
+      auto ExpectedNextLine = std::partition(
+          ExpectedIt, ExpectedEnd,
+          [LineNo](PendingDiag &D) { return D.LineNo == LineNo; });
+
+      // Iterate over seen diags, checking for a matching expected diag.
+      for (; SeenIt != SeenNextLine; ++SeenIt) {
+        auto ExpectedMatch = std::find_if(ExpectedIt, ExpectedNextLine,
+                                          [SeenIt](PendingDiag &Expected) {
+          return SeenIt->DiagText.find(Expected.DiagText) !=
+                     std::string::npos &&
+                 SeenIt->Kind == Expected.Kind && !Expected.Matched;
+        });
+        if (ExpectedMatch != ExpectedNextLine) {
+          ExpectedMatch->Matched = true;
+        } else {
+          ReportMismatchError(*SeenIt, true);
+        }
+      }
+
+      // Iterate over expected diags, checking for ones that weren't matched by
+      // any seen ones.
+      for (; ExpectedIt != ExpectedNextLine; ++ExpectedIt) {
+        if (!ExpectedIt->Matched) {
+          ReportMismatchError(*ExpectedIt, false);
+        }
+      }
+    }
+
+    return HadError;
+  }
+};
+
 static int AssembleInput(const char *ProgName, const Target *TheTarget,
                          SourceMgr &SrcMgr, MCContext &Ctx, MCStreamer &Str,
                          MCAsmInfo &MAI, MCSubtargetInfo &STI,
-                         MCInstrInfo &MCII, MCTargetOptions &MCOptions) {
-  std::unique_ptr<MCAsmParser> Parser(
-      createMCAsmParser(SrcMgr, Ctx, Str, MAI));
+                         MCInstrInfo &MCII, MCTargetOptions &MCOptions,
+                         bool VerifyDiags) {
+  std::unique_ptr<MCAsmParser> Parser(createMCAsmParser(SrcMgr, Ctx, Str, MAI));
   std::unique_ptr<MCTargetAsmParser> TAP(
       TheTarget->createMCAsmParser(STI, *Parser, MCII, MCOptions));
 
+  VerifyAsmDiagnosticConsumer DiagChecker(SrcMgr, *Parser);
+  if (VerifyDiags) {
+    Parser->getLexer().setCommentConsumer(&DiagChecker);
+    SrcMgr.setDiagHandler(&VerifyAsmDiagnosticConsumer::HandleDiagnosticCallback,
+                          &DiagChecker);
+  }
+
   if (!TAP) {
     errs() << ProgName
            << ": error: this target does not support assembly parsing.\n";
     return 1;
   }
 
   int SymbolResult = fillCommandLineSymbols(*Parser);
   if(SymbolResult)
     return SymbolResult;
   Parser->setShowParsedOperands(ShowInstOperands);
   Parser->setTargetParser(*TAP);
 
   int Res = Parser->Run(NoInitialTextSection);
 
+  if (VerifyDiags) {
+    SrcMgr.setDiagHandler(nullptr, nullptr);
+    Res = DiagChecker.Finalize();
+  }
+
   return Res;
 }
 
 int main(int argc, char **argv) {
   // Print a stack trace if we signal out.
   sys::PrintStackTraceOnErrorSignal(argv[0]);
   PrettyStackTraceProgram X(argc, argv);
   llvm_shutdown_obj Y;  // Call llvm_shutdown() on exit.
int main(int argc, char **argv) {
 
   int Res = 1;
   bool disassemble = false;
   switch (Action) {
   case AC_AsLex:
     Res = AsLexInput(SrcMgr, *MAI, Out->os());
     break;
   case AC_Assemble:
+  case AC_AssembleVerifyDiags:
     Res = AssembleInput(ProgName, TheTarget, SrcMgr, Ctx, *Str, *MAI, *STI,
-                        *MCII, MCOptions);
+                        *MCII, MCOptions, Action == AC_AssembleVerifyDiags);
     break;
   case AC_MDisassemble:
     assert(IP && "Expected assembly output");
     IP->setUseMarkup(1);
     disassemble = true;
     break;
   case AC_Disassemble:
     disassemble = true;