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

384 ↗

(On Diff #235202)

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

466 ↗

(On Diff #235202)

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

5 ↗

(On Diff #235404)

Add space after // lines.

mlir/test/lib/Transforms/TestDiagnosticHandler.cpp

1

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

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

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 ↗

(On Diff #243603)

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

392 ↗

(On Diff #243603)

Same here.

mlir/test/IR/diagnostic-handler.mlir

6 ↗

(On Diff #243603)

Start sentence with a capital letter.

14 ↗

(On Diff #243603)

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/test/lib/Transforms/TestDiagnosticHandler.cpp

 //===- TestDiagnosticHandler.cpp - Pass to test diagnostic handler --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/Dialect/StandardOps/Ops.h"
 #include "mlir/Pass/Pass.h"
 
 using namespace mlir;
 
 namespace {
 struct TestSourceMgrDiagnosticHandler : public SourceMgrDiagnosticHandler {
   explicit TestSourceMgrDiagnosticHandler(llvm::SourceMgr &mgr,
-                                                MLIRContext *ctx)
       : SourceMgrDiagnosticHandler(mgr, ctx, llvm::outs()) {}
 
   void emitTestDiagnostic(Operation *op) {
-        Diagnostic diag(op->getLoc(), DiagnosticSeverity::Note);
     emitDiagnostic(diag);
   }
 };
 
 /// Pass that emit diagnostic for each op.
 struct TestDiagnosticHandler : public FunctionPass<TestDiagnosticHandler> {
 
   void runOnFunction() override {
     llvm::SourceMgr fileSourceMgr;
-        TestSourceMgrDiagnosticHandler diagHandler(fileSourceMgr, &getContext());
     getFunction().walk([&](Operation *op) {
       if (isa<FuncOp>(op) || op->isKnownTerminator())
         return;
       diagHandler.emitTestDiagnostic(op);
     });
   }
 };
 
 } // end anonymous namespace
 
 static PassRegistration<TestDiagnosticHandler> pass("test-diagnostic-handler",
-                                                        "emit diagnostic for ops");