[llvm-branch-commits] [libcxx] [libc++][modules] Adds module testing. (PR #76246)

Mark de Wever via llvm-branch-commits llvm-branch-commits at lists.llvm.org
Tue Jan 16 11:41:21 PST 2024


https://github.com/mordante updated https://github.com/llvm/llvm-project/pull/76246

>From eebe9b2fab5c5eef4776852270bf70af4626cfcb Mon Sep 17 00:00:00 2001
From: Mark de Wever <koraq at xs4all.nl>
Date: Wed, 20 Dec 2023 20:43:38 +0100
Subject: [PATCH 1/3] [libc++][modules] Adds module testing.

This adds a new module test infrastructure. This requires tagging tests
using modules. The test runner uses this information to determine the
compiler flags needed to build and use the module.

Currently modules are build per test, which allows testing them for
tests with ADDITIONAL_COMPILE_FLAGS. At the moment only 4 tests use
modules. Therefore the performance penalty is not measurable. If in the
future more tests use modules it would be good to measure the overhead
and determine whether it's acceptable.
---
 libcxx/docs/TestingLibcxx.rst                 |  9 ++-
 libcxx/modules/std/memory.inc                 |  2 +
 libcxx/test/libcxx/module_std.gen.py          |  1 +
 libcxx/test/libcxx/module_std_compat.gen.py   |  1 +
 .../libcxx/selftest/modules/no-modules.sh.cpp | 14 ++++
 .../modules/std-and-std.compat-module.sh.cpp  | 22 ++++++
 .../libcxx/selftest/modules/std-module.sh.cpp | 24 +++++++
 .../selftest/modules/std.compat-module.sh.cpp | 24 +++++++
 libcxx/test/std/modules/std.compat.pass.cpp   |  5 +-
 libcxx/test/std/modules/std.pass.cpp          |  5 +-
 libcxx/utils/libcxx/test/features.py          | 12 ++++
 libcxx/utils/libcxx/test/format.py            | 71 +++++++++++++++++++
 libcxx/utils/libcxx/test/modules.py           |  5 +-
 13 files changed, 190 insertions(+), 5 deletions(-)
 create mode 100644 libcxx/test/libcxx/selftest/modules/no-modules.sh.cpp
 create mode 100644 libcxx/test/libcxx/selftest/modules/std-and-std.compat-module.sh.cpp
 create mode 100644 libcxx/test/libcxx/selftest/modules/std-module.sh.cpp
 create mode 100644 libcxx/test/libcxx/selftest/modules/std.compat-module.sh.cpp

diff --git a/libcxx/docs/TestingLibcxx.rst b/libcxx/docs/TestingLibcxx.rst
index e7645cb5885f14..50ee9d4ee400b4 100644
--- a/libcxx/docs/TestingLibcxx.rst
+++ b/libcxx/docs/TestingLibcxx.rst
@@ -394,7 +394,7 @@ Custom Directives
 ~~~~~~~~~~~~~~~~~
 
 Lit has many directives built in (e.g., ``DEFINE``, ``UNSUPPORTED``). In addition to those directives, libc++ adds two additional libc++-specific directives that makes
-writing tests easier. See `libc++-specific Lit Directives`_ for more information about the ``FILE_DEPENDENCIES`` and ``ADDITIONAL_COMPILE_FLAGS`` libc++-specific directives.
+writing tests easier. See `libc++-specific Lit Directives`_ for more information about the ``FILE_DEPENDENCIES``, ``ADDITIONAL_COMPILE_FLAGS``, and ``MODULE_DEPENDENCIES`` libc++-specific directives.
 
 .. _libc++-specific Lit Directives:
 .. list-table:: libc++-specific Lit Directives
@@ -417,6 +417,13 @@ writing tests easier. See `libc++-specific Lit Directives`_ for more information
      - The additional compiler flags specified by a space-separated list to the ``ADDITIONAL_COMPILE_FLAGS`` libc++-specific Lit directive will be added to the end of the ``%{compile_flags}``
        substitution for the test that contains it. This libc++-specific Lit directive makes it possible to add special compilation flags without having to resort to writing a ``.sh.cpp`` test (see
        `Lit Meaning of libc++ Test Filenames`_), more powerful but perhaps overkill.
+   * - ``MODULE_DEPENDENCIES``
+     - ``// MODULE_DEPENDENCIES: std std.compat``
+     - This directive will build the required C++23 standard library
+       modules and add the additional compiler flags in
+       %{compile_flags}. (Libc++ offers these modules in C++20 as an
+       extension.)
+
 
 Benchmarks
 ==========
diff --git a/libcxx/modules/std/memory.inc b/libcxx/modules/std/memory.inc
index fba2461732c1b9..ef89845457fbb9 100644
--- a/libcxx/modules/std/memory.inc
+++ b/libcxx/modules/std/memory.inc
@@ -156,7 +156,9 @@ export namespace std {
   using std::reinterpret_pointer_cast;
   using std::static_pointer_cast;
 
+#ifndef _LIBCPP_HAS_NO_RTTI
   using std::get_deleter;
+#endif // _LIBCPP_HAS_NO_RTTI
 
   // [util.smartptr.shared.io], shared_ptr I/O
 
diff --git a/libcxx/test/libcxx/module_std.gen.py b/libcxx/test/libcxx/module_std.gen.py
index 8e03d6e5b5b523..a9a05a0cd74e61 100644
--- a/libcxx/test/libcxx/module_std.gen.py
+++ b/libcxx/test/libcxx/module_std.gen.py
@@ -30,6 +30,7 @@
     "%{test-tools}/clang_tidy_checks/libcxx-tidy.plugin",
     "%{cxx}",
     "%{flags} %{compile_flags}",
+    "std",
 )
 
 
diff --git a/libcxx/test/libcxx/module_std_compat.gen.py b/libcxx/test/libcxx/module_std_compat.gen.py
index c4792db3d71e62..2866066ccedc89 100644
--- a/libcxx/test/libcxx/module_std_compat.gen.py
+++ b/libcxx/test/libcxx/module_std_compat.gen.py
@@ -30,6 +30,7 @@
     "%{test-tools}/clang_tidy_checks/libcxx-tidy.plugin",
     "%{cxx}",
     "%{flags} %{compile_flags}",
+    "std.compat",
 )
 
 
