This is an archive of the discontinued LLVM Phabricator instance.

enable plugins for clang-tidy
AbandonedPublic

Authored by vtjnash on Oct 4 2021, 3:43 PM.

Details

Summary

Fixes https://bugs.llvm.org//show_bug.cgi?id=32739, so pinging people who previously discussed that issue. This takes advantage of the existing plugin capabilities of llvm to add support for -load simply by requesting adding the header file to request it. I've tested out-of-tree something of this form (leaving out a few details of the check logic itself), and confirmed that it seems to load and get called with this patch:

#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang-tidy/ClangTidy.h"
#include "clang-tidy/ClangTidyCheck.h"
#include "clang-tidy/ClangTidyModule.h"
#include "clang-tidy/ClangTidyModuleRegistry.h"

using namespace clang;
using namespace clang::tidy;
using namespace clang::ast_matchers;

class MyTestCheck : public ClangTidyCheck;

namespace {
class CTTestModule : public ClangTidyModule {
public:
  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
    CheckFactories.registerCheck<MyTestCheck>("mytest");
  }
};
} // namespace

namespace clang {
namespace tidy {

// Register the CTTestTidyModule using this statically initialized variable.
static ClangTidyModuleRegistry::Add<::CTTestModule>
    X("mytest-module", "Adds my checks.");

// This anchor is used to force the linker to link in the generated object file
// and thus register the CTTestModule.
volatile int CTTestModuleAnchorSource = 0;

} // namespace tidy
} // namespace clang
$ ./bin/clang-tidy --checks=-*,mytest -list-checks -load mytestcheckplugin.so 
Enabled checks:
    mytest

Diff Detail

Event Timeline

vtjnash created this revision.Oct 4 2021, 3:43 PM
vtjnash requested review of this revision.Oct 4 2021, 3:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2021, 3:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vtjnash edited the summary of this revision. (Show Details)Oct 4 2021, 3:50 PM
vtjnash added a subscriber: Restricted Project.

bump? tagging some more people who seemed like possible reviewers. I realized my original list of candidates might have mostly been inactive people.

aaron.ballman requested changes to this revision.Nov 4 2021, 7:18 AM

Modifying the reviewer list a bit (feel free to re-add yourself if you were dropped but still wanted to provide review).

Thank you for working on this! Previously, the only way to get a clang-tidy plugin was to use a clang static analyzer plugin and run it through clang-tidy, which is a lot of hoops to jump through. However, that may raise an interesting question -- how well does this functionality work when there are static analyzer and tidy plugins (I'm presuming it'll run them both)?

