Support C++20 Modules in clang-repl #79261

ChuanqiXu9 · 2024-01-24T06:24:57Z

This comes from when I playing around clang-repl with moduels : )

I succeeded to import std with https://libcxx.llvm.org/Modules.html and calling std::printf after this patch.

I want to put the documentation part to https://clang.llvm.org/docs/StandardCPlusPlusModules.html in a separate commit.

llvmbot · 2024-01-24T06:25:30Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Changes

This comes from when I playing around clang-repl with moduels : )

I succeeded to import std with https://libcxx.llvm.org/Modules.html and calling std::printf after this patch.

I want to put the documentation part to https://clang.llvm.org/docs/StandardCPlusPlusModules.html in a separate commit.

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

4 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+2)
(modified) clang/include/clang/Basic/LangOptions.def (+1-1)
(modified) clang/lib/Serialization/ASTReaderDecl.cpp (+3-3)
(added) clang/test/Interpreter/cxx20-modules.cppm (+31)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fc7511e9136734..db3d74e124e7d1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -191,6 +191,8 @@ Crash and bug fixes
 Improvements
 ^^^^^^^^^^^^
 
+- Support importing C++20 modules in clang-repl.
+
 Moved checkers
 ^^^^^^^^^^^^^^
 
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 8fc75e1cca0399..1e671a7c460163 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -485,7 +485,7 @@ VALUE_LANGOPT(FuchsiaAPILevel, 32, 0, "Fuchsia API level")
 // on large _BitInts.
 BENIGN_VALUE_LANGOPT(MaxBitIntWidth, 32, 128, "Maximum width of a _BitInt")
 
-LANGOPT(IncrementalExtensions, 1, 0, " True if we want to process statements"
+COMPATIBLE_LANGOPT(IncrementalExtensions, 1, 0, " True if we want to process statements"
         "on the global scope, ignore EOF token and continue later on (thus "
         "avoid tearing the Lexer and etc. down). Controlled by "
         "-fincremental-extensions.")
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index a149d82153037f..867f4c47eaeceb 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -3286,10 +3286,10 @@ DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader,
   if (auto *OID = dyn_cast<ObjCInterfaceDecl>(DC))
     return OID->getDefinition();
 
-  // We can see the TU here only if we have no Sema object. In that case,
-  // there's no TU scope to look in, so using the DC alone is sufficient.
+  // We can see the TU here only if we have no Sema object. It is possible
+  // we're in clang-repl so we still need to get the primary context.
   if (auto *TU = dyn_cast<TranslationUnitDecl>(DC))
-    return TU;
+    return TU->getPrimaryContext();
 
   return nullptr;
 }
diff --git a/clang/test/Interpreter/cxx20-modules.cppm b/clang/test/Interpreter/cxx20-modules.cppm
new file mode 100644
index 00000000000000..bc2b722f6b5197
--- /dev/null
+++ b/clang/test/Interpreter/cxx20-modules.cppm
@@ -0,0 +1,31 @@
+// UNSUPPORTED: system-aix
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang -std=c++20 %t/mod.cppm --precompile \
+// RUN:     -o %t/mod.pcm
+// RUN: %clang %t/mod.pcm -c -o %t/mod.o
+// RUN: %clang -shared %t/mod.o -o %t/libmod.so
+//
+// RUN: cat %t/import.cpp | env LD_LIBRARY_PATH=%t:$LD_LIBRARY_PATH \
+// RUN:     clang-repl -Xcc=-std=c++20 -Xcc=-fmodule-file=M=%t/mod.pcm \
+// RUN:     | FileCheck %t/import.cpp
+
+//--- mod.cppm
+export module M;
+export const char* Hello() {
+    return "Hello Interpreter for Modules!";
+}
+
+//--- import.cpp
+
+%lib libmod.so
+
+import M;
+
+extern "C" int printf(const char *, ...);
+printf("%s\n", Hello());
+
+// CHECK: Hello Interpreter for Modules!

vgvassilev

Lgtm! Thank you @ChuanqiXu9!

dyung · 2024-01-24T07:59:25Z

Hi @ChuanqiXu9, the test you added is failing on the PS4 bot because for the PS4 target, it requires an external linker which is not present/available. Can the test be rewritten to not require linking?

https://lab.llvm.org/buildbot/#/builders/139/builds/57928

clang: error: unable to execute command: Executable "orbis-ld" doesn't exist!
clang: error: linker command failed with exit code 1 (use -v to see invocation)

