[clang][modules] Print library module manifest path. #76451

mordante · 2023-12-27T16:39:24Z

This implements a way for the compiler to find the modules.json associated with the C++23 Standard library modules.

This is based on a discussion in SG15. At the moment no Standard library installs this manifest. #75741 adds this feature in libc++.

This implements a way for the compiler to find the modules.json associated with the C++23 Standard library modules. This is based on a discussion in SG15. At the moment no Standard library installs this manifest. llvm#75741 adds this feature in libc++.

llvmbot · 2023-12-27T16:39:55Z

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Mark de Wever (mordante)

Changes

This implements a way for the compiler to find the modules.json associated with the C++23 Standard library modules.

This is based on a discussion in SG15. At the moment no Standard library installs this manifest. #75741 adds this feature in libc++.

Full diff: https://github.com/llvm/llvm-project/pull/76451.diff

7 Files Affected:

(modified) clang/include/clang/Driver/Driver.h (+10)
(modified) clang/include/clang/Driver/Options.td (+3)
(modified) clang/lib/Driver/Driver.cpp (+40)
(added) clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/libc++.so ()
(added) clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/modules.json ()
(added) clang/test/Driver/cxx23-modules-print-library-module-manifest-path.c (+15)
(added) clang/test/Driver/darwin-print-library-module-manifest-path.c (+9)

