[clang] Do not serialize function definitions without a body #121550

alejandro-alvarez-sonarsource · 2025-01-03T08:33:05Z

An instantiated templated function definition may not have a body due to parsing errors inside the templated function. When serializing, an assert is triggered inside ASTRecordWriter::AddFunctionDefinition.

The instantiation may happen on an intermediate module.

The test case was reduced from mp-units.

An instantiated templated function definition may not have a body due to parsing errors inside the templated function. When serializing, an assert is tripped inside `ASTRecordWriter::AddFunctionDefinition` when building with assertions enabled. The instantiation may happen on an intermediate module. The test case was reduced from `mp-units`.

llvmbot · 2025-01-03T08:33:38Z

@llvm/pr-subscribers-clang-modules

Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource)

Changes

An instantiated templated function definition may not have a body due to parsing errors inside the templated function. When serializing, an assert is triggered inside ASTRecordWriter::AddFunctionDefinition.

The instantiation may happen on an intermediate module.

The test case was reduced from mp-units.

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

2 Files Affected:

(modified) clang/lib/Serialization/ASTWriter.cpp (+4-2)
(added) clang/test/Modules/missing-body-in-import.cpp (+42)

diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 4a6027943072c0..36b9e3e2ba1720 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -6227,8 +6227,10 @@ void ASTWriter::WriteDeclUpdatesBlocks(ASTContext &Context,
     // Add a trailing update record, if any. These must go last because we
     // lazily load their attached statement.
     if (!GeneratingReducedBMI || !CanElideDeclDef(D)) {
-      if (HasUpdatedBody) {
-        const auto *Def = cast<FunctionDecl>(D);
+      assert(!(HasUpdatedBody && HasAddedVarDefinition) &&
+             "Declaration can not be both a FunctionDecl and a VarDecl");
+      if (const auto *Def = dyn_cast<FunctionDecl>(D);
+          HasUpdatedBody && Def->doesThisDeclarationHaveABody()) {
         Record.push_back(UPD_CXX_ADDED_FUNCTION_DEFINITION);
         Record.push_back(Def->isInlined());
         Record.AddSourceLocation(Def->getInnerLocStart());
diff --git a/clang/test/Modules/missing-body-in-import.cpp b/clang/test/Modules/missing-body-in-import.cpp
new file mode 100644
index 00000000000000..b52ebba15087a3
--- /dev/null
+++ b/clang/test/Modules/missing-body-in-import.cpp
@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -std=c++23 mod1.cppm -emit-module-interface -o mod1.pcm -fallow-pcm-with-compiler-errors -verify
+// RUN: %clang_cc1 -std=c++23 mod2.cppm -emit-module-interface -o mod2.pcm -fmodule-file=mod1=mod1.pcm -verify -fallow-pcm-with-compiler-errors
+// RUN: %clang_cc1 -std=c++23 mod3.cppm -emit-module-interface -o mod3.pcm -fmodule-file=mod1=mod1.pcm -fmodule-file=mod2=mod2.pcm -verify -fallow-pcm-with-compiler-errors
+// RUN: %clang_cc1 -std=c++23 main.cpp -fmodule-file=mod1=mod1.pcm -fmodule-file=mod2=mod2.pcm -fmodule-file=mod3=mod3.pcm -verify -fallow-pcm-with-compiler-errors -ast-dump-all
+
+//--- mod1.cppm
+export module mod1;
+
+export template <unsigned N, unsigned M>
+class A {
+public:
+  constexpr A(const char[], const char[]) {
+    auto x = BrokenExpr; // expected-error {{use of undeclared identifier 'BrokenExpr'}}
+  }
+};
+
+export template<A<1,1> NTTP>
+struct B {};
+
+template < unsigned N, unsigned M >
+A(const char (&)[N], const char (&)[M]) -> A< 1, 1 >;
+
+//--- mod2.cppm
+export module mod2;
+import mod1;
+
+struct C: B <A{"a", "b"}> { // expected-error {{non-type template argument is not a constant expression}}
+  constexpr C(int a) { }
+};
+
+//--- mod3.cppm
+// expected-no-diagnostics
+export module mod3;
+export import mod2;
+
+//--- main.cpp
+// expected-no-diagnostics
+import mod3; // no crash

llvmbot · 2025-01-03T08:33:39Z

@llvm/pr-subscribers-clang

Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource)

Changes

An instantiated templated function definition may not have a body due to parsing errors inside the templated function. When serializing, an assert is triggered inside ASTRecordWriter::AddFunctionDefinition.

The instantiation may happen on an intermediate module.

The test case was reduced from mp-units.

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

2 Files Affected:

(modified) clang/lib/Serialization/ASTWriter.cpp (+4-2)
(added) clang/test/Modules/missing-body-in-import.cpp (+42)

diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index 4a6027943072c0..36b9e3e2ba1720 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -6227,8 +6227,10 @@ void ASTWriter::WriteDeclUpdatesBlocks(ASTContext &Context,
     // Add a trailing update record, if any. These must go last because we
     // lazily load their attached statement.
     if (!GeneratingReducedBMI || !CanElideDeclDef(D)) {
-      if (HasUpdatedBody) {
-        const auto *Def = cast<FunctionDecl>(D);
+      assert(!(HasUpdatedBody && HasAddedVarDefinition) &&
+             "Declaration can not be both a FunctionDecl and a VarDecl");
+      if (const auto *Def = dyn_cast<FunctionDecl>(D);
+          HasUpdatedBody && Def->doesThisDeclarationHaveABody()) {
         Record.push_back(UPD_CXX_ADDED_FUNCTION_DEFINITION);
         Record.push_back(Def->isInlined());
         Record.AddSourceLocation(Def->getInnerLocStart());
diff --git a/clang/test/Modules/missing-body-in-import.cpp b/clang/test/Modules/missing-body-in-import.cpp
new file mode 100644
index 00000000000000..b52ebba15087a3
--- /dev/null
+++ b/clang/test/Modules/missing-body-in-import.cpp
@@ -0,0 +1,42 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -std=c++23 mod1.cppm -emit-module-interface -o mod1.pcm -fallow-pcm-with-compiler-errors -verify
+// RUN: %clang_cc1 -std=c++23 mod2.cppm -emit-module-interface -o mod2.pcm -fmodule-file=mod1=mod1.pcm -verify -fallow-pcm-with-compiler-errors
+// RUN: %clang_cc1 -std=c++23 mod3.cppm -emit-module-interface -o mod3.pcm -fmodule-file=mod1=mod1.pcm -fmodule-file=mod2=mod2.pcm -verify -fallow-pcm-with-compiler-errors
+// RUN: %clang_cc1 -std=c++23 main.cpp -fmodule-file=mod1=mod1.pcm -fmodule-file=mod2=mod2.pcm -fmodule-file=mod3=mod3.pcm -verify -fallow-pcm-with-compiler-errors -ast-dump-all
+
+//--- mod1.cppm
+export module mod1;
+
+export template <unsigned N, unsigned M>
+class A {
+public:
+  constexpr A(const char[], const char[]) {
+    auto x = BrokenExpr; // expected-error {{use of undeclared identifier 'BrokenExpr'}}
+  }
+};
+
+export template<A<1,1> NTTP>
+struct B {};
+
+template < unsigned N, unsigned M >
+A(const char (&)[N], const char (&)[M]) -> A< 1, 1 >;
+
+//--- mod2.cppm
+export module mod2;
+import mod1;
+
+struct C: B <A{"a", "b"}> { // expected-error {{non-type template argument is not a constant expression}}
+  constexpr C(int a) { }
+};
+
+//--- mod3.cppm
+// expected-no-diagnostics
+export module mod3;
+export import mod2;
+
+//--- main.cpp
+// expected-no-diagnostics
+import mod3; // no crash

cor3ntin

I think this looks correct but I'd like @ChuanqiXu9 to confirm

ChuanqiXu9

Oh, previously we didn't allow -fallow-pcm-with-compiler-errors so we didn't meet such problems. Out of curiosity (not a blocking issue), why do you want this feature for C++20 modules?

ChuanqiXu9 · 2025-01-06T01:57:59Z

clang/lib/Serialization/ASTWriter.cpp

+      if (const auto *Def = dyn_cast<FunctionDecl>(D);
+          HasUpdatedBody && Def->doesThisDeclarationHaveABody()) {


Logically, it looks better to not set HasUpdatedBody if the declaration doesn't have a body.

Changed as suggested.

alejandro-alvarez-sonarsource · 2025-01-06T08:03:03Z

Out of curiosity (not a blocking issue), why do you want this feature for C++20 modules?

Sanitizers, linters and indexers have to deal with code using non supported C++ language features or extensions. This is also true for C++20 modules. While conceivably one may implement module support without going through the intermediate PCM, to keep the semantics close to what the compiler does, the most straightforward is to do what the compiler does and go throught the same codepath as when actually compiling 😃
And also, they would be able to not having to reparse everything and leverage one of the benefits of C++20 modules.

ChuanqiXu9 · 2025-01-06T08:07:41Z

Out of curiosity (not a blocking issue), why do you want this feature for C++20 modules?

Sanitizers, linters and indexers have to deal with code using non supported C++ language features or extensions. This is also true for C++20 modules. While conceivably one may implement module support without going through the intermediate PCM, to keep the semantics close to what the compiler does, the most straightforward is to do what the compiler does and go throught the same codepath as when actually compiling 😃 And also, they would be able to not having to reparse everything and leverage one of the benefits of C++20 modules.

Just to make sure I understand things correctly, you're saying:

// some.cc
import mod;
...

You hope the linter doesn't give up some.cc if mod contains some errors? If yes, that makes sense to me.

But why Sanitizers? IIRC, they should only work if the project compiles. Or are you saying the static analyzer?

alejandro-alvarez-sonarsource · 2025-01-06T08:12:09Z

You hope the linter doesn't give up some.cc if mod contains some errors? If yes, that makes sense to me.

Precisely.

But why Sanitizers? IIRC, they should only work if the project compiles. Or are you saying the static analyzer?

Sorry, my mind slipped there. Indeed I meant analyzers.

clang/lib/Serialization/ASTWriter.cpp

ChuanqiXu9 · 2025-01-06T08:21:04Z

clang/lib/Serialization/ASTWriter.cpp

+      if (Kind == UPD_CXX_ADDED_FUNCTION_DEFINITION) {
+        assert(isa<FunctionDecl>(D) && "expected FunctionDecl");
+        HasUpdatedBody = dyn_cast<FunctionDecl>(D)->hasBody();
+      } else if (Kind == UPD_CXX_ADDED_VAR_DEFINITION)


Suggested change

} else if (Kind == UPD_CXX_ADDED_VAR_DEFINITION)

} else if (Kind == UPD_CXX_ADDED_VAR_DEFINITION) {

Please see LLVM's policy for the use of { : https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

we hope the style is consistent with the if-else branches

ChuanqiXu9 · 2025-01-06T08:21:20Z

clang/lib/Serialization/ASTWriter.cpp

+      if (Kind == UPD_CXX_ADDED_FUNCTION_DEFINITION) {
+        assert(isa<FunctionDecl>(D) && "expected FunctionDecl");
+        HasUpdatedBody = dyn_cast<FunctionDecl>(D)->hasBody();
+      } else if (Kind == UPD_CXX_ADDED_VAR_DEFINITION)
        HasAddedVarDefinition = true;
      else


ditto,

Suggested change

else

else {

ChuanqiXu9

LGTM with nits

Co-authored-by: Chuanqi Xu <[email protected]>

ChuanqiXu9 · 2025-01-06T08:32:21Z

clang/lib/Serialization/ASTWriter.cpp

-      else if (Kind == UPD_CXX_ADDED_VAR_DEFINITION)
+      if (Kind == UPD_CXX_ADDED_FUNCTION_DEFINITION) {
+        assert(isa<FunctionDecl>(D) && "expected FunctionDecl");
+        HasUpdatedBody = cast<FunctionDecl>(D)->hasBody();


Sorry to skip this on the last iteration. We should use doesThisDeclarationHaveABody instead of hasBody. Since hasBody may be more expensive.

And also it is still slightly better to not set UPD_CXX_ADDED_FUNCTION_DEFINITION if the function doesn't have a body. We can check this in CompletedImplicitDefinition and VariableDefinitionInstantiated.

No problem! I want the get it right, not to rush it :D

Changed, although I imagine you meant FunctionDefinitionInstantiated (not VariableDefinitionInstantiated)

…tDefinition

github-actions · 2025-01-06T08:55:53Z

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

ChuanqiXu9

LGTM now.

We can land this after you fix the formatting issue. I generally use the following command to format my patch:

git diff -U0 --no-color --relative HEAD^ | clang/tools/clang-format/clang-format-diff.py -p1 -i

feel free to use other methods

ChuanqiXu9 · 2025-01-06T08:58:08Z

clang/lib/Serialization/ASTWriter.cpp

@@ -7230,6 +7230,9 @@ void ASTWriter::CompletedImplicitDefinition(const FunctionDecl *D) {
  if (!D->isFromASTFile())
    return; // Declaration not imported from PCH.

+  if (!D->doesThisDeclarationHaveABody())
+    return; // The function definition may not have a body due to parsing errors.


I prefer to see the comments above the if.

alejandro-alvarez-sonarsource · 2025-01-06T10:23:57Z

Thanks for the review! Would you mind doing the merge? I do not have permission.

ChuanqiXu9 · 2025-01-06T10:52:04Z

Thanks for the review! Would you mind doing the merge? I do not have permission.

I am just waiting the CI to be green : )

…1550) An instantiated templated function definition may not have a body due to parsing errors inside the templated function. When serializing, an assert is triggered inside `ASTRecordWriter::AddFunctionDefinition`. The instantiation may happen on an intermediate module. The test case was reduced from `mp-units`.

llvmbot added clang Clang issues not falling into any other category clang:modules C++20 modules and Clang Header Modules labels Jan 3, 2025

cor3ntin assigned ChuanqiXu9 Jan 3, 2025

cor3ntin approved these changes Jan 3, 2025

View reviewed changes

ChuanqiXu9 reviewed Jan 6, 2025

View reviewed changes

Do not set HasUpdatedBody

300707b

ChuanqiXu9 reviewed Jan 6, 2025

View reviewed changes

clang/lib/Serialization/ASTWriter.cpp Outdated Show resolved Hide resolved

ChuanqiXu9 reviewed Jan 6, 2025

View reviewed changes

ChuanqiXu9 approved these changes Jan 6, 2025

View reviewed changes

alejandro-alvarez-sonarsource and others added 2 commits January 6, 2025 09:24

Update clang/lib/Serialization/ASTWriter.cpp

6e0dbf5

Co-authored-by: Chuanqi Xu <[email protected]>

Add missing braces

f0a8e43

ChuanqiXu9 reviewed Jan 6, 2025

View reviewed changes

Move the check to FunctionDefinitionInstantiated and CompletedImplici…

3e2c300

…tDefinition

ChuanqiXu9 approved these changes Jan 6, 2025

View reviewed changes

Move comment above

cefbcf0

alejandro-alvarez-sonarsource force-pushed the aa/port_serialization_no_body branch from 7b2f79d to cefbcf0 Compare January 6, 2025 09:29

Merge branch 'main' into aa/port_serialization_no_body

4394ec9

ChuanqiXu9 merged commit a13bcf3 into llvm:main Jan 6, 2025
8 checks passed

		if (const auto *Def = dyn_cast<FunctionDecl>(D);
		HasUpdatedBody && Def->doesThisDeclarationHaveABody()) {

	} else if (Kind == UPD_CXX_ADDED_VAR_DEFINITION)
	} else if (Kind == UPD_CXX_ADDED_VAR_DEFINITION) {

[clang] Do not serialize function definitions without a body #121550

[clang] Do not serialize function definitions without a body #121550

Uh oh!

Conversation

alejandro-alvarez-sonarsource commented Jan 3, 2025

Uh oh!

llvmbot commented Jan 3, 2025

Uh oh!

llvmbot commented Jan 3, 2025

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alejandro-alvarez-sonarsource commented Jan 6, 2025

Uh oh!

ChuanqiXu9 commented Jan 6, 2025

Uh oh!

alejandro-alvarez-sonarsource commented Jan 6, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChuanqiXu9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 6, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alejandro-alvarez-sonarsource commented Jan 6, 2025

Uh oh!

ChuanqiXu9 commented Jan 6, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 6, 2025 •

edited

Loading