Details
- Reviewers
jdoerfert ldionne njames93 - Group Reviewers
Restricted Project - Commits
- rG841399a2188a: [libc++] Add custom clang-tidy checks
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
This is really interesting! If you can get me more details about what FindClang.cmake looks like (I wasn't able to find after searching quickly), I might be able to provide more guidance on how to integrate it.
libcxx/test/configs/cmake-bridge.cfg.in | ||
---|---|---|
33 | Instead, maybe we could introduce a directory where we install build tools like this and use --load=%{build-tools}/libcxx-tidy.*? | |
libcxx/test/libcxx/clang_tidy_checks/CMakeLists.txt | ||
16 ↗ | (On Diff #452985) | This relies on the fact that we used AddLLVM in the top-level CMake. This is like a weird hybrid between using the system-installed clang-tidy and the one we have in the monorepo. I think we should do the former. I would expect something like this (pseudo code): // In the Dockerfile sudo apt-get install clang-devel # that installs development headers and stuff for clang/clang-tidy/etc // In the CMake find_package(Clang) add_library(libcxx_tidy SHARED ${SOURCES}) target_link_libraries(libcxx_tidy PUBLIC Clang::clang-tidy-lib) I actually expect that something like this should be all that's needed if Clang does provide a working FindClang.cmake (IDK whether it does, though). |
libcxx/utils/libcxx/test/features.py | ||
160 | Instead, has-clang-tidy should represent whether we have clang-tidy *AND* the development headers have been installed. That way, we can check for has-clang-tidy only in the tests. |
I think we have to options here:
- Use the clang-tidy in the LLVM monorepo.
- Use a system-provided clang-tidy if there is one, and disable these tests otherwise.
I think (1) is a non-starter, because we can't start depending on clang-tidy (which depends on clang, LLVM, etc.) inside libc++. That would require doing a bootstrapping build every time we want to run the tests.
So I think we have to to (2), and simply make sure that our Docker image in the CI does have the development packages for clang-tidy.
libcxx/test/tools/clang_tidy_checks/CMakeLists.txt | ||
---|---|---|
10 | Instead, let's do if(NOT Clang_FOUND OR ${LLVM_VERSION_MAJOR} LESS 15) message(STATUS "Could not find a suitable version of the Clang development package, custom libc++ clang-tidy checks will not be available.") return() endif() And in fact, we need to also bail out if we *do* find Clang, but if the development package is not installed. Otherwise, the includes and .a files needed below won't be found. | |
11–12 | ||
12 | I would like to make it really clear that we're not using the in-tree version of headers/libraries. I suggest we add a target like this (since find_package(Clang) does not seem to do that): add_library(clang-tidy-devel IMPORTED) set_target_properties(clang-tidy-devel PROPERTIES IMPORTED_LOCATION "${?????}/libclangTidy.a") target_include_directories(clang-tidy-devel INTERFACE "${LLVM_INCLUDE_DIRS}" "${CLANG_INCLUDE_DIRS}" "${LLVM_MAIN_INCLUDE_DIR}/../../clang-tools-extra") target_compile_options(clang-tidy-devel INTERFACE -fno-rtti) Then, we can link cxx-tidy against clang-tidy-devel. | |
14–16 | I'm not sure why we'd need those. We already set that at the top-level. |
libcxx/CMakeLists.txt | ||
---|---|---|
316–318 ↗ | (On Diff #455594) | Let's remove this and make this automatic based on whether we have the dev packages. Basically, I don't see a reason to disable this if we can have them enabled. |
libcxx/test/tools/clang_tidy_checks/CMakeLists.txt | ||
6 | Does this render our other tests for robustness against ADL obsolete? If so, we should take one of those, make it regress, and confirm that this clang-tidy check will catch it. If it doesn't, we should understand why. Basically, I'd like us to have a clear understanding of whether we need to write manual tests for ADL robustness or not. | |
19 | This can be removed if we have the INTERFACE flag suggested above. | |
25 | Based on the CMake documentation, this should already be the default. I don't understand why this is needed -- can we try removing it and see how it fails? | |
26 | Also, how about using .plugin instead? It's more platform agnostic. | |
libcxx/utils/libcxx/test/features.py | ||
24 | That doesn't work. You're manually expanding the substitution, but substitutions can contains substitutions themselves and so on. We should instead do: int(re.search('[0-9]+', commandOutput(cfg, ['clang-tidy --version'])).group()) >= 13 and runScriptExitCode(['stat %{test-tools}/clang_tidy_checks/libcxx-tidy.plugin']) == 0 |
- Rebased
- Address comments
libcxx/test/tools/clang_tidy_checks/CMakeLists.txt | ||
---|---|---|
6 | I think we should do that in a follow-up where we remove the current ADL tests and make sure clang-tidy catches all the cases. If clang-tidy doesn't catch something we should find out why and fix the clang-tidy check. | |
14–16 | We use set_target_properties(${target} PROPERTIES CXX_STANDARD 20 CXX_STANDARD_REQUIRED YES CXX_EXTENSIONS NO) at the top level. Should I do the same here? | |
25 | When I remove it the output file is set as /home/nikolask/llvm-projects/default/build/lib/libcxx-tidy.plugin. I don't know why it doesn't put it in the normal location. |
Could this be done with clang-query instead of a clang-tidy plugin?
libcxx/include/charconv | ||
---|---|---|
302 | Why is this code moved? I am working on this header so the changes probably cause merge conflicts. In general I dislike moving code in these kind of patches since they may cause unneeded merge conflicts. Would it be possible to exclude this file and add a TODO so we can fix this after the consexpr version of charconv is done? | |
libcxx/utils/libcxx/test/features.py | ||
24 | I think it would be better when the user specifies the path to clang-tidy in CMake. Our Docker image contains multiple versions of clang-tidy; one for main and one for the release branch. To be able to use both branches the build bot configuration should be able to select the proper version. It is already intended to remove the symlinks from the Docker file for that reason. |
No. With a clang-tidy plugin we can get all the information in the AST, which we can't through clang-query. See AST_MATCHER(...) in robust_against_adl.cpp for examples. Using a clang-tidy plugin also has the benefit of getting proper error messages and (potentially) IDE integration. If we're ambitious maybe even fixit hints.
libcxx/include/charconv | ||
---|---|---|
302 | It's moved because it calls the __to_chars_itoa in line 295. This wasn't checked before until the functions is called I think. | |
libcxx/utils/libcxx/test/features.py | ||
24 | Ok, but what has that to do with this change? |
is there a reason why we add our own clang-tidy plugin instead of upstreaming this to clang-tidy itself? I could imagine that other library developers could also benefit from this clang-tidy check, if we make the list of allowed customization points a configuration option
We can definitely do this for some checks, but I don't think it makes sense for all checks. I'd like to extend our _LIBCPP_HIDE_FROM_ABI check for example, but that check probably doesn't make sense for any other project. Upstreaming a check would also mean that we'd have to wait up to six months before being able to enforce it.
I too prefer to upstream useful changes; _LIBCPP_HIDE_FROM_ABI is indeed not one of them. I disagree with your assessment. We can do both, implement it locally and upstream it at the same time. That way we can use it directly and remove the local version half a year later.
libcxx/include/charconv | ||
---|---|---|
302 | Can we exclude this file as asked above? | |
libcxx/utils/libcxx/test/features.py | ||
24 | I thought it changed the clang-tidy version use. |
I didn't mean that we should never upstream it. I meant that if we didn't add a clang-tidy plugin that we'd have to wait up to six months before enabling a check. I'd also rather wait to upstream this check, since it's not very well tested yet. I'd like to check if it catches all ADL that the current ADL tests catch and if not improve the check in a follow-up.
libcxx/include/charconv | ||
---|---|---|
302 | I'll do that if this lands before your patches, which I don't really expect. There is a reason I didn't consider the comment done yet. |
The LLVM libc has "official" clang tidy checks:
https://github.com/llvm/llvm-project/tree/main/clang-tools-extra/clang-tidy/llvmlibc
I guess there is a buildbot that builds libc and clang-tools-extra.
Even with their own documentation:
https://libc.llvm.org/clang_tidy_checks.html
libcxx/test/tools/clang_tidy_checks/CMakeLists.txt | ||
---|---|---|
3 | You should be able to skip the version check below that way. We'll also have to fix the ClangConfig.cmake file, I don't see a good way of moving forward without that. | |
14–16 | Yes, that seems like the correct thing to do to me. | |
33 | ||
libcxx/utils/ci/run-buildbot | ||
507 ↗ | (On Diff #455921) | This looks like a leftover. |
I'd be fine with this as-is if it works in the CI.
IIUC, the current blocker for this is that the ClangConfig.cmake installed by LLVM is not robust to the dev packages missing. If you do find_package(Clang 16.0), it will error out if the dev packages are not present, since it will try to reference static libraries (and other artifacts) that don't exist and try to create IMPORTED targets for those. @smeenai @beanz do you know more about that, or do you know someone that does? Do you know who set up the CMake config files so that find_package(Clang) would work in the first place? I'd also welcome your input on the ClangConfigVersion.cmake.in changes.
Just for the context, what we're trying to do here is simply use clang-tidy's development packages from the libc++ build to add our own custom clang-tidy checks.
Accepting because this LGTM, although we do have some stuff to resolve before this can be shipped.
libcxx/test/tools/clang_tidy_checks/CMakeLists.txt | ||
---|---|---|
40 | ||
libcxx/test/tools/clang_tidy_checks/robust_against_adl.cpp | ||
23 | This is funny, I actually saw a LWG issue about that go by a few weeks ago. I'll try to get more details. |
libcxx/test/tools/clang_tidy_checks/robust_against_adl.cpp | ||
---|---|---|
23 | @ldionne @jwakely posted this bug report about it https://github.com/llvm/llvm-project/issues/57614 |
libcxx/test/tools/clang_tidy_checks/robust_against_adl.cpp | ||
---|---|---|
23 | I created https://reviews.llvm.org/D134943 to fix those customization points. |
IIRC, the intended solution was to use LLVM_DISTRIBUTION_COMPONENTS (https://llvm.org/docs/BuildingADistribution.html). When you use that option, the generated CMake package files (at least in the install tree; I'm not sure about the ones in the build tree) should only contain imported targets that were part of your distribution. Multi-distribution support extends this even further, where the file defining the imported targets for a distribution is only imported if it's present, so things should work as expected both with and without the dev packages. Is that workable for your use case?
Correct, that would have to be set up by whoever distributes the LLVM package you're using.
Let's make those checks optional until we can figure out the packaging stuff, and land this (with green CI). This is going to become way too stale otherwise.
clang/cmake/modules/ClangConfigVersion.cmake.in | ||
---|---|---|
2 | This has been added in main now. |
libcxx/test/libcxx/clang_tidy.sh.cpp | ||
---|---|---|
15 | I actually think it would be better to do this is a separate script. Is there any documentation on how to run this test? |
This has been added in main now.