At a minimum, I think these changes require test coverage. Also, the release notes should be updated to remark on the new functionality, documentation should be added that explains this is supported, and we may also want to consider adding an in-tree example (clang-tidy doesn't have an examples folder like Clang does, but this seems like a good reason for us to add one). But I think this is the correct direction to do -- this is functionality I've wished I had several times in the past.

This revision now requires changes to proceed.Nov 4 2021, 7:18 AM

Wait... Is the diff really supposed to be this small? You didn't include code... that one header takes care of everything?

Yes, this header does everything

brick added a subscriber: brick.Nov 17 2021, 10:25 AM
vtjnash updated this revision to Diff 389652.Nov 24 2021, 8:14 PM

add release notes, docs mention, and test example

vtjnash updated this revision to Diff 389653.Nov 24 2021, 8:15 PM

add release notes, docs mention, and test example

vtjnash updated this revision to Diff 389658.Nov 24 2021, 9:03 PM

enable help

This is probably just a draft, but please let me know what you think

There is clearly some more work to do to get the cmake file to be correct, but was hoping to check this looked like the direction you thought looked right for adding this test, since there isn't an obvious example to follow.

There is clearly some more work to do to get the cmake file to be correct, but was hoping to check this looked like the direction you thought looked right for adding this test, since there isn't an obvious example to follow.

My familiarity with plugins is pretty limited (I'm on Windows, where plugins don't work at all), but this looks like a reasonable direction to me.

vtjnash updated this revision to Diff 392835.Dec 8 2021, 10:58 AM
fix cmake
aaron.ballman added inline comments.Dec 16 2021, 11:19 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
392–394
clang-tools-extra/docs/clang-tidy/Contributing.rst
421–424 ↗(On Diff #392835)

I wordsmithed a bit and this should be similar to what you already had. I did remove the "while allowing some symbols to be undefined during linking" bit because I wasn't certain what that was about.

426–427 ↗(On Diff #392835)
clang-tools-extra/docs/clang-tidy/index.rst
223 ↗(On Diff #392835)
clang-tools-extra/test/clang-tidy/CTTestTidyModule.cpp
24–25 ↗(On Diff #392835)

Shouldn't these functions be overloaded? We don't need it to be particularly functional, but the plugin should demonstrate that it works and can be run by clang-tidy (not just loaded and listed as a check).

Thanks! I will work on making those changes

clang-tools-extra/test/clang-tidy/CTTestTidyModule.cpp
24–25 ↗(On Diff #392835)

I figured that is guaranteed by the C++ linker, if it can successfully list the check, so I didn't think it seemed essential to test for that also. I put these here mostly just to help anyone copying the file to getting started with adopting it to their use case.

aaron.ballman added inline comments.Jan 3 2022, 9:27 AM
clang-tools-extra/docs/clang-tidy/Contributing.rst
428 ↗(On Diff #392835)

We should also document our expectations explicitly regarding things like ABI and API stability. e.g., remind users that there is none and so the plugin must be compiled against the version of clang-tidy that will be loading the plugin. (We should also make sure we're correct about that; I don't know if Clang 13 and Clang 13.0.1 can share plugins or not.)

Another thing we should document is whether the plugins can or cannot use threads or TLS, or other details along those lines that developers need to be aware of.

clang-tools-extra/test/clang-tidy/CTTestTidyModule.cpp
24–25 ↗(On Diff #392835)

I think it's important to demonstrate that the functionality works, and I think it's even more important to ensure that plugin support is maintained as new features are added to clang-tidy. "It links" is insufficient to tell us that.

One possible approach to this is to write and maintain a simple check as a plugin, then test it using the normal infrastructure (additional command line arg to load the plugin notwithstanding). We could either make it a functional check that users can optionally use (perhaps the reason it's a plugin is because it relies on a third-party library that may not always be available on all users' systems), or we could make it a toy check that only exists to test tidy features like plugins, plugin config options, etc.

The kinds of things I want to verify work are things like: does the plugin name get properly emitted as part of the diagnostics from the check? Are config options properly loaded for the plugin? Can a plugin alias a builtin check and provides a different set of config options?

We should also verify the tool gracefully handles a plugin that isn't valid (e.g., ask to load an arbitrary shared library as a plugin, ensure that tidy doesn't crash).

vtjnash updated this revision to Diff 401997.Jan 21 2022, 8:25 AM

address review feedback

@aaron.ballman I think I incorporated all of your feedback. Is this okay for me to merge to main? I would like to get it in before the feature branch.

aaron.ballman accepted this revision.Jan 26 2022, 8:51 AM

Thanks for the fixes, this LGTM! However, if @alexfh can sign off as well as code owner, I'd appreciate it; this is a pretty big directional change and I think he should be on board for it. (If we don't hear back by Fri Jan 28, then I think this is okay to land.)

This revision is now accepted and ready to land.Jan 26 2022, 8:51 AM
This revision was automatically updated to reflect the committed changes.
phosek added a subscriber: phosek.Jan 29 2022, 5:14 PM

We started seeing CMake error after this change landed:

CMake Error at cmake/modules/AddLLVM.cmake:683 (add_dependencies):
  The dependency target "clang-tidy-headers" of target "CTTestTidyModule"
  does not exist.
Call Stack (most recent call first):
  /b/s/w/ir/x/w/llvm-llvm-project/clang-tools-extra/test/CMakeLists.txt:82 (llvm_add_library)

Would it be possible to revert it?

Yes, please push a revert so I can look later. Do you have a link to the buildbot configuration, so I can reproduce that?

Yes, please push a revert so I can look later. Do you have a link to the buildbot configuration, so I can reproduce that?

I did some debugging and it looks like this failure is due to -DLLVM_INSTALL_TOOLCHAIN_ONLY=ON in our toolchain build. The clang-tidy-headers is created conditionally only when LLVM_INSTALL_TOOLCHAIN_ONLY is OFF, see https://github.com/llvm/llvm-project/blob/ab3b89855c5318f0009e1f016ffe5b1483507fd0/clang-tools-extra/clang-tidy/CMakeLists.txt#L115.

It looks like this is probably the first time that a test was written for a PLUGIN_TOOL, outside of the docs, so we are likely in new territory here :/

There seem to be a couple of paths forward:

  1. disable the PLUGIN_TOOL option in cmake if the user has disabled the headers needed to make that usable to cmake
  2. define the headers target unconditionally (as done by https://github.com/llvm/llvm-project/blob/ab3b89855c5318f0009e1f016ffe5b1483507fd0/clang/lib/Headers/CMakeLists.txt#L232)
  3. disable the added test if the headers required to use plugins are disabled by the user
vtjnash reopened this revision.Feb 1 2022, 10:28 AM
This revision is now accepted and ready to land.Feb 1 2022, 10:28 AM
vtjnash updated this revision to Diff 404993.Feb 1 2022, 10:28 AM
  • Reland "enable plugins for clang-tidy"
  • fixup! Reland "enable plugins for clang-tidy": Disable the test if the user has disabled support for building it.

I decided it made the most sense to me to go with option 3, so this should be ready to land again.

aaron.ballman accepted this revision.Feb 1 2022, 11:09 AM

LGTM, I think this approach is worth trying. I agree with you that we're in a bit of new territory here regarding the testing.

mgorny reopened this revision.Feb 6 2022, 3:28 AM

This breaks build of clang against system-installed LLVM:

CMake Error at /usr/lib/llvm/14/lib64/cmake/llvm/AddLLVM.cmake:1821 (add_dependencies):
  The dependency target "LLVMHello" of target "check-all" does not exist.
Call Stack (most recent call first):
  CMakeLists.txt:574 (add_lit_target)

LLVMHello isn't installed, so you can't rely on it being available. The customary way to resolve this kind of issue would be to check whether the target is present, and build it via add_subdirectory() against the respective LLVM directory (see how LLVMTestingSupport is handled in unittests). That said, you will also probably need to account for different build path (it will land in clang's shlibdir).

This revision is now accepted and ready to land.Feb 6 2022, 3:28 AM
mgorny requested changes to this revision.Feb 6 2022, 3:28 AM
This revision now requires changes to proceed.Feb 6 2022, 3:28 AM

It is a somewhat worthless test IMO, and might belong better in LLVM itself (where this functionality is defined), but there does not appear to be any other like it currently, and it was requested by a previous reviewer. Comparing to the code in LLVMTestingSupport, does this fix it for you:

diff --git a/clang-tools-extra/test/CMakeLists.txt b/clang-tools-extra/test/CMakeLists.txt
index 9321457ae1a3..17cc12473565 100644
--- a/clang-tools-extra/test/CMakeLists.txt
+++ b/clang-tools-extra/test/CMakeLists.txt
@@ -87,6 +87,15 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
       PLUGIN_TOOL clang-tidy
       DEPENDS clang-tidy-headers)
 
+  if(CLANG_BUILT_STANDALONE)
+    # LLVMHello library is needed below
+    if (EXISTS ${LLVM_MAIN_SRC_DIR}/lib/Transforms/Hello
+       AND NOT TARGET LLVMHello)
+      add_subdirectory(${LLVM_MAIN_SRC_DIR}/lib/Transforms/Hello
+        lib/Transforms/Hello)
+    endif()
+  endif()
+
   if(TARGET CTTestTidyModule)
       list(APPEND CLANG_TOOLS_TEST_DEPS CTTestTidyModule LLVMHello)
       target_include_directories(CTTestTidyModule PUBLIC BEFORE "${CLANG_TOOLS_SOURCE_DIR}")
thakis added a comment.Feb 7 2022, 1:10 PM

This is still breaking tests on our bots: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8822909450036830977/+/u/package_clang/stdout?format=raw

FAIL: Clang Tools :: clang-tidy/CTTestTidyModule.cpp (17913 of 89667)
 ******************** TEST 'Clang Tools :: clang-tidy/CTTestTidyModule.cpp' FAILED ********************
 Script:
 --
 : 'RUN: at line 2';   clang-tidy -checks='-*,mytest*' --list-checks -load /b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/./lib/CTTestTidyModule.so -load /b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/./lib/LLVMHello.so | FileCheck --check-prefix=CHECK-LIST /b/s/w/ir/cache/builder/src/third_party/llvm/clang-tools-extra/test/clang-tidy/CTTestTidyModule.cpp
 : 'RUN: at line 6';   clang-tidy -checks='-*,mytest*,misc-definitions-in-headers' -load /b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/./lib/CTTestTidyModule.so /dev/null -- -xc 2>&1 | FileCheck /b/s/w/ir/cache/builder/src/third_party/llvm/clang-tools-extra/test/clang-tidy/CTTestTidyModule.cpp
 --
 Exit Code: 2
 
 Command Output (stderr):
 --
 Error opening '/b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/./lib/CTTestTidyModule.so': /b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/./lib/CTTestTidyModule.so: undefined symbol: _ZN5clang4tidy15ClangTidyModule16getModuleOptionsEv
   -load request ignored.
 Error opening '/b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/./lib/LLVMHello.so': /b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts/./lib/LLVMHello.so: undefined symbol: _ZN4llvm4PassD2Ev
   -load request ignored.
 No checks enabled.
 FileCheck error: '<stdin>' is empty.
 FileCheck command line:  FileCheck --check-prefix=CHECK-LIST /b/s/w/ir/cache/builder/src/third_party/llvm/clang-tools-extra/test/clang-tidy/CTTestTidyModule.cpp
 
 --
 
 ********************
 Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
 ********************
 Failed Tests (1):
   Clang Tools :: clang-tidy/CTTestTidyModule.cpp

The cmake invocation looks something like this:

Running cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=OFF '-DLLVM_ENABLE_PROJECTS=clang;compiler-rt;lld;clang-tools-extra' '-DLLVM_TARGETS_TO_BUILD=AArch64;ARM;Mips;PowerPC;SystemZ;WebAssembly;X86' -DLLVM_ENABLE_PIC=ON -DLLVM_ENABLE_UNWIND_TABLES=OFF -DLLVM_ENABLE_TERMINFO=OFF -DLLVM_ENABLE_Z3_SOLVER=OFF -DCLANG_PLUGIN_SUPPORT=OFF -DCLANG_ENABLE_STATIC_ANALYZER=OFF -DCLANG_ENABLE_ARCMT=OFF '-DBUG_REPORT_URL=https://crbug.com and run tools/clang/scripts/process_crashreports.py (only works inside Google) which will upload a report' -DLLVM_INCLUDE_GO_TESTS=OFF -DENABLE_X86_RELAX_RELOCATIONS=NO -DLLVM_ENABLE_DIA_SDK=OFF '-DCOMPILER_RT_SANITIZERS_TO_BUILD=asan;dfsan;msan;hwasan;tsan;cfi' -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF -DLLVM_ENABLE_CURL=OFF -DLLVM_LOCAL_RPATH=/b/s/w/ir/cache/builder/src/third_party/llvm-build-tools/gcc-10.2.0-trusty/lib64 '-DCOMPILER_RT_TEST_COMPILER_CFLAGS=--gcc-toolchain=/b/s/w/ir/cache/builder/src/third_party/llvm-build-tools/gcc-10.2.0-trusty -Wl,-rpath,/b/s/w/ir/cache/builder/src/third_party/llvm-build-tools/gcc-10.2.0-trusty/lib64 -Wl,-rpath,/b/s/w/ir/cache/builder/src/third_party/llvm-build-tools/gcc-10.2.0-trusty/lib32' -DLLVM_ENABLE_LIBXML2=FORCE_ON -DLIBXML2_INCLUDE_DIR=/b/s/w/ir/cache/builder/src/third_party/llvm-build-tools/libxml2-v2.9.12/build/install/include/libxml2 -DLIBXML2_LIBRARIES=/b/s/w/ir/cache/builder/src/third_party/llvm-build-tools/libxml2-v2.9.12/build/install/lib/libxml2.a -DLLVM_ENABLE_LLD=ON -DCMAKE_C_COMPILER=/b/s/w/ir/cache/builder/src/third_party/llvm-bootstrap-install/bin/clang -DCMAKE_CXX_COMPILER=/b/s/w/ir/cache/builder/src/third_party/llvm-bootstrap-install/bin/clang++ -DCOMPILER_RT_BUILD_CRT=ON -DCOMPILER_RT_BUILD_LIBFUZZER=OFF -DCOMPILER_RT_BUILD_MEMPROF=OFF -DCOMPILER_RT_BUILD_ORC=OFF -DCOMPILER_RT_BUILD_PROFILE=ON -DCOMPILER_RT_BUILD_SANITIZERS=ON -DCOMPILER_RT_BUILD_XRAY=OFF -DCOMPILER_RT_BUILD_BUILTINS=ON '-DCMAKE_C_FLAGS=-DLIBXML_STATIC --gcc-toolchain=/b/s/w/ir/cache/builder/src/third_party/llvm-build-tools/gcc-10.2.0-trusty' '-DCMAKE_CXX_FLAGS=-DLIBXML_STATIC --gcc-toolchain=/b/s/w/ir/cache/builder/src/third_party/llvm-build-tools/gcc-10.2.0-trusty' -DCMAKE_EXE_LINKER_FLAGS= -DCMAKE_SHARED_LINKER_FLAGS= -DCMAKE_MODULE_LINKER_FLAGS= -DCMAKE_INSTALL_PREFIX=/b/s/w/ir/cache/builder/src/third_party/llvm-build/Release+Asserts -DLLVM_EXTERNAL_PROJECTS=chrometools -DLLVM_EXTERNAL_CHROMETOOLS_SOURCE_DIR=/b/s/w/ir/cache/builder/src/tools/clang '-DCHROMIUM_TOOLS=blink_gc_plugin;plugins;translation_unit' -DLLVM_PROFDATA_FILE=/b/s/w/ir/cache/builder/src/third_party/llvm-instrumented/profdata.prof -DLLVM_ENABLE_LTO=Thin -DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-unknown-linux-gnu -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON /b/s/w/ir/cache/builder/src/third_party/llvm/llvm

Ah, this looks annoying: there are apparently two flags, CLANG_PLUGIN_SUPPORT and LLVM_ENABLE_PLUGINS, but the existing build for clang uses CLANG_PLUGIN_SUPPORT to turn off the build support and LLVM_ENABLE_PLUGINS to turn off the tests (you might not have noticed this existing issue since you turned off CLANG_ENABLE_STATIC_ANALYZER support, and CLANG_BUILD_EXAMPLES is off by default, which looks like it would disable all of the existing tests for this functionality). This might fix this particular test?

diff --git a/clang-tools-extra/test/CMakeLists.txt b/clang-tools-extra/test/CMakeLists.txt
index 9321457ae1a3..c98ec90a179b 100644
--- a/clang-tools-extra/test/CMakeLists.txt
+++ b/clang-tools-extra/test/CMakeLists.txt
@@ -17,7 +17,7 @@ string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} CLANG_TOOLS_DIR ${LLVM_RUN
 
 llvm_canonicalize_cmake_booleans(
   CLANG_TIDY_ENABLE_STATIC_ANALYZER
-  LLVM_ENABLE_PLUGINS
+  CLANG_PLUGIN_SUPPORT
   LLVM_INSTALL_TOOLCHAIN_ONLY
   )
 
diff --git a/clang-tools-extra/test/lit.site.cfg.py.in b/clang-tools-extra/test/lit.site.cfg.py.in
index e7db0e2ef2cb..d30e6664816b 100644
--- a/clang-tools-extra/test/lit.site.cfg.py.in
+++ b/clang-tools-extra/test/lit.site.cfg.py.in
@@ -12,7 +12,7 @@ config.clang_libs_dir = "@SHLIBDIR@"
 config.python_executable = "@Python3_EXECUTABLE@"
 config.target_triple = "@TARGET_TRIPLE@"
 config.clang_tidy_staticanalyzer = @CLANG_TIDY_ENABLE_STATIC_ANALYZER@
-config.has_plugins = @LLVM_ENABLE_PLUGINS@ & ~@LLVM_INSTALL_TOOLCHAIN_ONLY@
+config.has_plugins = @CLANG_PLUGIN_SUPPORT@ & ~@LLVM_INSTALL_TOOLCHAIN_ONLY@
 
 # Support substitution of the tools and libs dirs with user parameters. This is
 # used when we can't determine the tool dir at configuration time.

but it means there are a few other places that need to be changed also to make your configuration work fully.

mgorny added a comment.Feb 7 2022, 3:04 PM

It is a somewhat worthless test IMO, and might belong better in LLVM itself (where this functionality is defined), but there does not appear to be any other like it currently, and it was requested by a previous reviewer. Comparing to the code in LLVMTestingSupport, does this fix it for you:

Yes, this patch fixes it for me. Thank you! When you push it, please also request a backport to 14.x.

vtjnash accepted this revision.Feb 7 2022, 3:48 PM

Fixed by D119199

cristian.adam requested changes to this revision.Feb 8 2022, 4:34 AM
cristian.adam added a subscriber: cristian.adam.

I had on Windows two uses cases that failed due to this patch.

  1. MSVC2019 with -D LLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON
FAILED: bin/CTTestTidyModule.dll 
cmd.exe /C "cd . && C:\tools\cmake\bin\cmake.exe -E vs_link_dll --intdir=tools\clang\tools\extra\test\CMakeFiles\CTTestTidyModule.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100183~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100183~1.0\x64\mt.exe --manifests  -- C:\PROGRA~2\MICROS~1\2019\BUILDT~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\link.exe /nologo tools\clang\tools\extra\test\CMakeFiles\CTTestTidyModule.dir\clang-tidy\CTTestTidyModule.cpp.obj  /out:bin\CTTestTidyModule.dll /implib:lib\CTTestTidyModule.lib /pdb:bin\CTTestTidyModule.pdb /dll /version:0.0 /machine:x64 /INCREMENTAL:NO  lib\clang-tidy.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib  && cd ."
LINK: command "C:\PROGRA~2\MICROS~1\2019\BUILDT~1\VC\Tools\MSVC\1429~1.301\bin\Hostx64\x64\link.exe /nologo tools\clang\tools\extra\test\CMakeFiles\CTTestTidyModule.dir\clang-tidy\CTTestTidyModule.cpp.obj /out:bin\CTTestTidyModule.dll /implib:lib\CTTestTidyModule.lib /pdb:bin\CTTestTidyModule.pdb /dll /version:0.0 /machine:x64 /INCREMENTAL:NO lib\clang-tidy.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:bin\CTTestTidyModule.dll.manifest" failed (exit code 1120) with the following output:
CTTestTidyModule.cpp.obj : error LNK2001: unresolved external symbol "class clang::ast_matchers::internal::VariadicDynCastAllOfMatcher<class clang::Decl,class clang::TranslationUnitDecl> const clang::ast_matchers::translationUnitDecl" (?translationUnitDecl@ast_matchers@clang@@3V?$VariadicDynCastAllOfMatcher@VDecl@clang@@VTranslationUnitDecl@2@@internal@12@B)
bin\CTTestTidyModule.dll : fatal error LNK1120: 1 unresolved externals
  1. MinGW 11.2.0 with -D LLVM_BUILD_LLVM_DYLIB=ON -D LLVM_LINK_LLVM_DYLIB=ON -D CLANG_LINK_CLANG_DYLIB=ON
FAILED: bin/CTTestTidyModule.dll 
cmd.exe /C "cd . && C:\mingw64\bin\g++.exe -Wa,-mbig-obj -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing  -O2 -DNDEBUG  -Wl,--gc-sections -shared -o bin\CTTestTidyModule.dll -Wl,--major-image-version,0,--minor-image-version,0 tools/clang/tools/extra/test/CMakeFiles/CTTestTidyModule.dir/clang-tidy/CTTestTidyModule.cpp.obj  lib/libLLVM-14.dll.a  -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -ladvapi32 && cd ."
C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/11.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: tools/clang/tools/extra/test/CMakeFiles/CTTestTidyModule.dir/clang-tidy/CTTestTidyModule.cpp.obj:CTTestTidyModule.cpp:(.text$_ZNK5clang12ast_matchers8internal30matcher_hasDecayedType0Matcher7matchesERKNS_11DecayedTypeEPNS1_14ASTMatchFinderEPNS1_21BoundNodesTreeBuilderE[_ZNK5clang12ast_matchers8internal30matcher_hasDecayedType0Matcher7matchesERKNS_11DecayedTypeEPNS1_14ASTMatchFinderEPNS1_21BoundNodesTreeBuilderE]+0x1f): undefined reference to `clang::ast_matchers::internal::DynTypedMatcher::matches(clang::DynTypedNode const&, clang::ast_matchers::internal::ASTMatchFinder*, clang::ast_matchers::internal::BoundNodesTreeBuilder*) const'
...
cristian.adam resigned from this revision.Feb 9 2022, 2:10 PM
tstellar added inline comments.
clang-tools-extra/test/CMakeLists.txt
91 ↗(On Diff #405105)

Our stand-alone builds for Fedora are still not working after this patch, even with D119199. Why is it necesary to add LLVMHello as a test dependency? Which test is using it?

aaron.ballman added inline comments.Feb 17 2022, 4:54 AM
clang-tools-extra/test/CMakeLists.txt
91 ↗(On Diff #405105)

CTestTidyModule.cpp is using it:

// RUN: clang-tidy -checks='-*,mytest*' --list-checks -load %llvmshlibdir/CTTestTidyModule%pluginext -load %llvmshlibdir/LLVMHello%pluginext | FileCheck --check-prefix=CHECK-LIST %s

I *think* this is testing that we can load a clang-tidy plugin and an llvm plugin at the same time and not hit conflicting symbols or other issues. If I'm correct, then I think that's useful test functionality, but I wouldn't describe it as critical, so I'd be fine if we dropped it for now to get this patch in, and then added the extra testing in a subsequent patch if we think it's necessary.

tstellar added inline comments.Feb 17 2022, 9:01 AM
clang-tools-extra/test/CMakeLists.txt
91 ↗(On Diff #405105)

OK, that does sound like a useful test. I will spend some time investigating how to make this work with stand-alone builds.

vtjnash added inline comments.Feb 17 2022, 10:03 AM
clang-tools-extra/test/CMakeLists.txt
91 ↗(On Diff #405105)

Yes, that is the purpose. It is only a test dependency, so we could prevent it from running these tests instead. However, in theory, it should have figured out the relative path to the llvm/ source directory in the repo and re-built this target it if required for the test and not already available (with D119199):
https://github.com/llvm/llvm-project/blob/27f72eb25e366cf6fd79ea7495fec5d926a5b895/clang-tools-extra/test/CMakeLists.txt#L92

As this seemed to be the way other subprojects were implementing it already:
https://github.com/llvm/llvm-project/blob/27f72eb25e366cf6fd79ea7495fec5d926a5b895/clang-tools-extra/clangd/unittests/CMakeLists.txt#L9

I've proposed a fix for the standalone builds here D120301.

vtjnash abandoned this revision.Feb 25 2022, 11:38 AM