Skip to content

Commit 2dcc618

Browse files
committed
[RFC][libc++] Reworks clang-tidy selection.
The current selection is done in the test scipts which gives the user no control where to find clang-tidy. The version selection itself is hard-coded and not based on the version of clang used. This moves the selection to configuration time and tries to find a better match. - Mixing the version of clang-tidy and the clang libraries causes ODR violations. This results in practice in crashes or incorrect results. - Mixing the version of clang-tidy and the clang binaray sometimes causes issues with supported diagnotic flags. For example, flags tested against clang 17 may not be available in clang-tidy 16. Currently clang-tidy 18.1 can be used, it tests against the clang libraries version 18. This is a caused by the new LLVM version numbering scheme. The new selection tries to match the clang version or the version of the HEAD and the last 3 releases. (During the release period libc++ supports 4 versions instead of the typical 3 versions.)
1 parent ceaf4a0 commit 2dcc618

File tree

7 files changed

+69
-48
lines changed

7 files changed

+69
-48
lines changed

cmake/Modules/LLVMVersion.cmake

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# The LLVM Version number information
2+
3+
if(NOT DEFINED LLVM_VERSION_MAJOR)
4+
set(LLVM_VERSION_MAJOR 19)
5+
endif()
6+
if(NOT DEFINED LLVM_VERSION_MINOR)
7+
set(LLVM_VERSION_MINOR 0)
8+
endif()
9+
if(NOT DEFINED LLVM_VERSION_PATCH)
10+
set(LLVM_VERSION_PATCH 0)
11+
endif()
12+
if(NOT DEFINED LLVM_VERSION_SUFFIX)
13+
set(LLVM_VERSION_SUFFIX git)
14+
endif()
15+

libcxx/test/tools/CMakeLists.txt

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,5 +3,10 @@ set(LIBCXX_TEST_TOOLS_PATH ${CMAKE_CURRENT_BINARY_DIR} PARENT_SCOPE)
33