diff --git a/libcxx/test/libcxx/selftest/modules/no-modules.sh.cpp b/libcxx/test/libcxx/selftest/modules/no-modules.sh.cpp
new file mode 100644
index 00000000000000..74e65a9072f7b6
--- /dev/null
+++ b/libcxx/test/libcxx/selftest/modules/no-modules.sh.cpp
@@ -0,0 +1,14 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// Make sure that the compile flags contain no module information.
+
+// MODULE_DEPENDENCIES:
+
+// RUN: echo "%{compile_flags}" | grep -v "std.pcm"
+// RUN: echo "%{compile_flags}" | grep -v "std.compat.pcm"
diff --git a/libcxx/test/libcxx/selftest/modules/std-and-std.compat-module.sh.cpp b/libcxx/test/libcxx/selftest/modules/std-and-std.compat-module.sh.cpp
new file mode 100644
index 00000000000000..89b32f96c3416a
--- /dev/null
+++ b/libcxx/test/libcxx/selftest/modules/std-and-std.compat-module.sh.cpp
@@ -0,0 +1,22 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// UNSUPPORTED: clang-modules-build
+// UNSUPPORTED: gcc
+
+// XFAIL: has-no-cxx-module-support
+
+// Make sure that the compile flags contain the expected elements.
+// The tests only look for the expected components and not the exact flags.
+// Otherwise changing the location of the module breaks this test.
+
+// MODULE_DEPENDENCIES: std std.compat
+
+// RUN: echo "%{compile_flags}" | grep -- "-fmodule-file=std=.*/std.pcm .*/std.pcm"
+// RUN: echo "%{compile_flags}" | grep -- "-fmodule-file=std.compat=.*/std.compat.pcm .*/std.compat.pcm"
diff --git a/libcxx/test/libcxx/selftest/modules/std-module.sh.cpp b/libcxx/test/libcxx/selftest/modules/std-module.sh.cpp
new file mode 100644
index 00000000000000..919c7d1c210cac
--- /dev/null
+++ b/libcxx/test/libcxx/selftest/modules/std-module.sh.cpp
@@ -0,0 +1,24 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// UNSUPPORTED: clang-modules-build
+// UNSUPPORTED: gcc
+
+// XFAIL: has-no-cxx-module-support
+
+// Make sure that the compile flags contain the expected elements.
+// The tests only look for the expected components and not the exact flags.
+// Otherwise changing the location of the module breaks this test.
+
+// MODULE_DEPENDENCIES: std
+
+// RUN: echo "%{compile_flags}" | grep -- "-fmodule-file=std=.*/std.pcm .*/std.pcm"
+
+// The std module should not provide the std.compat module
+// RUN: echo "%{compile_flags}" | grep -v "std.compat.pcm"
diff --git a/libcxx/test/libcxx/selftest/modules/std.compat-module.sh.cpp b/libcxx/test/libcxx/selftest/modules/std.compat-module.sh.cpp
new file mode 100644
index 00000000000000..0bc76f4d53f76b
--- /dev/null
+++ b/libcxx/test/libcxx/selftest/modules/std.compat-module.sh.cpp
@@ -0,0 +1,24 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// UNSUPPORTED: clang-modules-build
+// UNSUPPORTED: gcc
+
+// XFAIL: has-no-cxx-module-support
+
+// Make sure that the compile flags contain the expected elements.
+// The tests only look for the expected components and not the exact flags.
+// Otherwise changing the location of the module breaks this test.
+
+// MODULE_DEPENDENCIES: std.compat
+
+// RUN: echo "%{compile_flags}" | grep -- "-fmodule-file=std.compat=.*/std.compat.pcm .*/std.compat.pcm"
+
+// It's unspecified whether std.compat is built on the std module.
+// Therefore don't test its presence
diff --git a/libcxx/test/std/modules/std.compat.pass.cpp b/libcxx/test/std/modules/std.compat.pass.cpp
index ba75f8e4010027..e840f3c6b629cf 100644
--- a/libcxx/test/std/modules/std.compat.pass.cpp
+++ b/libcxx/test/std/modules/std.compat.pass.cpp
@@ -8,11 +8,14 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-modules-build
+// UNSUPPORTED: gcc
 
