Make module std; and module std.compat; work with Clang#5982
Make module std; and module std.compat; work with Clang#5982cpplearner wants to merge 5 commits into
module std; and module std.compat; work with Clang#5982Conversation
c6ce684 to
5c99c5f
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves support for building and testing the STL named modules (module std; and module std.compat;) with Clang/clang-cl, primarily by adding Clang-specific diagnostic suppressions and adjusting module-related test build steps to use Clang’s module BMI precompile flow.
Changes:
- Add Clang diagnostic push/pop suppressions in
std.ixxandstd.compat.ixx(and a Clang/ARM64EC workaround include instd.ixx). - Extend the modules test matrix to include
clang-clconfigurations and refine the per-config flag composition. - Update module-related custom build scripts (Perl + Lit Python formats) to add Clang-specific
--precompilesteps and module compilation flags.
Show a summary per file
| File | Description |
|---|---|
| tests/std/tests/modules_20_matrix.lst | Adds clang-cl rows and restructures module test matrix flags. |
| tests/std/tests/VSO_1775715_user_defined_modules/custombuild.pl | Adds a Clang branch that precompiles the module interface before building. |
| tests/std/tests/VSO_1775715_user_defined_modules/custom_format.py | Adds Clang-specific Lit build steps for precompiling module BMIs. |
| tests/std/tests/P2465R3_standard_library_modules/custombuild.pl | Adds a Clang branch to precompile std.ixx/std.compat.ixx before building. |
| tests/std/tests/P2465R3_standard_library_modules/custom_format.py | Adds Clang-specific Lit build steps for precompiling standard library module BMIs. |
| tests/std/include/test_header_units_and_modules.hpp | Adjusts tests for Clang/ARM64EC generator workaround and Clang source_location::column() differences. |
| stl/modules/std.ixx | Adds Clang diagnostic suppressions and a Clang-specific include workaround in the global module fragment. |
| stl/modules/std.compat.ixx | Adds a global module fragment and Clang diagnostic suppression for reserved module identifier warnings. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 4
|
|
||
| # Dependency order is important here: | ||
| my @inputPaths = ("user.ixx", "test.cpp"); | ||
| if ($ENV{PM_COMPILER} =~ m/clang/) { |
There was a problem hiding this comment.
This Clang detection is case-sensitive ($ENV{PM_COMPILER} =~ m/clang/). If PM_COMPILER is provided with different casing (e.g. an absolute path with Clang-cl.exe), the Clang-specific module build steps will be skipped. Consider using a case-insensitive regex (/clang/i) to make the detection robust.
| if ($ENV{PM_COMPILER} =~ m/clang/) { | |
| if ($ENV{PM_COMPILER} =~ m/clang/i) { |
| my @traditionalUnits = ("test.cpp", "test2.cpp", "test3.cpp", "test4.cpp", "classic.cpp"); | ||
|
|
||
| Run::ExecuteCL(join(" ", @inputPaths, "/Fe$cwd.exe")); | ||
| if ($ENV{PM_COMPILER} =~ m/clang/) { |
There was a problem hiding this comment.
This Clang detection is case-sensitive ($ENV{PM_COMPILER} =~ m/clang/). If PM_COMPILER is set to a differently-cased value/path, the Clang module precompile path will be skipped and the build will likely fail. Consider using a case-insensitive regex (/clang/i).
| if ($ENV{PM_COMPILER} =~ m/clang/) { | |
| if ($ENV{PM_COMPILER} =~ m/clang/i) { |
| if 'clang' in test.cxx: | ||
| cmd = [test.cxx, '-x', 'c++-module', userIxx, '--precompile', *test.flags, *test.compileFlags] | ||
| yield TestStep(cmd, shared.execDir, shared.env, False) |
There was a problem hiding this comment.
The Clang detection is case-sensitive ('clang' in test.cxx). tests/utils/stl/test/tests.py treats compiler names case-insensitively (via casefold()), so this can mis-detect Clang if the compiler path contains different casing (e.g. Clang-cl.exe) and then skip the required --precompile steps. Consider switching to a case-insensitive check (e.g. based on os.path.basename(test.cxx).casefold()) or using the existing clang feature instead of substring matching.
See below for a potential fix:
compiler_name = os.path.basename(test.cxx).casefold()
if 'clang' in compiler_name:
There was a problem hiding this comment.
Yeah there's an existing feature.
| if 'clang' in test.cxx: | |
| cmd = [test.cxx, '-x', 'c++-module', userIxx, '--precompile', *test.flags, *test.compileFlags] | |
| yield TestStep(cmd, shared.execDir, shared.env, False) | |
| if 'clang' in test.config.available_features: | |
| cmd = [test.cxx, '-x', 'c++-module', userIxx, '--precompile', *test.flags, *test.compileFlags] | |
| yield TestStep(cmd, shared.execDir, shared.env, False) |
| traditionalUnits = [testCpp, test2Cpp, test3Cpp, test4Cpp, classicCpp] | ||
|
|
||
| cmd = [test.cxx, *inputPaths, *test.flags, *test.compileFlags] | ||
| if 'clang' in test.cxx: |
There was a problem hiding this comment.
The Clang detection is case-sensitive ('clang' in test.cxx). Elsewhere in the lit harness the compiler is identified case-insensitively (see tests/utils/stl/test/tests.py using casefold()), so this can fail to recognize Clang for differently-cased paths and skip the --precompile steps. Please make the check case-insensitive (preferably using os.path.basename(test.cxx).casefold()) or key off the existing clang feature.
| if 'clang' in test.cxx: | |
| if 'clang' in os.path.basename(test.cxx).casefold(): |
There was a problem hiding this comment.
| if 'clang' in test.cxx: | |
| if 'clang' in test.config.available_features: |
|
I'll keep this PR as draft until the next toolset update brings Clang 22. Good news: Clang 22 fixed LLVM-173689, which makes the Bad news: Clang 22 introduced |
|
Thank you; I will try to prioritize this when I can focus on the STL again. (I believe I'll need to write Perl to make this work in the internal test harness, like most things that need custom Python for the GitHub test harness, which makes this way more obnoxious to deal with, through no fault of your own.) |
|
The It will basically look like: diff --git a/tests/std/tests/P2465R3_standard_library_modules/custom_format.py b/tests/std/tests/P2465R3_standard_library_modules/custom_format.py
index d7d6a69f..1cca3ff6 100644
--- a/tests/std/tests/P2465R3_standard_library_modules/custom_format.py
+++ b/tests/std/tests/P2465R3_standard_library_modules/custom_format.py
@@ -22,7 +22,12 @@ class CustomTestFormat(STLTestFormat):
classicCpp = os.path.join(sourceDir, 'classic.cpp')
# Dependency order is important here:
- inputPaths = [stdIxx, stdCompatIxx, testCpp, test2Cpp, test3Cpp, test4Cpp, classicCpp]
+ moduleUnits = [stdIxx, stdCompatIxx]
+ traditionalUnits = [testCpp, test2Cpp, test3Cpp, test4Cpp, classicCpp]
+ if 'clang' in test.config.available_features:
+ inputPaths = ['-x', 'c++-module', *moduleUnits, '-x', 'none', *traditionalUnits]
+ else:
+ inputPaths = [*moduleUnits, *traditionalUnits]
cmd = [test.cxx, *inputPaths, *test.flags, *test.compileFlags] |
This adds Clang diagnostic suppressions as requested in #5975, and makes the module-related tests work with
clang-cl.Thanks to LLVM-98761 and LLVM-89772,
clang-clunderstands the Clang-style options needed to produce and consume Built Module Interfaces (BMIs). This is good enough for our test purposes.I need to guard a line that calls
source_location::column, because Clang and MSVC produce different values. This isn't specific to modules. The difference can also be found in theP1208R6_source_locationtest.I also need to workaround a bug, where clang thinks the declaration of
_allocain<malloc.h>has conflicting ownership. This looks similar to LLVM-37969, though the trigger is different. I've added a repro to that issue. Note that the bug only affects ARM64/ARM64EC, because<intrin.h>is included in the global module fragment, which includes<malloc.h>on x64/x86.Fixes #5975