Reland "[clang][modules] Print library module manifest path." #82160

mordante · 2024-02-18T12:01:49Z

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 reverts commit 82f424f.

Disables the tests on non-X86 platforms as suggested.

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++. This reverts commit 82f424f. Disables the tests on non-X86 platforms as suggested.

llvmbot · 2024-02-18T12:02:23Z

@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++.

This reverts commit 82f424f.

Disables the tests on non-X86 platforms as suggested.

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

4 Files Affected:

(modified) clang/include/clang/Driver/Driver.h (+10)
(modified) clang/include/clang/Driver/Options.td (+3)
(modified) clang/lib/Driver/Driver.cpp (+44)
(added) clang/test/Driver/modules-print-library-module-manifest-path.cpp (+38)

diff --git a/clang/include/clang/Driver/Driver.h b/clang/include/clang/Driver/Driver.h
index 908bc87c14b1ca..ac258dc384e688 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -611,6 +611,16 @@ class Driver {
   // FIXME: This should be in CompilationInfo.
   std::string GetProgramPath(StringRef Name, const ToolChain &TC) const;
 
+  /// Lookup the path to the Standard library module 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 GetStdModuleManifestPath(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 53f23f9abb4c96..2963cbc6af5328 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -5371,6 +5371,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_std_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 5a323bf4c0c5f4..82ee18b4588aa5 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -2197,6 +2197,12 @@ bool Driver::HandleImmediateArgs(const Compilation &C) {
     return false;
   }
 