-// XFAIL: *
+// XFAIL: has-no-cxx-module-support
 
 // A minimal test to validate import works.
 
+// MODULE_DEPENDENCIES: std.compat
+
 import std.compat;
 
 int main(int, char**) { return !(::strlen("Hello modular world") == 19); }
diff --git a/libcxx/test/std/modules/std.pass.cpp b/libcxx/test/std/modules/std.pass.cpp
index a018e42a265891..ca05825b3a1863 100644
--- a/libcxx/test/std/modules/std.pass.cpp
+++ b/libcxx/test/std/modules/std.pass.cpp
@@ -8,11 +8,14 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 // UNSUPPORTED: clang-modules-build
+// UNSUPPORTED: gcc
 
-// XFAIL: *
+// XFAIL: has-no-cxx-module-support
 
 // A minimal test to validate import works.
 
+// MODULE_DEPENDENCIES: std
+
 import std;
 
 int main(int, char**) {
diff --git a/libcxx/utils/libcxx/test/features.py b/libcxx/utils/libcxx/test/features.py
index 332def02980ca6..6387a5c0de007b 100644
--- a/libcxx/utils/libcxx/test/features.py
+++ b/libcxx/utils/libcxx/test/features.py
@@ -317,6 +317,18 @@ def _getAndroidDeviceApi(cfg):
             AddSubstitution("%{clang-tidy}", lambda cfg: _getSuitableClangTidy(cfg))
         ],
     ),