diff --git a/clang/include/clang/Driver/Driver.h b/clang/include/clang/Driver/Driver.h
index 3ee1bcf2a69c9b..2e1e3b128744ff 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -602,6 +602,16 @@ class Driver {
   // FIXME: This should be in CompilationInfo.
   std::string GetProgramPath(StringRef Name, const ToolChain &TC) const;
 
+  /// GetModuleManifestPath - Lookup the name of the Standard library manifest.
+  ///
+  /// \param C - The compilation.
+  /// \param TC - The tool chain for additional information on
+  /// directories to search.
+  //
+  // FIXME: This should be in CompilationInfo.
+  std::string GetModuleManifestPath(const Compilation &C,
+                                    const ToolChain &TC) const;
+
   /// HandleAutocompletions - Handle --autocomplete by searching and printing
   /// possible flags, descriptions, and its arguments.
   void HandleAutocompletions(StringRef PassedFlags) const;
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 2b93ddf033499c..890257e11485b6 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -5280,6 +5280,9 @@ def print_resource_dir : Flag<["-", "--"], "print-resource-dir">,
 def print_search_dirs : Flag<["-", "--"], "print-search-dirs">,
   HelpText<"Print the paths used for finding libraries and programs">,
   Visibility<[ClangOption, CLOption]>;
+def print_library_module_manifest_path : Flag<["-", "--"], "print-library-module-manifest-path">,
+  HelpText<"Print the path for the C++ Standard library module manifest">,
+  Visibility<[ClangOption, CLOption]>;
 def print_targets : Flag<["-", "--"], "print-targets">,
   HelpText<"Print the registered targets">,
   Visibility<[ClangOption, CLOption]>;
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index ff95c899c5f3d4..8d682f9238c6b8 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -2164,6 +2164,12 @@ bool Driver::HandleImmediateArgs(const Compilation &C) {
     return false;
   }
 
+  if (C.getArgs().hasArg(options::OPT_print_library_module_manifest_path)) {
+    llvm::outs() << "module: ="
+                 << GetModuleManifestPath(C, C.getDefaultToolChain()) << '\n';
+    return false;
+  }
+
   if (C.getArgs().hasArg(options::OPT_print_runtime_dir)) {
     if (std::optional<std::string> RuntimePath = TC.getRuntimePath())
       llvm::outs() << *RuntimePath << '\n';
@@ -6135,6 +6141,40 @@ std::string Driver::GetProgramPath(StringRef Name, const ToolChain &TC) const {
   return std::string(Name);
 }
 
+std::string Driver::GetModuleManifestPath(const Compilation &C,
+                                          const ToolChain &TC) const {
+
+  switch (TC.GetCXXStdlibType(C.getArgs())) {
+  case ToolChain::CST_Libcxx: {
+    std::string lib = "libc++.so";
+    std::string path = GetFilePath(lib, TC);
+
+    // Note when there are multiple flavours of libc++ the module json needs to
+    // look at the command-line arguments for the proper json.
+
+    // For example
+    /*
+        const SanitizerArgs &Sanitize = TC.getSanitizerArgs(C.getArgs());
+        if (Sanitize.needsAsanRt())
+          return path.replace(path.size() - lib.size(), lib.size(),
+                              "modules-asan.json");
+    */
+
+    path = path.replace(path.size() - lib.size(), lib.size(), "modules.json");
+    if (TC.getVFS().exists(path))
+      return path;
+
+    return "";
+  }
+
+  case ToolChain::CST_Libstdcxx:
+    // libstdc++ does not provide Standard library modules yet.
+    return "";
+  }
+
+  return "";
+}
+
 std::string Driver::GetTemporaryPath(StringRef Prefix, StringRef Suffix) const {
   SmallString<128> Path;
   std::error_code EC = llvm::sys::fs::createTemporaryFile(Prefix, Suffix, Path);
diff --git a/clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/libc++.so b/clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/libc++.so
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/modules.json b/clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/modules.json
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/cxx23-modules-print-library-module-manifest-path.c b/clang/test/Driver/cxx23-modules-print-library-module-manifest-path.c
new file mode 100644
index 00000000000000..7ad2b10081bc5b
--- /dev/null
+++ b/clang/test/Driver/cxx23-modules-print-library-module-manifest-path.c
@@ -0,0 +1,15 @@
+// Test that -print-library-module-manifest-path finds the correct file.
+
+// RUN: %clang -print-library-module-manifest-path \
+// RUN:     -stdlib=libc++ \
+// RUN:     --sysroot=%S/Inputs/cxx23_modules \
+// RUN:     --target=x86_64-linux-gnu 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX %s
+// CHECK-LIBCXX: module: ={{.*}}/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/modules.json
+
+// RUN: %clang -print-library-module-manifest-path \
+// RUN:     -stdlib=libstdc++ \
+// RUN:     --sysroot=%S/Inputs/cxx23_modules \
+// RUN:     --target=x86_64-linux-gnu 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBSTDCXX %s
+// CHECK-LIBSTDCXX: module: =
diff --git a/clang/test/Driver/darwin-print-library-module-manifest-path.c b/clang/test/Driver/darwin-print-library-module-manifest-path.c
new file mode 100644
index 00000000000000..39d28589b146ae
--- /dev/null
+++ b/clang/test/Driver/darwin-print-library-module-manifest-path.c
@@ -0,0 +1,9 @@
+// Test that -print-library-module-manifest-path finds the correct file.
+//
+// Note this file is currently not available on Apple platforms
+
+// RUN: %clang -print-library-module-manifest-path \
+// RUN:     -resource-dir=%S/Inputs/resource_dir \
+// RUN:     --target=x86_64-unknown-linux-gnu 2>&1 \
+// RUN:   | FileCheck %s
+// CHECK: module: =

clang/include/clang/Driver/Driver.h

clang/test/Driver/cxx23-modules-print-library-module-manifest-path.c

clang/lib/Driver/Driver.cpp

clang/include/clang/Driver/Driver.h

clang/include/clang/Driver/Options.td

clang/lib/Driver/Driver.cpp

ChuanqiXu9 · 2023-12-28T02:16:07Z

clang/test/Driver/cxx23-modules-print-library-module-manifest-path.c

+// RUN:     --target=x86_64-linux-gnu 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX %s
+// CHECK-LIBCXX: module: ={{.*}}/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/modules.json
+


Is there a path for libc++ like --gcc-toolchain= so that we can choose the different locations for libstdc++?

not sure, I've never used --gcc-toolchain either.

clang/include/clang/Driver/Driver.h

clang/lib/Driver/Driver.cpp

clang/test/Driver/cxx23-modules-print-library-module-manifest-path.c

clang/test/Driver/darwin-print-library-module-manifest-path.c

boris-kolpackov · 2023-12-29T21:41:21Z

If I understand correctly, this invents a new option just to print the std modules path. Is there a reason why we cannot just print this information as part of -print-search-dirs? The benefit of this will be saving an extra compiler invocation in case the build system already uses this option, for example, to extract the library search paths (build2 does this). BTW, this would also allow you to sidestep the whole "what to print if there is no path" issue -- just omit the entry from the output.

If for some reason a separate option is desirable, can we then also print this information as part of -print-search-dirs?

mordante · 2023-12-30T10:14:42Z

I had looked at -print-search-dirs but it seems buggy. For example, it ignores the -stdlib flag to the compiler invocation. Solving this seems a lot of work. So didn't want to add new features to that code.

boris-kolpackov · 2023-12-30T12:06:18Z

I had looked at -print-search-dirs but it seems buggy. For example, it ignores the -stdlib flag to the compiler invocation.

True. I couldn't find a bug report for this so I filed one: #76614

Solving this seems a lot of work. So didn't want to add new features to that code.

If this is the right thing to do (i.e., printing this information as part of -print-search-dirs), then perhaps it's still worth it to try and fix? Once you add a separate option, you will have to drag it forever (for backwards compatibility).

mordante · 2024-01-03T17:40:59Z

If this is the right thing to do (i.e., printing this information as part of -print-search-dirs), then perhaps it's still worth it to try and fix? Once you add a separate option, you will have to drag it forever (for backwards compatibility).
I have a feeling how deep that rabbit hole is. This is not something I really want to dive into. Especially since I'm unfamiliar with this part of the code.

mordante · 2024-01-11T19:24:21Z

@ChuanqiXu9 can you do a full review? I'd like to have this reviewed so we can land it when the libc++ side is ready.

ChuanqiXu9

LGTM generally.

clang/lib/Driver/Driver.cpp

ChuanqiXu9 · 2024-01-12T01:42:23Z

clang/test/Driver/cxx23-modules-print-library-module-manifest-path.cpp

+// RUN:     --sysroot=%S/Inputs/cxx23_modules \
+// RUN:     --target=x86_64-linux-gnu 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-LIBCXX %s
+// CHECK-LIBCXX: {{.*}}/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/modules.json


Let's try to avoid use Inputs pattern nowadays. We can use split-file to multiple files into a single lit test. You can find the use example by searching split-file in clang/test/Modules

clang/test/Driver/darwin-print-library-module-manifest-path.cpp

github-actions · 2024-01-12T16:45:35Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ChuanqiXu9

LGTM.

We need to delete clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/libc++.so and clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/modules.json, we should generate them with split-file

mordante · 2024-01-17T07:04:01Z

LGTM.

We need to delete clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/libc++.so and clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/modules.json, we should generate them with split-file

Are you sure that is the proper way? Should these tests not use a complete installation to make sure it works on installations?

ChuanqiXu9 · 2024-01-17T08:01:11Z

LGTM.
We need to delete clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/libc++.so and clang/test/Driver/Inputs/cxx23_modules/usr/lib/x86_64-linux-gnu/modules.json, we should generate them with split-file

Are you sure that is the proper way? Should these tests not use a complete installation to make sure it works on installations?

We can use mkdir to generate more complex paths. For example,

// RUN: split-file %s %t
//
// RUN: mkdir %t/....
// RUN: touch %t/.../libc++.so

The intention is to remove the use of Inputs in test. It is really hard to read.

MaskRay · 2024-01-18T18:55:30Z

clang/test/Driver/cxx23-modules-print-library-module-manifest-path.cpp

+
+// RUN: %clang -print-library-module-manifest-path \
+// RUN:     -stdlib=libstdc++ \
+// RUN:     --sysroot=%S/Inputs/cxx23_modules \


Does %S/Inputs/cxx23_modules exist?

No this is a left-over when it used on-disc files. Nice catch.

clang/include/clang/Driver/Driver.h

clang/lib/Driver/Driver.cpp

clang/test/Driver/cxx23-modules-print-library-module-manifest-path.cpp

@ruoso

Installs the source files of the experimental libc++ modules. These source files (.cppm) are used by the Clang to build the std and std.compat modules. The design of this patch is based on a discussing in SG-15 on 12.12.2023. (SG-15 is the ISO C++ Tooling study group): - The modules are installed at a location, that is not known to build systems and compilers. - Next to the library there will be a module manifest json file. This json file contains the information to build the module from the libraries sources. This information includes the location where the sources are installed. @ruoso supplied the specification of this json file. - If possible, the compiler has an option to give the location of the module manifest file (#76451). Currently there is no build system support, but it expected to be added in the future. Fixes: #73089

mordante · 2024-01-21T20:33:57Z

@MaskRay can you have another look, it would be great to land this before LLVM 18 branches, thanks!

MaskRay · 2024-01-22T06:23:47Z

clang/lib/Driver/Driver.cpp

+    //      return "modules-asan.json";
+    //    return "modules.json";
+    //  }();
+    StringRef modules = "modules.json";


modules is only used once. inline it

MaskRay

LGTM with a few nits. Can you double check modules.json is inside lib/ (usr/lib/x86_64-linux-gnu{{/|\\}}modules.json) instead of some directory under include?

mordante · 2024-01-22T18:42:24Z

LGTM with a few nits. Can you double check modules.json is inside lib/ (usr/lib/x86_64-linux-gnu{{/|\\}}modules.json) instead of some directory under include?

Thanks for the review. The json will indeed be installed in the lib dir in libc++.

kaz7 · 2024-01-23T04:09:28Z

Hi @mordante,

Does this test, Driver/modules-print-library-module-manifest-path.cpp, work on other than x86? It is failing on VE build bot like below.

https://lab.llvm.org/buildbot/#/builders/91/builds/22189

This reverts commit a301fb1. The changes caused failures like: https://lab.llvm.org/buildbot/#/builders/91/builds/22189

AaronBallman · 2024-01-23T12:07:09Z

The build is still failing, so I've reverted these changes in 82f424f

mordante · 2024-02-04T14:20:36Z

Thanks for reverting @AaronBallman I somehow missed this comment.

@kaz7 it should work on all platforms AFAIK. I don't have access to a VE system. Looking at some VE tests I wonder whether the following patch would work.
patch.txt

@ruoso

Installs the source files of the experimental libc++ modules. These source files (.cppm) are used by the Clang to build the std and std.compat modules. The design of this patch is based on a discussing in SG-15 on 12.12.2023. (SG-15 is the ISO C++ Tooling study group): - The modules are installed at a location, that is not known to build systems and compilers. - Next to the library there will be a module manifest json file. This json file contains the information to build the module from the libraries sources. This information includes the location where the sources are installed. @ruoso supplied the specification of this json file. - If possible, the compiler has an option to give the location of the module manifest file (llvm/llvm-project#76451). Currently there is no build system support, but it expected to be added in the future. Fixes: llvm/llvm-project#73089 NOKEYCHECK=True GitOrigin-RevId: 8b47bb657b5905d954b9041415020358802407d5

mordante · 2024-02-10T17:08:55Z

@kaz7 ping

mordante · 2024-02-17T16:21:05Z

@kaz7 ping, can you have a look at the patch, I really like to reland this patch.

ChuanqiXu9 · 2024-02-18T05:42:08Z

(This is another example that the github review can't work well with the reverted patches...)

@mordante I think you can add // REQUIRES: x86-registered-target to the test if @kaz7 can't respond quickly. It will skip the test on targets other than x86. And it should keep the CI green and enable us to backport this clang18.

We can still improve this later.

mordante · 2024-02-18T12:02:27Z

(This is another example that the github review can't work well with the reverted patches...)

@mordante I think you can add // REQUIRES: x86-registered-target to the test if @kaz7 can't respond quickly. It will skip the test on targets other than x86. And it should keep the CI green and enable us to backport this clang18.

We can still improve this later.

I've created #82160 as you suggested.

mordante requested review from Bigcheese, tahonermann and ChuanqiXu9 December 27, 2023 16:39

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Dec 27, 2023

mordante mentioned this pull request Dec 27, 2023

[libc++] Install modules. #75741

Merged