This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add custom clang-tidy checks
ClosedPublic

Authored by philnik on Aug 16 2022, 6:34 AM.

Details

Reviewers
jdoerfert
ldionne
njames93
Group Reviewers
Restricted Project
Commits
rG841399a2188a: [libc++] Add custom clang-tidy checks

Diff Detail

Event Timeline

philnik created this revision.Aug 16 2022, 6:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2022, 6:34 AM

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.

philnik updated this revision to Diff 455594.Aug 25 2022, 8:18 AM
philnik marked 2 inline comments as done.
  • Address some comments
ldionne published this revision for review.Aug 25 2022, 8:34 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Aug 25 2022, 9:13 AM

I think we have to options here:

  1. Use the clang-tidy in the LLVM monorepo.
  2. 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.

ldionne added inline comments.Aug 25 2022, 9:13 AM
libcxx/CMakeLists.txt
319–321

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
This revision now requires changes to proceed.Aug 25 2022, 9:13 AM
philnik updated this revision to Diff 455921.Aug 26 2022, 8:34 AM
philnik marked 7 inline comments as done.
  • 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
316

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.

Could this be done with clang-query instead of a clang-tidy plugin?

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
316

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

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.

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
316

Can we exclude this file as asked above?

libcxx/utils/libcxx/test/features.py
24

I thought it changed the clang-tidy version use.

philnik marked 2 inline comments as done.Aug 31 2022, 12:05 PM

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.

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
316

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.

tschuett added a subscriber: tschuett.EditedAug 31 2022, 10:52 PM

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

ldionne added inline comments.Sep 2 2022, 9:11 AM
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
553

This looks like a leftover.

philnik updated this revision to Diff 461070.Sep 18 2022, 6:17 AM
philnik marked 5 inline comments as done.
  • Rebased
  • Address comments
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2022, 6:17 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
philnik updated this revision to Diff 462180.Sep 22 2022, 8:26 AM
  • Rebased
  • Try to fix CI
ldionne accepted this revision.Sep 23 2022, 8:33 AM
ldionne added subscribers: smeenai, beanz.

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.

This revision is now accepted and ready to land.Sep 23 2022, 8:33 AM
Mordante added inline comments.
libcxx/test/tools/clang_tidy_checks/robust_against_adl.cpp
23
jwakely added inline comments.Sep 25 2022, 9:41 AM
libcxx/test/tools/clang_tidy_checks/robust_against_adl.cpp
23

They're definitely customization points. If they weren't, specializing is_error_code_enum and is_error_condition_enum would be completely useless and serve no purpose.

LWG confirmed that and LWG 3629 is Tentatively Ready now.

jwakely added inline comments.Sep 30 2022, 4:55 AM
libcxx/test/tools/clang_tidy_checks/robust_against_adl.cpp
23

I created https://reviews.llvm.org/D134943 to fix those customization points.

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.

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?

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.

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?

That's a thing the vendor has to change, right? If we only get the targets which are actually available it should work just fine.

libcxx/test/tools/clang_tidy_checks/robust_against_adl.cpp
23

Thanks for the info and patch @jwakely!

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.

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?

That's a thing the vendor has to change, right? If we only get the targets which are actually available it should work just fine.

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
1 ↗(On Diff #463877)

This has been added in main now.

philnik updated this revision to Diff 478665.Nov 29 2022, 11:42 AM
philnik marked 4 inline comments as done.
  • Address comments
  • Make the checks optional for now
philnik updated this revision to Diff 478989.Nov 30 2022, 9:28 AM

Use the latest llvm-dev version

philnik updated this revision to Diff 482984.Dec 14 2022, 1:48 PM

Try to fix CI

philnik updated this revision to Diff 484591.Dec 21 2022, 8:28 AM

Try to fix CI

philnik updated this revision to Diff 484869.Dec 22 2022, 9:36 AM

Only run clang-tidy in some configurations

This revision was automatically updated to reflect the committed changes.
Mordante added inline comments.Dec 23 2022, 10:14 AM
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?