+    # Whether module support for the platform is available.
+    Feature(
+        name="has-no-cxx-module-support",
+        # The libc of these platforms have functions with internal linkage.
+        # This is not allowed per C11 7.1.2 Standard headers/6
+        #  Any declaration of a library function shall have external linkage.
+        when=lambda cfg: "__ANDROID__" in compilerMacros(cfg)
+        or "__PICOLIBC__" in compilerMacros(cfg)
+        or platform.system().lower().startswith("aix")
+        # Avoid building on platforms that don't support modules properly.
+        or not hasCompileFlag(cfg, "-Wno-reserved-module-identifier"),
+    ),
 ]
 
 # Deduce and add the test features that that are implied by the #defines in
diff --git a/libcxx/utils/libcxx/test/format.py b/libcxx/utils/libcxx/test/format.py
index 5d84711bf5d282..45c0a3d2378c12 100644
--- a/libcxx/utils/libcxx/test/format.py
+++ b/libcxx/utils/libcxx/test/format.py
@@ -52,6 +52,21 @@ def _executeScriptInternal(test, litConfig, commands):
     return (out, err, exitCode, timeoutInfo, parsedCommands)
 
 
+def _validateModuleDependencies(modules):
+    for m in modules:
+        if m not in ("std", "std.compat"):
+            raise RuntimeError(
+                f"Invalid module dependency '{m}', only 'std' and 'std.compat' are valid"
+            )
+
+
+def _getSubstitution(substitution, config):
+    for (orig, replacement) in config.substitutions:
+        if orig == substitution:
+            return replacement
+    raise ValueError("Substitution {} is not in the config.".format(substitution))
+
+
 def parseScript(test, preamble):
     """
     Extract the script from a test, with substitutions applied.
@@ -91,6 +106,8 @@ def parseScript(test, preamble):
     # Parse the test file, including custom directives
     additionalCompileFlags = []
     fileDependencies = []
+    modules = []  # The enabled modules
+    moduleCompileFlags = []  # The compilation flags to use modules
     parsers = [
         lit.TestRunner.IntegratedTestKeywordParser(
             "FILE_DEPENDENCIES:",
@@ -102,6 +119,11 @@ def parseScript(test, preamble):
             lit.TestRunner.ParserKind.SPACE_LIST,
             initial_value=additionalCompileFlags,
         ),
+        lit.TestRunner.IntegratedTestKeywordParser(
+            "MODULE_DEPENDENCIES:",
+            lit.TestRunner.ParserKind.SPACE_LIST,
+            initial_value=modules,
+        ),
     ]
 
     # Add conditional parsers for ADDITIONAL_COMPILE_FLAGS. This should be replaced by first
@@ -132,6 +154,8 @@ def parseScript(test, preamble):
     script += scriptInTest
 
     # Add compile flags specified with ADDITIONAL_COMPILE_FLAGS.
+    # Modules need to be build with the same compilation flags as the
+    # test. So add these flags before adding the modules.
     substitutions = [
         (s, x + " " + " ".join(additionalCompileFlags))
         if s == "%{compile_flags}"
@@ -139,6 +163,53 @@ def parseScript(test, preamble):
         for (s, x) in substitutions
     ]
 
+    if modules:
+        _validateModuleDependencies(modules)
+
+        # This flag is needed for both modules.
+        #moduleCompileFlags.append("-fprebuilt-module-path=%T")
+
+        # The moduleCompileFlags are added to the %{compile_flags}, but
+        # the modules need should be built without these flags. So
+        # expand the compile_flags and add the expanded value to the
+        # build script.
+        compileFlags = _getSubstitution("%{compile_flags}", test.config)
+
+        # Building the modules needs to happen before the other script
+        # commands are executed. Therefore the commands are added to the
+        # front of the list.
+        if "std.compat" in modules:
+            script.insert(
+                0,
+                "%dbg(MODULE std.compat) %{cxx} %{flags} "
+                f"{compileFlags} "
+                "-Wno-reserved-module-identifier -Wno-reserved-user-defined-literal "
+                "--precompile -o %T/std.compat.pcm -c %{module}/std.compat.cppm",
+            )
+            moduleCompileFlags.append("-fmodule-file=std.compat=%T/std.compat.pcm %T/std.compat.pcm")
+
+        # Make sure the std module is added before std.compat. Libc++'s
+        # std.compat module will depend on its std module.  It is not
+        # known whether the compiler expects the modules in the order of
+        # their dependencies. However it's trivial to provide them in
+        # that order.
+        script.insert(
+            0,
+            "%dbg(MODULE std) %{cxx} %{flags} "
+            f"{compileFlags} "
+            "-Wno-reserved-module-identifier -Wno-reserved-user-defined-literal "
+            "--precompile -o %T/std.pcm -c %{module}/std.cppm",
+        )
+        moduleCompileFlags.append("-fmodule-file=std=%T/std.pcm %T/std.pcm")
+
+        # Add compile flags required for the modules.
+        substitutions = [
+            (s, x + " " + " ".join(moduleCompileFlags))
+            if s == "%{compile_flags}"
+            else (s, x)
+            for (s, x) in substitutions
+        ]
+
     # Perform substitutions in the script itself.
     script = lit.TestRunner.applySubstitutions(
         script, substitutions, recursion_limit=test.config.recursiveExpansionLimit
diff --git a/libcxx/utils/libcxx/test/modules.py b/libcxx/utils/libcxx/test/modules.py
index 9362d52cb72d2c..3e9fcae4c5389a 100644
--- a/libcxx/utils/libcxx/test/modules.py
+++ b/libcxx/utils/libcxx/test/modules.py
@@ -113,6 +113,7 @@ class module_test_generator:
     clang_tidy_plugin: str
     compiler: str
     compiler_flags: str
+    module: str
 
     def write_lit_configuration(self):
         print(
@@ -120,13 +121,13 @@ def write_lit_configuration(self):
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: clang-modules-build
 
-// XFAIL: *
-
 // REQUIRES: has-clang-tidy
 
 // The GCC compiler flags are not always compatible with clang-tidy.
 // UNSUPPORTED: gcc
 
+// MODULE_DEPENDENCIES: {self.module}
+
 // RUN: echo -n > {self.tmp_prefix}.all_partitions
 """
         )