ChuanqiXu9 · 2024-01-24T08:09:38Z

Hi @ChuanqiXu9, the test you added is failing on the PS4 bot because for the PS4 target, it requires an external linker which is not present/available. Can the test be rewritten to not require linking?

https://lab.llvm.org/buildbot/#/builders/139/builds/57928
clang: error: unable to execute command: Executable "orbis-ld" doesn't exist!
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Oh, maybe I need to add more requires to the test.

I tried to handle it in c125965. It shouldn't fail on PS4 any more.

The previous test can't work on other platforms like PS4. Address the comments in #79261 (comment)

nathanchance · 2024-01-25T01:21:17Z

For what it's worth, this test appears to fail when LLVM_DEFAULT_TARGET_TRIPLE is changed (my script sets it to the output of my Linux distribution's clang -print-target-triple)

error: PCH file was compiled for the target 'x86_64-pc-linux-gnu' but the current translation unit is being compiled for target 'x86_64-unknown-linux-gnu'
error: module file .../tools/clang/test/Interpreter/Output/cxx20-modules.cppm.tmp/mod.pcm cannot be loaded due to a configuration mismatch with the current compilation [-Wmodule-file-config-mismatch]
error: Parsing failed.

ChuanqiXu9 · 2024-01-25T06:10:08Z

For what it's worth, this test appears to fail when LLVM_DEFAULT_TARGET_TRIPLE is changed (my script sets it to the output of my Linux distribution's clang -print-target-triple)
error: PCH file was compiled for the target 'x86_64-pc-linux-gnu' but the current translation unit is being compiled for target 'x86_64-unknown-linux-gnu'
error: module file .../tools/clang/test/Interpreter/Output/cxx20-modules.cppm.tmp/mod.pcm cannot be loaded due to a configuration mismatch with the current compilation [-Wmodule-file-config-mismatch]
error: Parsing failed.

Thanks. But I am not sure how should I change the test. Do you have any idea or suggestion?

vgvassilev · 2024-01-25T06:41:14Z

Perhaps we are not propagating the correct triple down to the module build action?

ChuanqiXu9 · 2024-01-25T06:47:22Z

It looks true but the test does the same thing as other tests to build modules

nathanchance · 2024-01-26T00:14:59Z

This is out of my wheelhouse to debug to be honest but it seems to me that clang-repl does not respect LLVM_DEFAULT_TARGET_TRIPLE somehow? This test does the same thing as other tests to build modules but this appears to be the only one that uses clang-repl to consume the .pcm.

vgvassilev · 2024-01-26T07:24:55Z

We pass the CompilerInstance's notion of the target triple this commit explains it: 49f9532

@nathanchance, can clang-repl be run on your setup? If the Jit is happy about the target triple and we can jit code that'd probably mean that the modules infrastructure gets confused about the target triple and build a C++ module for the wrong configuration.

nathanchance · 2024-01-26T16:19:45Z

@vgvassilev Yes, it appears so, I tried one of the examples from the documentation since I have no prior experience with clang-repl.

