Page MenuHomePhabricator

Use the first location in the fused location for diagnostic handler
ClosedPublic

Authored by liufengdb on Dec 23 2019, 9:34 PM.

Diff Detail

Unit TestsFailed

TimeTest
2,040 mslibc++.std/thread/thread_mutex/thread_mutex_requirements/thread_sharedtimedmutex_requirements/thread_sharedtimedmutex_class::Unknown Unit Message ("")
Compiled With: '/usr/bin/clang++ -o /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/Output/try_lock.pass.cpp.o -x c++ /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/try_lock.pass.cpp -c -v -ftemplate-depth=270 -Werror=thread-safety -std=c++2a -include /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support/nasty_macros.h -nostdinc++ -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/include -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support -DLIBCXX_FILESYSTEM_STATIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/std/input.output/filesystems/Inputs/static_test_env" -DLIBCXX_FILESYSTEM_DYNAMIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/test/filesystem/Output/dynamic_env" -DLIBCXX_FILESYSTEM_DYNAMIC_TEST_HELPER="/usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/support/filesystem_dynamic_test_helper.py" -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -Wall -Wextra -Werror -Wuser-defined-warnings -Wshadow -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -c && /usr/bin/clang++ -o /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/Output/try_lock.pass.cpp.exe…

Event Timeline

liufengdb created this revision.Dec 23 2019, 9:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2019, 9:34 PM

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.

Can you add a test case for this?

mlir/lib/IR/Diagnostics.cpp
384

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

465–466

Why not also use getCallSiteLoc here?

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

liufengdb updated this revision to Diff 235252.Dec 24 2019, 9:24 PM
  • fix river's comment

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

  • fix river's comment

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

  • fix river's comment

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 updated this revision to Diff 235381.Dec 26 2019, 4:38 PM
  • fix the stack
  • fix the typo
  • fix river's comment

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

Add unit test

remove unused include

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

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

This revision is now accepted and ready to land.Dec 27 2019, 3:36 AM
liufengdb updated this revision to Diff 235404.Dec 27 2019, 5:37 AM

create a test handler and add unit tests

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 requested changes to this revision.Dec 27 2019, 3:32 PM
rriddle added inline comments.
mlir/test/IR/diagnostic-handler.mlir
5 ↗(On Diff #235404)

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.

This revision now requires changes to proceed.Dec 27 2019, 3:32 PM
liufengdb marked an inline comment as done.Dec 28 2019, 10:25 AM
liufengdb added inline comments.
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 marked an inline comment as done and an inline comment as not done.Dec 28 2019, 11:54 AM
liufengdb added inline comments.
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 updated this revision to Diff 236453.Jan 6 2020, 2:07 PM
liufengdb marked an inline comment as not done.

rebase

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 updated this revision to Diff 236456.Jan 6 2020, 2:33 PM

fix the rebase error

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

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

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 updated this revision to Diff 243603.Feb 10 2020, 9:43 AM

Use the first location in the fused location for diagnostic handler

rriddle accepted this revision.Feb 10 2020, 9:49 AM
rriddle added inline comments.
mlir/lib/IR/Diagnostics.cpp
377

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

391

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?

This revision is now accepted and ready to land.Feb 10 2020, 9:49 AM

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

add a new line at the end

This revision was automatically updated to reflect the committed changes.