>From 47dc15e7cf89cb27180324b633604bcbded18f6d Mon Sep 17 00:00:00 2001
From: Mark de Wever <koraq at xs4all.nl>
Date: Tue, 16 Jan 2024 19:18:14 +0100
Subject: [PATCH 2/3] Apply suggestions from code review

Co-authored-by: Louis Dionne <ldionne.2 at gmail.com>
---
 .../modules/std-and-std.compat-module.sh.cpp   |  2 +-
 .../libcxx/selftest/modules/std-module.sh.cpp  |  2 +-
 .../selftest/modules/std.compat-module.sh.cpp  |  2 +-
 libcxx/utils/libcxx/test/format.py             | 18 +++++++-----------
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/libcxx/test/libcxx/selftest/modules/std-and-std.compat-module.sh.cpp b/libcxx/test/libcxx/selftest/modules/std-and-std.compat-module.sh.cpp
index 89b32f96c3416a..d56ebb2961d4ae 100644
--- a/libcxx/test/libcxx/selftest/modules/std-and-std.compat-module.sh.cpp
+++ b/libcxx/test/libcxx/selftest/modules/std-and-std.compat-module.sh.cpp
@@ -14,7 +14,7 @@
 
 // Make sure that the compile flags contain the expected elements.
 // The tests only look for the expected components and not the exact flags.
-// Otherwise changing the location of the module breaks this test.
+// Otherwise changing the location of the module would break this test.
 
 // MODULE_DEPENDENCIES: std std.compat
 
diff --git a/libcxx/test/libcxx/selftest/modules/std-module.sh.cpp b/libcxx/test/libcxx/selftest/modules/std-module.sh.cpp
index 919c7d1c210cac..ec43994fa1ef97 100644
--- a/libcxx/test/libcxx/selftest/modules/std-module.sh.cpp
+++ b/libcxx/test/libcxx/selftest/modules/std-module.sh.cpp
@@ -14,7 +14,7 @@
 
 // Make sure that the compile flags contain the expected elements.
 // The tests only look for the expected components and not the exact flags.
-// Otherwise changing the location of the module breaks this test.
+// Otherwise changing the location of the module would break this test.
 
 // MODULE_DEPENDENCIES: std
 