$ clang-repl --version
LLVM (http://llvm.org/):
  LLVM version 19.0.0git
  Optimized build with assertions.

$ clang-repl
clang-repl> #include <iostream>
clang-repl> int sum(int a, int b){ return a+b; };
clang-repl> int c = sum(9,10);
clang-repl> std::cout << c << std::endl;
19
clang-repl> ^D

nikic · 2024-01-29T13:50:33Z

Normal clang creates a driver using the default target triple here:

llvm-project/clang/tools/driver/driver.cpp

Line 485 in 754a8ad

Driver TheDriver(Path, llvm::sys::getDefaultTargetTriple(), Diags);

clang-repl appears to use IncrementalCompilerBuilder, which uses getProcessTriple() here:

llvm-project/clang/lib/Interpreter/Interpreter.cpp

Lines 165 to 166 in 754a8ad

    
           driver::Driver Driver(/*MainBinaryName=*/ClangArgv[0], 
        
                                 llvm::sys::getProcessTriple(), Diags);

getProcessTriple() is based on LLVM_HOST_TRIPLE with some fixups.

I'm not really sure what the right fix here is. It makes sense that an interpreter uses the host triple by default, but it's also a problem that clang and clang-repl use different default triples.

vgvassilev · 2024-01-29T13:57:14Z

@nikic, thanks for the details!

Until this change we sailed fairly well. It seems that the pcm rigorously records the clang notion of the triple which is probably what we want. However, if we are building a pcm in the context of clang-repl would it make sense to use the getProcessTriple notion for that pcm -- that'd be the behavior if we used header files instead of a module.

Looks like it doesn't work when the default host triple is changed: llvm/llvm-project#79261

nikic · 2024-01-30T08:58:38Z

Until this change we sailed fairly well. It seems that the pcm rigorously records the clang notion of the triple which is probably what we want. However, if we are building a pcm in the context of clang-repl would it make sense to use the getProcessTriple notion for that pcm -- that'd be the behavior if we used header files instead of a module.

The pcm here is being generated by clang, not by clang-repl, so I'm not sure how we would know that we need to switch the triple.

vgvassilev · 2024-01-30T09:00:23Z

We have a LangOption called IncrementalExtensions. We might be able to use it there.

ChuanqiXu9 · 2024-01-30T09:12:02Z

IncrementalExtensions

But the change itself is to make IncrementalExtensions a compatible lang options. So that we don't need to specify it when generating the modules.

ChuanqiXu9 · 2024-01-30T09:13:28Z

I am wondering if it helps if we specify the target triple in the test, then it won't be so troublesome.

vgvassilev · 2024-01-30T09:22:30Z

IncrementalExtensions

But the change itself is to make IncrementalExtensions a compatible lang options. So that we don't need to specify it when generating the modules.

I mean conditionally if IncrementalExtensions is set we change the target triple in the clang fork of the C++ module build.

I am wondering if it helps if we specify the target triple in the test, then it won't be so troublesome.

I was wondering that too. Can we express that in lit?

ChuanqiXu9 · 2024-01-30T09:32:16Z

IncrementalExtensions

But the change itself is to make IncrementalExtensions a compatible lang options. So that we don't need to specify it when generating the modules.

I mean conditionally if IncrementalExtensions is set we change the target triple in the clang fork of the C++ module build.

I am not sure if it is possible since we don't know the clang fork before we see the import. Even if we can, it is more or less problematic if there are multiple imported module from different fork.

I am wondering if it helps if we specify the target triple in the test, then it won't be so troublesome.

I was wondering that too. Can we express that in lit?

For %clang_cc1, we can use %clang_cc1 -triple=x86_64-linux-gnu. I am not sure about clang-repl. It may be possible by -Xcc=.

But due to I can't reproduce the failure, @nathanchance would you like to make this? I don't want to make something that I can't test.

vgvassilev · 2024-01-30T12:26:33Z

IncrementalExtensions

But the change itself is to make IncrementalExtensions a compatible lang options. So that we don't need to specify it when generating the modules.

I mean conditionally if IncrementalExtensions is set we change the target triple in the clang fork of the C++ module build.

I am not sure if it is possible since we don't know the clang fork before we see the import. Even if we can, it is more or less problematic if there are multiple imported module from different fork.

I can find arguments in changing the clang-repl target triple to match the loaded module… That would have some reasoning but maybe we won’t be able to start the jit if the triples are too different.

nathanchance · 2024-01-30T15:21:14Z

But due to I can't reproduce the failure, @nathanchance would you like to make this? I don't want to make something that I can't test.

I don't mind submitting something if I have some guidance around how this should be handled (I am a little lost in this thread at the moment). Alternatively, I am able to reproduce this with:

(/usr/bin/clang -print-target-triple is x86_64-pc-linux-gnu, if you'd rather use it directly)

$ cmake \
-B build \
-G Ninja \
-S llvm \
-Wno-dev \
--log-level=NOTICE \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_C_COMPILER=clang \
-DCMAKE_CXX_COMPILER=clang++ \
-DLLVM_DEFAULT_TARGET_TRIPLE=$(/usr/bin/clang -print-target-triple) \
-DLLVM_ENABLE_PROJECTS=clang \
-DLLVM_USE_LINKER=lld

$ ninja -C build check-clang
...
error: PCH file was compiled for the target 'x86_64-pc-linux-gnu' but the current translation unit is being compiled for target 'x86_64-unknown-linux-gnu'
error: module file .../tools/clang/test/Interpreter/Output/cxx20-modules.cppm.tmp/mod.pcm cannot be loaded due to a configuration mismatch with the current compilation [-Wmodule-file-config-mismatch]
error: Parsing failed.
...

so it should be easy to test and verify the fix.

ChuanqiXu9

This is what I had in mind

ChuanqiXu9 · 2024-01-31T01:57:39Z

clang/test/Interpreter/cxx20-modules.cppm

+// RUN: %clang -std=c++20 %t/mod.cppm --precompile \
+// RUN:     -o %t/mod.pcm
+// RUN: %clang %t/mod.pcm -c -o %t/mod.o
+// RUN: %clang -shared %t/mod.o -o %t/libmod.so
+//
+// RUN: cat %t/import.cpp | env LD_LIBRARY_PATH=%t:$LD_LIBRARY_PATH \
+// RUN:     clang-repl -Xcc=-std=c++20 -Xcc=-fmodule-file=M=%t/mod.pcm \
+// RUN:     | FileCheck %t/import.cpp


// RUN: %clang -std=c++20 %t/mod.cppm --precompile
// RUN: -o %t/mod.pcm -triple=x86_64-linux-gnu
// RUN: %clang %t/mod.pcm -c -o %t/mod.o -triple=x86_64-linux-gnu
// RUN: %clang -shared %t/mod.o -o %t/libmod.so -triple=x86_64-linux-gnu
//
// RUN: cat %t/import.cpp | env LD_LIBRARY_PATH=%t:$LD_LIBRARY_PATH
// RUN: clang-repl -Xcc=-std=c++20 -Xcc=-fmodule-file=M=%t/mod.pcm
// RUN: -Xcc=-triple=x86_64-linux-gnu | FileCheck %t/import.cpp

@nathanchance this is what I expected to test

diff --git a/clang/test/Interpreter/cxx20-modules.cppm b/clang/test/Interpreter/cxx20-modules.cppm index 2c6eba525519..cd2b04fdc547 100644 --- a/clang/test/Interpreter/cxx20-modules.cppm +++ b/clang/test/Interpreter/cxx20-modules.cppm @@ -6,13 +6,13 @@ // RUN: split-file %s %t // // RUN: %clang -std=c++20 %t/mod.cppm --precompile \ -// RUN: -o %t/mod.pcm -// RUN: %clang %t/mod.pcm -c -o %t/mod.o -// RUN: %clang -shared %t/mod.o -o %t/libmod.so +// RUN: -o %t/mod.pcm --target=x86_64-linux-gnu +// RUN: %clang %t/mod.pcm -c -o %t/mod.o --target=x86_64-linux-gnu +// RUN: %clang -shared %t/mod.o -o %t/libmod.so --target=x86_64-linux-gnu // // RUN: cat %t/import.cpp | env LD_LIBRARY_PATH=%t:$LD_LIBRARY_PATH \ // RUN: clang-repl -Xcc=-std=c++20 -Xcc=-fmodule-file=M=%t/mod.pcm \ -// RUN: | FileCheck %t/import.cpp +// RUN: -Xcc=--target=x86_64-linux-gnu | FileCheck %t/import.cpp //--- mod.cppm export module M;

appears to work for me.

Good, would you like to land it? Or I will do it.

You can land it, thanks for continuing to chase this!

See #79261 for details. It shows that clang-repl uses a different target triple with clang so that it may be problematic if the calng-repl reads the generated BMI from clang in a different target triple. While the underlying issue is not easy to fix, this patch tries to make this test green to not bother developers.

steakhal · 2024-07-02T11:47:09Z

@vgvassilev It appears that this PR added an entry to the Static Analyzer section, and I'm not sure if that's the right one.
Could you please suggest an alternative section where I should move it?

vgvassilev · 2024-07-02T12:44:55Z

Do you mean the documentation? If so, yes, that’s probably not the right place. I am on my phone but can you suggest a place where we should move this or just move it? I think that was an oversight.

steakhal · 2024-07-02T12:51:52Z

Do you mean the documentation? If so, yes, that’s probably not the right place. I am on my phone but can you suggest a place where we should move this or just move it? I think that was an oversight.

Thanks. There is nothing urgent. I was just preparing a PR for syncing the release notes with what we actually did in CSA, so I realized this.

I wanted to check first if you already have a well established place/section where you would prefer to have this, as I couldn't find one in 3 seconds.
You can move it, or alternatively I can also move it.
It's up to you. :)

(The release branch is supposed to branch off on the 23rd of July)

vgvassilev · 2024-07-02T12:58:09Z

Oh, understood. Perhaps would be better if you move it. I am currently on vacation for a while…

Let's just move these under the `Non-comprehensive list of changes in this release` section. Resolves: - #79261 (comment) - #65484 (comment)

Let's just move these under the `Non-comprehensive list of changes in this release` section. Resolves: - llvm#79261 (comment) - llvm#65484 (comment)

Support C++20 Modules in clang-repl

70cb9ac

ChuanqiXu9 requested review from steakhal, vgvassilev and tbaederr January 24, 2024 06:24

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Jan 24, 2024

vgvassilev approved these changes Jan 24, 2024

View reviewed changes

ChuanqiXu9 merged commit dd3e6c8 into llvm:main Jan 24, 2024

ChuanqiXu9 deleted the ModulesInClangRepl branch January 24, 2024 07:45

ChuanqiXu9 added a commit that referenced this pull request Jan 24, 2024

[NFC] Add more requirement to clang/test/Interpreter/cxx20-modules.cppm

c125965

The previous test can't work on other platforms like PS4. Address the comments in #79261 (comment)

nikic mentioned this pull request Jan 29, 2024

Broken snapshot for 20240126 detected (standalone) fedora-llvm-team/llvm-snapshots#220

Closed

mysterymath added a commit to llvm-mos/llvm-mos that referenced this pull request Jan 29, 2024

Disable failing clang-repl test

30bb4c5

Looks like it doesn't work when the default host triple is changed: llvm/llvm-project#79261

ChuanqiXu9 commented Jan 31, 2024

View reviewed changes

kwk mentioned this pull request Jan 31, 2024

Broken snapshot for 20240131 detected (big-merge) fedora-llvm-team/llvm-snapshots#234

Closed

steakhal mentioned this pull request Jul 2, 2024

[clang][docs] Move entries around #97416

Merged

steakhal added a commit that referenced this pull request Jul 2, 2024

[clang][docs] Move entries around (#97416)

9da86ae

Let's just move these under the `Non-comprehensive list of changes in this release` section. Resolves: - #79261 (comment) - #65484 (comment)

Support C++20 Modules in clang-repl #79261

Support C++20 Modules in clang-repl #79261

Uh oh!

Conversation

ChuanqiXu9 commented Jan 24, 2024

Uh oh!

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

Uh oh!

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

dyung commented Jan 24, 2024

Uh oh!

ChuanqiXu9 commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nathanchance commented Jan 25, 2024

Uh oh!

ChuanqiXu9 commented Jan 25, 2024

Uh oh!

vgvassilev commented Jan 25, 2024

Uh oh!

ChuanqiXu9 commented Jan 25, 2024

Uh oh!

nathanchance commented Jan 26, 2024

Uh oh!

vgvassilev commented Jan 26, 2024

Uh oh!

nathanchance commented Jan 26, 2024

Uh oh!

nikic commented Jan 29, 2024

Uh oh!

vgvassilev commented Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Jan 30, 2024

Uh oh!

vgvassilev commented Jan 30, 2024

Uh oh!

ChuanqiXu9 commented Jan 30, 2024

Uh oh!

ChuanqiXu9 commented Jan 30, 2024

Uh oh!

vgvassilev commented Jan 30, 2024

Uh oh!

ChuanqiXu9 commented Jan 30, 2024

Uh oh!

vgvassilev commented Jan 30, 2024

Uh oh!

nathanchance commented Jan 30, 2024

Uh oh!

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Uh oh!

ChuanqiXu9 Jan 31, 2024

Choose a reason for hiding this comment

Uh oh!

ChuanqiXu9 Jan 31, 2024

Choose a reason for hiding this comment

Uh oh!

nathanchance Jan 31, 2024

Choose a reason for hiding this comment

Uh oh!

ChuanqiXu9 Jan 31, 2024

Choose a reason for hiding this comment

Uh oh!

nathanchance Jan 31, 2024

Choose a reason for hiding this comment

Uh oh!

steakhal commented Jul 2, 2024

Uh oh!

vgvassilev commented Jul 2, 2024

Uh oh!

steakhal commented Jul 2, 2024

Uh oh!

vgvassilev commented Jul 2, 2024

Uh oh!

Uh oh!

llvmbot commented Jan 24, 2024 •

edited

Loading

ChuanqiXu9 commented Jan 24, 2024 •

edited

Loading

vgvassilev commented Jan 29, 2024 •

edited

Loading