44
# TODO: Remove LIBCXX_ENABLE_CLANG_TIDY
55
if(LIBCXX_ENABLE_CLANG_TIDY)
6+
if(NOT CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
7+
message(STATUS "Clang-tidy can only be used when building libc++ with "
8+
"a clang compiler.")
9+
return()
10+
endif()
611
add_subdirectory(clang_tidy_checks)
712
endif()

libcxx/test/tools/clang_tidy_checks/CMakeLists.txt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@
55
set(LLVM_DIR_SAVE ${LLVM_DIR})
66
set(Clang_DIR_SAVE ${Clang_DIR})
77

8-
find_package(Clang 18)
9-
if (NOT Clang_FOUND)
10-
find_package(Clang 17)
11-
endif()
8+
# Since the Clang C++ ABI is not stable the Clang libraries and clang-tidy
9+
# versions must match. Otherwise there likely will be ODR-violations. This had
10+
# led to crashes and incorrect output of the clang-tidy based checks.
11+
find_package(Clang ${CMAKE_CXX_COMPILER_VERSION})
1212

1313
set(SOURCES
1414
abi_tag_on_virtual.cpp

libcxx/utils/libcxx/test/features.py

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -23,30 +23,6 @@
2323
_isMSVC = lambda cfg: "_MSC_VER" in compilerMacros(cfg)
2424
_msvcVersion = lambda cfg: (int(compilerMacros(cfg)["_MSC_VER"]) // 100, int(compilerMacros(cfg)["_MSC_VER"]) % 100)
2525

26-
27-
def _getSuitableClangTidy(cfg):
28-
try:
29-
# If we didn't build the libcxx-tidy plugin via CMake, we can't run the clang-tidy tests.
30-
if runScriptExitCode(cfg, ["stat %{test-tools-dir}/clang_tidy_checks/libcxx-tidy.plugin"]) != 0:
31-
return None
32-
33-
# TODO MODULES require ToT due module specific fixes.
34-
if runScriptExitCode(cfg, ['clang-tidy-18 --version']) == 0:
35-
return 'clang-tidy-18'
36-
37-
# TODO This should be the last stable release.
38-
# LLVM RELEASE bump to latest stable version
39-
if runScriptExitCode(cfg, ["clang-tidy-16 --version"]) == 0:
40-
return "clang-tidy-16"
41-
42-
# LLVM RELEASE bump version
43-
if int(re.search("[0-9]+", commandOutput(cfg, ["clang-tidy --version"])).group()) >= 16:
44-
return "clang-tidy"
45-
46-
except ConfigurationRuntimeError:
47-
return None
48-
49-
5026
def _getAndroidDeviceApi(cfg):
5127
return int(
5228
programOutput(
@@ -297,13 +273,6 @@ def _getAndroidDeviceApi(cfg):
297273
name="executor-has-no-bash",
298274
when=lambda cfg: runScriptExitCode(cfg, ["%{exec} bash -c 'bash --version'"]) != 0,
299275
),
300-
Feature(
301-
name="has-clang-tidy",
302-
when=lambda cfg: _getSuitableClangTidy(cfg) is not None,
303-
actions=[
304-
AddSubstitution("%{clang-tidy}", lambda cfg: _getSuitableClangTidy(cfg))
305-
],
306-
),
307276
# Whether module support for the platform is available.
308277
Feature(
309278
name="has-no-cxx-module-support",

libcxx/utils/libcxx/test/params.py

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,37 @@ def getSizeOptimizationFlag(cfg):
110110
)
111111

112112

113+
def testClangTidy(cfg, version, executable):
114+
try:
115+
if version in commandOutput(cfg, [f"{executable} --version"]):
116+
return executable
117+
except ConfigurationRuntimeError:
118+
return None
119+
120+
121+
def getSuitableClangTidy(cfg):
122+
# If we didn't build the libcxx-tidy plugin via CMake, we can't run the clang-tidy tests.
123+
if (
124+
runScriptExitCode(
125+
cfg, ["stat %{test-tools-dir}/clang_tidy_checks/libcxx-tidy.plugin"]
126+
)
127+
!= 0
128+
):
129+
return None
130+
131+
version = "{__clang_major__}.{__clang_minor__}.{__clang_patchlevel__}".format(
132+
**compilerMacros(cfg)
133+
)
134+
exe = testClangTidy(
135+
cfg, version, "clang-tidy-{__clang_major__}".format(**compilerMacros(cfg))
136+
)
137+
138+
if not exe:
139+
exe = testClangTidy(cfg, version, "clang-tidy")
140+
141+
return exe
142+
143+
113144
# fmt: off
114145
DEFAULT_PARAMETERS = [
115146
Parameter(
@@ -366,6 +397,16 @@ def getSizeOptimizationFlag(cfg):
366397
default=f"{shlex.quote(sys.executable)} {shlex.quote(str(Path(__file__).resolve().parent.parent.parent / 'run.py'))}",
367398
help="Custom executor to use instead of the configured default.",
368399
actions=lambda executor: [AddSubstitution("%{executor}", executor)],
369-
)
400+
),
401+
Parameter(
402+
name='clang-tidy-executable',
403+
type=str,
404+
default=lambda cfg: getSuitableClangTidy(cfg),
405+
help="Selects the clang-tidy executable to use.",
406+
actions=lambda exe: [] if exe is None else [
407+
AddFeature('has-clang-tidy'),
408+
AddSubstitution('%{clang-tidy}', exe),
409+
]
410+
),
370411
]
371412
# fmt: on

llvm/CMakeLists.txt

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,18 +15,7 @@ if(NOT LLVM_NO_INSTALL_NAME_DIR_FOR_BUILD_TREE)
1515
set(CMAKE_BUILD_WITH_INSTALL_NAME_DIR ON)
1616
endif()
1717

18-
if(NOT DEFINED LLVM_VERSION_MAJOR)
19-
set(LLVM_VERSION_MAJOR 19)
20-
endif()
21-
if(NOT DEFINED LLVM_VERSION_MINOR)
22-
set(LLVM_VERSION_MINOR 0)
23-
endif()
24-
if(NOT DEFINED LLVM_VERSION_PATCH)
25-
set(LLVM_VERSION_PATCH 0)
26-
endif()
27-
if(NOT DEFINED LLVM_VERSION_SUFFIX)
28-
set(LLVM_VERSION_SUFFIX git)
29-
endif()
18+
include(${LLVM_COMMON_CMAKE_UTILS}/Modules/LLVMVersion.cmake)
3019

3120
set_directory_properties(PROPERTIES LLVM_VERSION_MAJOR "${LLVM_VERSION_MAJOR}")
3221

runtimes/CMakeLists.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ set(LLVM_COMMON_CMAKE_UTILS "${CMAKE_CURRENT_SOURCE_DIR}/../cmake")
66
include(${LLVM_COMMON_CMAKE_UTILS}/Modules/CMakePolicy.cmake
77
NO_POLICY_SCOPE)
88

9+
include(${LLVM_COMMON_CMAKE_UTILS}/Modules/LLVMVersion.cmake)
10+
911
project(Runtimes C CXX ASM)
1012

1113
list(INSERT CMAKE_MODULE_PATH 0

0 commit comments

Comments
 (0)