diff --git a/libcxx/test/libcxx/selftest/modules/std.compat-module.sh.cpp b/libcxx/test/libcxx/selftest/modules/std.compat-module.sh.cpp
index 0bc76f4d53f76b..e84709853fbca1 100644
--- a/libcxx/test/libcxx/selftest/modules/std.compat-module.sh.cpp
+++ b/libcxx/test/libcxx/selftest/modules/std.compat-module.sh.cpp
@@ -14,7 +14,7 @@
 
 // Make sure that the compile flags contain the expected elements.
 // The tests only look for the expected components and not the exact flags.
-// Otherwise changing the location of the module breaks this test.
+// Otherwise changing the location of the module would break this test.
 
 // MODULE_DEPENDENCIES: std.compat
 
diff --git a/libcxx/utils/libcxx/test/format.py b/libcxx/utils/libcxx/test/format.py
index 45c0a3d2378c12..c8110f38d87a7f 100644
--- a/libcxx/utils/libcxx/test/format.py
+++ b/libcxx/utils/libcxx/test/format.py
@@ -154,7 +154,7 @@ def parseScript(test, preamble):
     script += scriptInTest
 
     # Add compile flags specified with ADDITIONAL_COMPILE_FLAGS.
-    # Modules need to be build with the same compilation flags as the
+    # Modules need to be built with the same compilation flags as the
     # test. So add these flags before adding the modules.
     substitutions = [
         (s, x + " " + " ".join(additionalCompileFlags))
@@ -166,13 +166,9 @@ def parseScript(test, preamble):
     if modules:
         _validateModuleDependencies(modules)
 
-        # This flag is needed for both modules.
-        #moduleCompileFlags.append("-fprebuilt-module-path=%T")
-
         # The moduleCompileFlags are added to the %{compile_flags}, but
-        # the modules need should be built without these flags. So
-        # expand the compile_flags and add the expanded value to the
-        # build script.
+        # the modules need to be built without these flags. So expand the
+        # %{compile_flags} eagerly and hardcode them in the build script.
         compileFlags = _getSubstitution("%{compile_flags}", test.config)
 
         # Building the modules needs to happen before the other script
@@ -186,10 +182,10 @@ def parseScript(test, preamble):
                 "-Wno-reserved-module-identifier -Wno-reserved-user-defined-literal "
                 "--precompile -o %T/std.compat.pcm -c %{module}/std.compat.cppm",
             )
-            moduleCompileFlags.append("-fmodule-file=std.compat=%T/std.compat.pcm %T/std.compat.pcm")
+            moduleCompileFlags.extend(["-fmodule-file=std.compat=%T/std.compat.pcm", "%T/std.compat.pcm"])
 
-        # Make sure the std module is added before std.compat. Libc++'s
-        # std.compat module will depend on its std module.  It is not
+        # Make sure the std module is built before std.compat. Libc++'s
+        # std.compat module depends on the std module. It is not
         # known whether the compiler expects the modules in the order of
         # their dependencies. However it's trivial to provide them in
         # that order.
@@ -200,7 +196,7 @@ def parseScript(test, preamble):
             "-Wno-reserved-module-identifier -Wno-reserved-user-defined-literal "
             "--precompile -o %T/std.pcm -c %{module}/std.cppm",
         )
-        moduleCompileFlags.append("-fmodule-file=std=%T/std.pcm %T/std.pcm")
+        moduleCompileFlags.extend(["-fmodule-file=std=%T/std.pcm", "%T/std.pcm"])
 
         # Add compile flags required for the modules.
         substitutions = [

>From dd2d7cecec8f3adff284b9c747092f4d35de24ed Mon Sep 17 00:00:00 2001
From: Mark de Wever <koraq at xs4all.nl>
Date: Tue, 16 Jan 2024 20:20:27 +0100
Subject: [PATCH 3/3] Address review comments.

---
 libcxx/utils/libcxx/test/config.py |  4 ++++
 libcxx/utils/libcxx/test/format.py | 32 ++++++++++--------------------
 2 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/libcxx/utils/libcxx/test/config.py b/libcxx/utils/libcxx/test/config.py
index 63cfa1b6778198..9a71c494030d70 100644
--- a/libcxx/utils/libcxx/test/config.py
+++ b/libcxx/utils/libcxx/test/config.py
@@ -16,6 +16,10 @@ def _getSubstitution(substitution, config):
     raise ValueError("Substitution {} is not in the config.".format(substitution))
 
 
+def _appendToSubstitution(substitutions, key, value):
+    return [(k, v + " " + value) if k == key else (k, v) for (k, v) in substitutions]
+
+
 def configure(parameters, features, config, lit_config):
     note = lambda s: lit_config.note("({}) {}".format(config.name, s))
     config.environment = dict(os.environ)
diff --git a/libcxx/utils/libcxx/test/format.py b/libcxx/utils/libcxx/test/format.py
index c8110f38d87a7f..c605e8796ac542 100644
--- a/libcxx/utils/libcxx/test/format.py
+++ b/libcxx/utils/libcxx/test/format.py
@@ -7,6 +7,7 @@
 # ===----------------------------------------------------------------------===##
 
 import lit
+import libcxx.test.config as config
 import lit.formats
 import os
 import re
@@ -60,13 +61,6 @@ def _validateModuleDependencies(modules):
             )
 
 
-def _getSubstitution(substitution, config):
-    for (orig, replacement) in config.substitutions:
-        if orig == substitution:
-            return replacement
-    raise ValueError("Substitution {} is not in the config.".format(substitution))
-
-
 def parseScript(test, preamble):
     """
     Extract the script from a test, with substitutions applied.
@@ -156,12 +150,9 @@ def parseScript(test, preamble):
     # Add compile flags specified with ADDITIONAL_COMPILE_FLAGS.
     # Modules need to be built with the same compilation flags as the
     # test. So add these flags before adding the modules.
-    substitutions = [
-        (s, x + " " + " ".join(additionalCompileFlags))
-        if s == "%{compile_flags}"
-        else (s, x)
-        for (s, x) in substitutions
-    ]
+    substitutions = config._appendToSubstitution(
+        substitutions, "%{compile_flags}", " ".join(additionalCompileFlags)
+    )
 
     if modules:
         _validateModuleDependencies(modules)
@@ -169,7 +160,7 @@ def parseScript(test, preamble):
         # The moduleCompileFlags are added to the %{compile_flags}, but
         # the modules need to be built without these flags. So expand the
         # %{compile_flags} eagerly and hardcode them in the build script.
-        compileFlags = _getSubstitution("%{compile_flags}", test.config)
+        compileFlags = config._getSubstitution("%{compile_flags}", test.config)
 
         # Building the modules needs to happen before the other script
         # commands are executed. Therefore the commands are added to the
@@ -182,7 +173,9 @@ def parseScript(test, preamble):
                 "-Wno-reserved-module-identifier -Wno-reserved-user-defined-literal "
                 "--precompile -o %T/std.compat.pcm -c %{module}/std.compat.cppm",
             )
-            moduleCompileFlags.extend(["-fmodule-file=std.compat=%T/std.compat.pcm", "%T/std.compat.pcm"])
+            moduleCompileFlags.extend(
+                ["-fmodule-file=std.compat=%T/std.compat.pcm", "%T/std.compat.pcm"]
+            )
 
         # Make sure the std module is built before std.compat. Libc++'s
         # std.compat module depends on the std module. It is not
@@ -199,12 +192,9 @@ def parseScript(test, preamble):
         moduleCompileFlags.extend(["-fmodule-file=std=%T/std.pcm", "%T/std.pcm"])
 
         # Add compile flags required for the modules.
-        substitutions = [
-            (s, x + " " + " ".join(moduleCompileFlags))
-            if s == "%{compile_flags}"
-            else (s, x)
-            for (s, x) in substitutions
-        ]
+        substitutions = config._appendToSubstitution(
+            substitutions, "%{compile_flags}", " ".join(moduleCompileFlags)
+        )
 
     # Perform substitutions in the script itself.
     script = lit.TestRunner.applySubstitutions(



More information about the llvm-branch-commits mailing list