Use the first location in the fused location for diagnostic handler

[liufengdb]

Use the first location in the fused location for diagnostic handler

A new utility method is created to find out the first CallSiteLoc to display the stack.

[rriddle]

Can you add a test case for this?

mlir/lib/IR/Diagnostics.cpp

385

This looks wrong, seems like it should be retuning a CallSiteLoc.

476–477

Why not also use getCallSiteLoc here?

[mehdi_amini]

@liufengdb : can you join this group: https://reviews.llvm.org/project/members/78/ ; this will enable pre-merge testing on your review here.

[liufengdb]

• fix river's comment

[merge_guards_bot]

Unit tests: pass. 61110 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[liufengdb]

• fix river's comment

[merge_guards_bot]

Unit tests: fail. 61109 tests passed, 1 failed and 728 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_sharedtimedmutex_requirements/thread_sharedtimedmutex_class/try_lock.pass.cpp

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[liufengdb]

• fix river's comment

[merge_guards_bot]

Unit tests: pass. 61110 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[liufengdb]

• fix the stack
• fix the typo
• fix river's comment

[merge_guards_bot]

Unit tests: pass. 61129 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[liufengdb]

Add unit test

[liufengdb]

remove unused include

[merge_guards_bot]

Unit tests: pass. 61129 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: pass. 61129 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[liufengdb]

create a test handler and add unit tests

[merge_guards_bot]

Unit tests: pass. 61129 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[rriddle]

mlir/test/IR/diagnostic-handler.mlir

6

Add space after // lines.

mlir/test/lib/Transforms/TestDiagnosticHandler.cpp

1 ↗

(On Diff #235404)

Do we actually need an entirely new test driver? Seems like this could just be tested with a verifier error where the parsed operation has a callsite location.

[liufengdb]

mlir/test/lib/Transforms/TestDiagnosticHandler.cpp

1 ↗

(On Diff #235404)

If I understand the logic correctly, the op verifier error triggers the InflightDiagnosticHandler with the "specified location" in the test. The SourceMgrDiagnosticHandler in mlir-opt is using the location of the buggy op in the mlir file. I couldn't connect the "specified location" with the SourceMgrDiagnosticHandler, which is actually the test target. Did I miss something?

[liufengdb]

mlir/test/lib/Transforms/TestDiagnosticHandler.cpp

1 ↗

(On Diff #235404)

o, InflightDiagnostic isn't the handler.

The stack trace display is by a protected method "emitDiagnostic(Diagnostic &diag)" and the verifier only triggers "emitDiagnostic(Location loc, Twine message, DiagnosticSeverity kind)".

[liufengdb]

rebase

[merge_guards_bot]

Unit tests: pass. 61259 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[liufengdb]

fix the rebase error

[liufengdb]

fix

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: diff.json, console-log.txt

[merge_guards_bot]

Unit tests: pass. 61266 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[liufengdb]

Use the first location in the fused location for diagnostic handler

[rriddle]

mlir/lib/IR/Diagnostics.cpp

378

I wonder if we should try each of the locations until we find one that is usable?

392

Same here.

mlir/test/IR/diagnostic-handler.mlir

7

Start sentence with a capital letter.

14

Can we add a newline here?

[liufengdb]

find out the first available file line loc in the fused loc

[liufengdb]

add a new line at the end

mlir/lib/IR/Diagnostics.cpp

static Optional<FileLineColLoc> getFileLineColLoc(Location loc) {
   switch (loc->getKind()) {
   case StandardAttributes::NameLocation:
     return getFileLineColLoc(loc.cast<NameLoc>().getChildLoc());
   case StandardAttributes::FileLineColLocation:
     return loc.cast<FileLineColLoc>();
   case StandardAttributes::CallSiteLocation:
     // Process the callee of a callsite location.
     return getFileLineColLoc(loc.cast<CallSiteLoc>().getCallee());
+  case StandardAttributes::FusedLocation:
+    for (auto subLoc : loc.cast<FusedLoc>().getLocations()) {
+      if (auto callLoc = getFileLineColLoc(subLoc)) {
+        return callLoc;
+      }
+    }
+    return llvm::None;
+  default:
+    return llvm::None;
+  }
+}
+
+/// Return a processable CallSiteLoc from the given location.
+static Optional<CallSiteLoc> getCallSiteLoc(Location loc) {
+  switch (loc->getKind()) {
+  case StandardAttributes::NameLocation:
+    return getCallSiteLoc(loc.cast<NameLoc>().getChildLoc());
+  case StandardAttributes::CallSiteLocation:
+    return loc.cast<CallSiteLoc>();
+  case StandardAttributes::FusedLocation:
+    for (auto subLoc : loc.cast<FusedLoc>().getLocations()) {
+      if (auto callLoc = getCallSiteLoc(subLoc)) {
+        return callLoc;
+      }
+    }
+    return llvm::None;
   default:
     return llvm::None;
   }
 }
 
 /// Given a diagnostic kind, returns the LLVM DiagKind.
 static llvm::SourceMgr::DiagKind getDiagKind(DiagnosticSeverity kind) {
   switch (kind) {

 /// Emit the given diagnostic with the held source manager.
 void SourceMgrDiagnosticHandler::emitDiagnostic(Diagnostic &diag) {
   // Emit the diagnostic.
   auto loc = diag.getLocation();
   emitDiagnostic(loc, diag.str(), diag.getSeverity());
 
   // If the diagnostic location was a call site location, then print the call
   // stack as well.
-  if (auto callLoc = loc.dyn_cast<CallSiteLoc>()) {
+  if (auto callLoc = getCallSiteLoc(loc)) {
     // Print the call stack while valid, or until the limit is reached.
-    Location callerLoc = callLoc.getCaller();
+    Location callerLoc = callLoc->getCaller();
     for (unsigned curDepth = 0; curDepth < callStackLimit; ++curDepth) {
       emitDiagnostic(callerLoc, "called from", DiagnosticSeverity::Note);
-      if ((callLoc = callerLoc.dyn_cast<CallSiteLoc>()))
-        callerLoc = callLoc.getCaller();
-      else
+      callLoc = getCallSiteLoc(callerLoc);
+      if (!callLoc)
         break;
+      callerLoc = callLoc->getCaller();
     }
   }
 
   // Emit each of the notes.
   for (auto &note : diag.getNotes())
     emitDiagnostic(note.getLocation(), note.str(), note.getSeverity());
 }
 

mlir/test/IR/diagnostic-handler.mlir

+// RUN: not mlir-opt %s -o - 2>&1 | FileCheck %s
+// This test verifies that diagnostic handler can emit the call stack successfully.
+
+// -----
+
+// Emit the first available call stack in the fused location.
+func @constant_out_of_range() {
+  // CHECK: mysource1: error: 'std.constant' op requires attribute's type ('i64') to match op's return type ('i1')
+  // CHECK-NEXT: mysource2: note: called from
+  // CHECK-NEXT: mysource3: note: called from
+  %x = "std.constant"() {value = 100} : () -> i1 loc(fused["bar", callsite("foo"("mysource1":0:0) at callsite("mysource2":1:0 at "mysource3":2:0))])
+  return
+}