+  if (C.getArgs().hasArg(options::OPT_print_std_module_manifest_path)) {
+    llvm::outs() << GetStdModuleManifestPath(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';
@@ -6183,6 +6189,44 @@ std::string Driver::GetProgramPath(StringRef Name, const ToolChain &TC) const {
   return std::string(Name);
 }
 
+std::string Driver::GetStdModuleManifestPath(const Compilation &C,
+                                             const ToolChain &TC) const {
+  std::string error = "<NOT PRESENT>";
+
+  switch (TC.GetCXXStdlibType(C.getArgs())) {
+  case ToolChain::CST_Libcxx: {
+    std::string lib = GetFilePath("libc++.so", TC);
+
+    // Note when there are multiple flavours of libc++ the module json needs to
+    // look at the command-line arguments for the proper json.
+    // These flavours do not exist at the moment, but there are plans to
+    // provide a variant that is built with sanitizer instrumentation enabled.
+
+    // For example
+    //  StringRef modules = [&] {
+    //    const SanitizerArgs &Sanitize = TC.getSanitizerArgs(C.getArgs());
+    //    if (Sanitize.needsAsanRt())
+    //      return "modules-asan.json";
+    //    return "modules.json";
+    //  }();
+
+    SmallString<128> path(lib.begin(), lib.end());
+    llvm::sys::path::remove_filename(path);
+    llvm::sys::path::append(path, "modules.json");
+    if (TC.getVFS().exists(path))
+      return static_cast<std::string>(path);
+
+    return error;
+  }
+
+  case ToolChain::CST_Libstdcxx:
+    // libstdc++ does not provide Standard library modules yet.
+    return error;
+  }
+
+  return error;
+}
+
 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/modules-print-library-module-manifest-path.cpp b/clang/test/Driver/modules-print-library-module-manifest-path.cpp
new file mode 100644
index 00000000000000..339d8b9a717684
--- /dev/null
+++ b/clang/test/Driver/modules-print-library-module-manifest-path.cpp
@@ -0,0 +1,38 @@
+// Test that -print-library-module-manifest-path finds the correct file.
+
+// REQUIRES: x86-registered-target
+
+// RUN: rm -rf %t && split-file %s %t && cd %t
+// RUN: mkdir -p %t/Inputs/usr/lib/x86_64-linux-gnu
+// RUN: touch %t/Inputs/usr/lib/x86_64-linux-gnu/libc++.so
+
+// RUN: %clang -print-library-module-manifest-path \
+// RUN:     -stdlib=libc++ \
+// RUN:     --sysroot=%t/Inputs \
+// RUN:     --target=x86_64-linux-gnu 2>&1 \
+// RUN:   | FileCheck libcxx-no-module-json.cpp
+
+// RUN: touch %t/Inputs/usr/lib/x86_64-linux-gnu/modules.json
+// RUN: %clang -print-library-module-manifest-path \
+// RUN:     -stdlib=libc++ \
+// RUN:     --sysroot=%t/Inputs \
+// RUN:     --target=x86_64-linux-gnu 2>&1 \
+// RUN:   | FileCheck libcxx.cpp
+
+// RUN: %clang -print-library-module-manifest-path \
+// RUN:     -stdlib=libstdc++ \
+// RUN:     --sysroot=%t/Inputs \
+// RUN:     --target=x86_64-linux-gnu 2>&1 \
+// RUN:   | FileCheck libstdcxx.cpp
+
+//--- libcxx-no-module-json.cpp
+
+// CHECK: <NOT PRESENT>
+
+//--- libcxx.cpp
+
+// CHECK: {{.*}}/Inputs/usr/lib/x86_64-linux-gnu{{/|\\}}modules.json
+
+//--- libstdcxx.cpp
+
+// CHECK: <NOT PRESENT>

ChuanqiXu9

LGTM.

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

ChuanqiXu9 · 2024-02-28T07:58:05Z

@mordante if we want this for 18, we need to land and backport it in this week.

mordante · 2024-03-03T17:00:27Z

@mordante if we want this for 18, we need to land and backport it in this week.

Thanks for the reminder, it got a bit under my radar. I'll ask a backport tomorrow unless a CI starts to complain again.

kaz7 · 2024-03-04T13:20:24Z

Unfortunately, this is breaking VE buildbot again. I'll check the source of your problem, but can you please revert your patch once?

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

…#82160)" This reverts commit 0c89427.

kaz7 · 2024-03-04T16:36:01Z

Reverted. It is possible to reproduce erros by following commands.

$ mkdir build
$ cd build
$ cmake -G Ninja -DCMAKE_BUILD_TYPE=Release ../llvm-project/llvm -DLLVM_ENABLE_PROJECTS=clang -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libunwind"
$ ninja
$ ninja check-clang
********************
********************
Failed Tests (1):
  Clang :: Driver/modules-print-library-module-manifest-path.cpp

kaz7 · 2024-03-04T16:48:14Z

The point is to build libraries and test clang at once. The TC.getFilePaths is constructed from following items in ToolChain::ToolChain

BUILD/bin/../lib/x86_64-unknown-linux-gnu
%t/Inputs/usr/lib/x86_64-linux-gnu
%t/Inputs/usr/lib

Yes. It contains 1st item at run time if you build libraries. As a result, GetStdModuleManifestPath find BUILD/bin/../lib/x86_64-unknown-linux-gnu/libc++.so first. However, modules.json is at %t/Inputs/usr/lib/x86_64-linux-gnu/modules.json.

Hope this helps you.

mordante · 2024-03-04T18:27:34Z

Thanks for the revert and additional information. I was expecting that the test should not be affected by files outside of the %t/Inputs directory. I will test this locally, it might take a few days before I have time.

mordante · 2024-03-11T18:56:50Z

The point is to build libraries and test clang at once. The TC.getFilePaths is constructed from following items in ToolChain::ToolChain
1. `BUILD/bin/../lib/x86_64-unknown-linux-gnu`

2. `%t/Inputs/usr/lib/x86_64-linux-gnu`

3. `%t/Inputs/usr/lib`
Yes. It contains 1st item at run time if you build libraries. As a result, GetStdModuleManifestPath find BUILD/bin/../lib/x86_64-unknown-linux-gnu/libc++.so first. However, modules.json is at %t/Inputs/usr/lib/x86_64-linux-gnu/modules.json.

Hope this helps you.

I can indeed reproduce this. I'm not sure whether this is the expected result. There is no real documentation for -sysroot, but it is intended for cross-compilation. @ChuanqiXu9 since you suggested this test approach, do you know whether this is the expected behavior of -sysroot ?

ChuanqiXu9 · 2024-03-12T01:48:21Z

@ChuanqiXu9 since you suggested this test approach

It looks like that I failed to understand VE is under X86 also...

do you know whether this is the expected behavior of -sysroot ?

I am not sure. This is surprising to me too.

The point is to build libraries and test clang at once. The TC.getFilePaths is constructed from following items in ToolChain::ToolChain

BUILD/bin/../lib/x86_64-unknown-linux-gnu

%t/Inputs/usr/lib/x86_64-linux-gnu

%t/Inputs/usr/lib

Yes. It contains 1st item at run time if you build libraries. As a result, GetStdModuleManifestPath find BUILD/bin/../lib/x86_64-unknown-linux-gnu/libc++.so first. However, modules.json is at %t/Inputs/usr/lib/x86_64-linux-gnu/modules.json.

Hope this helps you.

It looks like this is not special to VE but reproducible for every constructing the clang and libc++ at once. So I'll try to look at it if I can reproduce it locally.

…2160) 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++. This reverts commit 82f424f. Disables the tests on non-X86 platforms as suggested.

ChuanqiXu9 · 2024-03-12T07:29:39Z

I sent the new PR in #84881. See the comments there for details.

@kaz7

Following of #82160 The reason why the above PR fails is that the `--sysroot` has lower priority than the libc++ built from the same source. On the one hand, it matches the codes behavior. We will add the built libc++ project paths in the ToolChain class. But we will only add the path related to sysroot in Linux class, which is derived from the ToolChain classes. So the paths of just built libc++ is in the front of the paths relative to sysroot. On the other hand, the behavior should be good from the higher level. Since the just built libc++ has the same version number with the just built clang, so it makes sense that these 2 compilers just matches. So for patch it self, I hacked it by using resource dir in the test since the resource dir has the higher priority, which is not strongly correct since we won't do that in practice. @kaz7 would you like to test on your environment to avoid this get reverted again? On the libc++ side, it shows that it lacks a `modules.json` file for the just built libc++ directory. If we don't have that, it will be problematic to use std modules from the just built clang and libc++ pair. Then it is not good. And I feel it may be problematic for future compiler/standard library developers. So I feel this is somewhat a libc++ issue that need to be fixed. Also if we don't like the hacked test in the current patch, we must wait for libc++ to fix this to proceed. But I feel this is somewhat odd since the test of clang shouldn't dependent on libc++. CC: @mordante --------- Co-authored-by: Mark de Wever <[email protected]>

@kaz7

Following of llvm#82160 The reason why the above PR fails is that the `--sysroot` has lower priority than the libc++ built from the same source. On the one hand, it matches the codes behavior. We will add the built libc++ project paths in the ToolChain class. But we will only add the path related to sysroot in Linux class, which is derived from the ToolChain classes. So the paths of just built libc++ is in the front of the paths relative to sysroot. On the other hand, the behavior should be good from the higher level. Since the just built libc++ has the same version number with the just built clang, so it makes sense that these 2 compilers just matches. So for patch it self, I hacked it by using resource dir in the test since the resource dir has the higher priority, which is not strongly correct since we won't do that in practice. @kaz7 would you like to test on your environment to avoid this get reverted again? On the libc++ side, it shows that it lacks a `modules.json` file for the just built libc++ directory. If we don't have that, it will be problematic to use std modules from the just built clang and libc++ pair. Then it is not good. And I feel it may be problematic for future compiler/standard library developers. So I feel this is somewhat a libc++ issue that need to be fixed. Also if we don't like the hacked test in the current patch, we must wait for libc++ to fix this to proceed. But I feel this is somewhat odd since the test of clang shouldn't dependent on libc++. CC: @mordante --------- Co-authored-by: Mark de Wever <[email protected]> (cherry picked from commit 0e1e1fc)

@kaz7

Following of llvm#82160 The reason why the above PR fails is that the `--sysroot` has lower priority than the libc++ built from the same source. On the one hand, it matches the codes behavior. We will add the built libc++ project paths in the ToolChain class. But we will only add the path related to sysroot in Linux class, which is derived from the ToolChain classes. So the paths of just built libc++ is in the front of the paths relative to sysroot. On the other hand, the behavior should be good from the higher level. Since the just built libc++ has the same version number with the just built clang, so it makes sense that these 2 compilers just matches. So for patch it self, I hacked it by using resource dir in the test since the resource dir has the higher priority, which is not strongly correct since we won't do that in practice. @kaz7 would you like to test on your environment to avoid this get reverted again? On the libc++ side, it shows that it lacks a `modules.json` file for the just built libc++ directory. If we don't have that, it will be problematic to use std modules from the just built clang and libc++ pair. Then it is not good. And I feel it may be problematic for future compiler/standard library developers. So I feel this is somewhat a libc++ issue that need to be fixed. Also if we don't like the hacked test in the current patch, we must wait for libc++ to fix this to proceed. But I feel this is somewhat odd since the test of clang shouldn't dependent on libc++. CC: @mordante --------- Co-authored-by: Mark de Wever <[email protected]> (cherry picked from commit 0e1e1fc)

mordante requested review from AaronBallman, kaz7 and ChuanqiXu9 February 18, 2024 12:01

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

mordante mentioned this pull request Feb 18, 2024

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

Merged

ChuanqiXu9 approved these changes Feb 19, 2024

View reviewed changes

clang/test/Driver/modules-print-library-module-manifest-path.cpp Show resolved Hide resolved

Address review comments.

edd25d5

mordante merged commit 0c89427 into llvm:main Mar 3, 2024

mordante deleted the print-library-module-manifest-path branch March 3, 2024 16:59

kaz7 added a commit that referenced this pull request Mar 4, 2024

Revert "Reland "[clang][modules] Print library module manifest path." (…

c240aca

…#82160)" This reverts commit 0c89427.

Arthapz mentioned this pull request Mar 6, 2024

Implement stdmodule clang detection flag xmake-io/xmake#4805

Merged

ChuanqiXu9 mentioned this pull request Mar 12, 2024

Reland Print library module manifest path again #84881

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reland "[clang][modules] Print library module manifest path." #82160

Reland "[clang][modules] Print library module manifest path." #82160

Uh oh!

mordante commented Feb 18, 2024

Uh oh!

llvmbot commented Feb 18, 2024 •

edited

Loading

Uh oh!

ChuanqiXu9 left a comment

Uh oh!

Uh oh!

ChuanqiXu9 commented Feb 28, 2024

Uh oh!

mordante commented Mar 3, 2024

Uh oh!

kaz7 commented Mar 4, 2024 •

edited

Loading

Uh oh!

kaz7 commented Mar 4, 2024

Uh oh!

kaz7 commented Mar 4, 2024 •

edited

Loading

Uh oh!

mordante commented Mar 4, 2024

Uh oh!

mordante commented Mar 11, 2024

Uh oh!

ChuanqiXu9 commented Mar 12, 2024

Uh oh!

ChuanqiXu9 commented Mar 12, 2024

Uh oh!

Uh oh!

Reland "[clang][modules] Print library module manifest path." #82160

Reland "[clang][modules] Print library module manifest path." #82160

Uh oh!

Conversation

mordante commented Feb 18, 2024

Uh oh!

llvmbot commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChuanqiXu9 commented Feb 28, 2024

Uh oh!

mordante commented Mar 3, 2024

Uh oh!

kaz7 commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaz7 commented Mar 4, 2024

Uh oh!

kaz7 commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mordante commented Mar 4, 2024

Uh oh!

mordante commented Mar 11, 2024

Uh oh!

ChuanqiXu9 commented Mar 12, 2024

Uh oh!

ChuanqiXu9 commented Mar 12, 2024

Uh oh!

Uh oh!

llvmbot commented Feb 18, 2024 •

edited

Loading

kaz7 commented Mar 4, 2024 •

edited

Loading

kaz7 commented Mar 4, 2024 •

edited

Loading