Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade clang-tidy to LLVM 16 #64699

Closed
wants to merge 5 commits into from
Closed

Conversation

BrettDong
Copy link
Member

@BrettDong BrettDong commented Mar 30, 2023

Summary

None

Purpose of change

We have custom clang-tidy checks, but the clang-tidy version we are using does not support plugins out-of-the-box. So we have to use a patched version of clang-tidy to run our custom checks: https://github.com/jbytheway/clang-tidy-plugin-support. This makes setting up clang-tidy checks in local development environments a little bit involved, especially for non-Debian/Ubuntu distro users.

Fortunately, plugin support landed in a later LLVM release: https://reviews.llvm.org/D111100. This simplifies clang-tidy workflow a lot. After building our custom checks as a shared library, we can just run the checks with a vanilla version of clang-tidy.

Describe the solution

  • Make custom clang-tidy checks compile with LLVM 16 API
  • Ensure lit test suite of custom clang-tidy checks pass
  • Run clang-tidy 16 on CDDA code base

Describe alternatives you've considered

Testing

Additional context

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` [Python] Code made in Python Code: Build Issues regarding different builds and build environments Code: Tooling Tooling that is not part of the main game but is part of the repo. Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. Translation I18n astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Mar 30, 2023
@BrettDong BrettDong force-pushed the llvm-16 branch 2 times, most recently from c9ca94e to 74ec1d3 Compare March 30, 2023 16:43
@BrettDong BrettDong added the Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style label Mar 30, 2023
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 30, 2023
@Qrox
Copy link
Contributor

Qrox commented Mar 30, 2023

Does it work on Windows? Asking because for the previous clang-tidy version I had to build the custom checks as the main executable rather than a shared library due to how Windows executable does not export its symbols.

@BrettDong
Copy link
Member Author

I don't really know, all information on clang-tidy --load= feature I can find is all on Unix systems.

@@ -57,26 +57,6 @@ endif ()
target_include_directories(${CataAnalyzerName} SYSTEM PRIVATE
${LLVM_INCLUDE_DIRS} ${CLANG_INCLUDE_DIRS})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (NOT "${CATA_CLANG_TIDY_INCLUDE_DIR}" STREQUAL "")
target_include_directories(${CataAnalyzerName} SYSTEM PRIVATE ${CATA_CLANG_TIDY_INCLUDE_DIR})
endif ()

This is still needed for compiling the custom executable on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"tool/ClangTidyMain.h" in line 43 should be changed to <clang-tidy/tool/ClangTidyMain.h> to match the rest of the header includes and to compile the custom executable correctly.

@Qrox
Copy link
Contributor

Qrox commented Mar 31, 2023

Do you plan to update the clang-tidy section in doc/DEVELOPER_TOOLING.md? I can update the Windows part but can't help with the Unix part.

@BrettDong
Copy link
Member Author

This is in early stage WIP now and the build workflow is not finalised. I’ll update the documentation once it’s made certain how to build.

@BrettDong
Copy link
Member Author

Meanwhile I would appreciate it if you could try whether —load a DLL works on Windows.

@BrettDong BrettDong force-pushed the llvm-16 branch 2 times, most recently from 4990b23 to 7c89034 Compare April 1, 2023 03:46
@andrei8l
Copy link
Contributor

andrei8l commented Apr 1, 2023

The plugin segfaults on my system while checking eoc_test.cpp

crash log
❯ LD_LIBRARY_PATH=/home/andrei/clang16/lib ~/clang16/bin/clang-tidy -p ../build_asan --load ../libCataAnalyzerPlugin.so  tests/eoc_test.cpp   >| log
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /home/andrei/clang16/bin/clang-tidy -p ../build_asan --load ../libCataAnalyzerPlugin.so tests/eoc_test.cpp
1.	<eof> parser at end of file
2.	ASTMatcher: Processing 'cata-almost-never-auto' against:
	VarDecl copy : <../cataclysm-dda-git/tests/catch/catch.hpp:3310:17, col:32>
--- Bound Nodes Begin ---
    decl - { VarDecl copy : <../cataclysm-dda-git/tests/catch/catch.hpp:3310:17, col:32> }
--- Bound Nodes End ---
 #0 0x0000564309d6b1e8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/andrei/clang16/bin/clang-tidy+0x3f6b1e8)
 #1 0x0000564309d6912e llvm::sys::RunSignalHandlers() (/home/andrei/clang16/bin/clang-tidy+0x3f6912e)
 #2 0x0000564309d6b98d SignalHandler(int) Signals.cpp:0:0
 #3 0x00007ff609251f50 (/usr/lib/libc.so.6+0x38f50)
 #4 0x0000564309344b8d clang::QualType::getSplitDesugaredType(clang::QualType) (/home/andrei/clang16/bin/clang-tidy+0x3544b8d)
 #5 0x0000564309344b29 clang::QualType::getDesugaredType(clang::QualType, clang::ASTContext const&) (/home/andrei/clang16/bin/clang-tidy+0x3544b29)
 #6 0x00007ff60910a17e clang::QualType::getTypePtr() const /home/andrei/clang16/include/clang/AST/Type.h:6634:26
 #7 0x00007ff60910a17e clang::tidy::cata::CheckDecl(clang::tidy::cata::AlmostNeverAutoCheck&, clang::ast_matchers::MatchFinder::MatchResult const&) /usr/src/cataclysm-dda-git/src/cataclysm-dda-git/tools/clang-tidy-plugin/AlmostNeverAutoCheck.cpp:95:67
 #8 0x0000564308f2e20d clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes const&) ASTMatchFinder.cpp:0:0
 #9 0x0000564308f5ba5c clang::ast_matchers::internal::BoundNodesTreeBuilder::visitMatches(clang::ast_matchers::internal::BoundNodesTreeBuilder::Visitor*) (/home/andrei/clang16/bin/clang-tidy+0x315ba5c)
#10 0x0000564308f2dc2e clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::matchWithFilter(clang::DynTypedNode const&) ASTMatchFinder.cpp:0:0
#11 0x0000564308f30510 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) ASTMatchFinder.cpp:0:0
#12 0x0000564308f402cc clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseDeclStmt(clang::DeclStmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*>>>>*) ASTMatchFinder.cpp:0:0
#13 0x0000564308f3d7eb clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseStmt(clang::Stmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*>>>>*) ASTMatchFinder.cpp:0:0
#14 0x0000564308f5b4bd clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseFunctionHelper(clang::FunctionDecl*) ASTMatchFinder.cpp:0:0
#15 0x0000564308f36383 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseCXXMethodDecl(clang::CXXMethodDecl*) ASTMatchFinder.cpp:0:0
#16 0x0000564308f30a66 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) ASTMatchFinder.cpp:0:0
#17 0x0000564308f3489b clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseCXXRecordDecl(clang::CXXRecordDecl*) ASTMatchFinder.cpp:0:0
#18 0x0000564308f3089e clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) ASTMatchFinder.cpp:0:0
#19 0x0000564308f33861 clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseClassTemplateDecl(clang::ClassTemplateDecl*) ASTMatchFinder.cpp:0:0
#20 0x0000564308f307f6 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) ASTMatchFinder.cpp:0:0
#21 0x0000564308f326bb clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseNamespaceDecl(clang::NamespaceDecl*) ASTMatchFinder.cpp:0:0
#22 0x0000564308f306d6 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) ASTMatchFinder.cpp:0:0
#23 0x0000564308f326bb clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseNamespaceDecl(clang::NamespaceDecl*) ASTMatchFinder.cpp:0:0
#24 0x0000564308f306d6 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) ASTMatchFinder.cpp:0:0
#25 0x0000564308f326bb clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseNamespaceDecl(clang::NamespaceDecl*) ASTMatchFinder.cpp:0:0
#26 0x0000564308f306d6 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) ASTMatchFinder.cpp:0:0
#27 0x0000564308f38a5b clang::RecursiveASTVisitor<clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor>::TraverseTranslationUnitDecl(clang::TranslationUnitDecl*) ASTMatchFinder.cpp:0:0
#28 0x0000564308f30d0c clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::TraverseDecl(clang::Decl*) ASTMatchFinder.cpp:0:0
#29 0x0000564308f04ccb clang::ast_matchers::MatchFinder::matchAST(clang::ASTContext&) (/home/andrei/clang16/bin/clang-tidy+0x3104ccb)
#30 0x00005643081aa91c clang::MultiplexConsumer::HandleTranslationUnit(clang::ASTContext&) (/home/andrei/clang16/bin/clang-tidy+0x23aa91c)
#31 0x00005643083c9ebd clang::ParseAST(clang::Sema&, bool, bool) (/home/andrei/clang16/bin/clang-tidy+0x25c9ebd)
#32 0x0000564308172427 clang::FrontendAction::Execute() (/home/andrei/clang16/bin/clang-tidy+0x2372427)
#33 0x00005643080ea094 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/home/andrei/clang16/bin/clang-tidy+0x22ea094)
#34 0x0000564307b89193 clang::tooling::FrontendActionFactory::runInvocation(std::shared_ptr<clang::CompilerInvocation>, clang::FileManager*, std::shared_ptr<clang::PCHContainerOperations>, clang::DiagnosticConsumer*) (/home/andrei/clang16/bin/clang-tidy+0x1d89193)
#35 0x0000564307b53fd4 clang::tidy::runClangTidy(clang::tidy::ClangTidyContext&, clang::tooling::CompilationDatabase const&, llvm::ArrayRef<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>, bool, bool, llvm::StringRef)::ActionFactory::runInvocation(std::shared_ptr<clang::CompilerInvocation>, clang::FileManager*, std::shared_ptr<clang::PCHContainerOperations>, clang::DiagnosticConsumer*) ClangTidy.cpp:0:0
#36 0x0000564307b88ee6 clang::tooling::ToolInvocation::runInvocation(char const*, clang::driver::Compilation*, std::shared_ptr<clang::CompilerInvocation>, std::shared_ptr<clang::PCHContainerOperations>) (/home/andrei/clang16/bin/clang-tidy+0x1d88ee6)
#37 0x0000564307b87e4b clang::tooling::ToolInvocation::run() (/home/andrei/clang16/bin/clang-tidy+0x1d87e4b)
#38 0x0000564307b8aa7a clang::tooling::ClangTool::run(clang::tooling::ToolAction*) (/home/andrei/clang16/bin/clang-tidy+0x1d8aa7a)
#39 0x0000564307b4eef1 clang::tidy::runClangTidy(clang::tidy::ClangTidyContext&, clang::tooling::CompilationDatabase const&, llvm::ArrayRef<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>, llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>, bool, bool, llvm::StringRef) (/home/andrei/clang16/bin/clang-tidy+0x1d4eef1)
#40 0x0000564306e6e199 clang::tidy::clangTidyMain(int, char const**) (/home/andrei/clang16/bin/clang-tidy+0x106e199)
#41 0x00007ff60923c790 (/usr/lib/libc.so.6+0x23790)
#42 0x00007ff60923c84a __libc_start_main (/usr/lib/libc.so.6+0x2384a)
#43 0x0000564306e6981a _start (/home/andrei/clang16/bin/clang-tidy+0x106981a)

I built the plugin with this
cmake -G Ninja ../cataclysm-dda-git -DCLANG_INSTALL_PREFIX=/home/andrei/clang16 -DCATA_CLANG_TIDY_PLUGIN=true -DClang_DIR=/home/andrei/clang16/lib/cmake/clang -DLLVM_DIR=/home/andrei/clang16/lib/cmake/llvm -DCMAKE_BUILD_TYPE=Release

with the ubuntu binaries from upstream

@BrettDong BrettDong closed this Apr 5, 2023
@sonphantrung
Copy link
Contributor

Why closed? Does it have something to do with #64852?

@jbytheway
Copy link
Contributor

Does it work on Windows?

A comment on the LLVM PR says "I'm on Windows, where plugins don't work at all", so I guess not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Code: Build Issues regarding different builds and build environments Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Code: Tooling Tooling that is not part of the main game but is part of the repo. Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. json-styled JSON lint passed, label assigned by github actions [Python] Code made in Python Translation I18n
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants