[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/dmpolukhin closed https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/dmpolukhin updated
https://github.com/llvm/llvm-project/pull/123648
>From e5cd06ddbc4193f9d5910eba93f0eb309d67063c Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Mon, 20 Jan 2025 09:03:25 -0800
Subject: [PATCH 1/7] [C++20][Modules] Fix crash/compiler error due broken AST
links
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Summary:
This PR fixes bugreport https://github.com/llvm/llvm-project/issues/122493
The root problem is the same as before lambda function and DeclRefExpr
references
a variable that does not belong to the same module as the enclosing function
body.
Therefore iteration over the function body doesn’t visit the VarDecl. Before
this
change RelatedDeclsMap was created only for canonical decl but in reality it
has to
be done for the definition of the function that does not always match the
canonical decl.
Test Plan: check-clang
---
clang/lib/Serialization/ASTWriterDecl.cpp | 5 +-
...ash-instantiated-in-scope-cxx-modules5.cpp | 92 +++
2 files changed, 95 insertions(+), 2 deletions(-)
create mode 100644
clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp
b/clang/lib/Serialization/ASTWriterDecl.cpp
index f8ed155ca389d7..371b4b29e85991 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back(
Writer.GetDeclRef(D));
}
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
new file mode 100644
index 00..ca80a3fd21dd00
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
@@ -0,0 +1,92 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=mock_resolver -o mock_resolver.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=sql_internal -o sql_internal.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -fmodule-file=mock_resolver.pcm
-fmodule-file=sql_internal.pcm main.cc -o main.o
+
+//--- module.cppmap
+module "mock_resolver" {
+ export *
+ module "mock_resolver.h" {
+export *
+header "mock_resolver.h"
+ }
+}
+
+module "sql_internal" {
+ export *
+ module "sql_transform_builder.h" {
+export *
+header "sql_transform_builder.h"
+ }
+}
+
+//--- set_bits2.h
+// expected-no-diagnostics
+#pragma once
+
+template
+void fwd(const T& x) {}
+
+namespace vox::bitset {
+
+template
+void ForEachSetBit2(const TFunc&) {
+ fwd([](int) {
+const int bit_index_base = 0;
+(void)[&](int) {
+ int v = bit_index_base;
+};
+ });
+}
+
+} // namespace vox::bitset
+
+//--- sql_transform_builder.h
+// expected-no-diagnostics
+#pragma once
+
+#include "set_bits2.h"
+
+class QualifyingSet3 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+template
+void DoTransform() {
+ vox::bitset::ForEachSetBit2([]() {});
+}
+
+//--- mock_resolver.h
+// expected-no-diagnostics
+#pragma once
+#include "set_bits2.h"
+
+class QualifyingSet2 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+//--- main.cc
+// expected-no-diagnostics
+#include "sql_transform_builder.h"
+
+template
+void get(const Callable& fn) {
+ fwd(fn);
+}
+
+namespace {
+
+void test() {
+ get([]() {});
+ DoTransform();
+}
+
+} // namespace
>From 01254792e2b75e00cc44259b27fcb4e38e719629 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Tue, 21 Jan 2025 02:32:33 -0800
Subject: [PATCH 2/7] Add -Werror=uninitialized to add compilation error in
builds without assserts All improve comments
---
clang/include/clang/Serialization/ASTReader.h | 11 ++-
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +++---
.../crash-instantiated-in-scope-cxx-modules5.cpp | 6 +++---
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 9f978762a6fb6b..d27a49863fc557 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -539,11 +539,12 @@ class ASTReader
/// Mapping from main decl ID to the related decls IDs.
//
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
@@ -799,14 +813,14 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
}
if (D->getFriendObjectKind()) {
-// For a function defined inline within a class template, we have to force
-// the canonical definition to be the one inside the canonical definition
of
-// the template. Remember this relation to deserialize them together.
-if (auto *RD = dyn_cast(D->getLexicalParent()))
- if (RD->isDependentContext() && RD->isThisDeclarationADefinition()) {
-Writer.RelatedDeclsMap[Writer.GetDeclRef(RD)].push_back(
-Writer.GetDeclRef(D));
- }
+// For a friend function defined inline within a class template, we have to
+// force the definition to be the one inside the definition of the template
+// class. Remember this relation to deserialize them together.
+if (auto *RD = dyn_cast(D->getLexicalParent());
+isDefinitionInDependentContext(RD)) {
dmpolukhin wrote:
Added but I think it cannot happen after `getFriendObjectKind` check.
https://github.com/llvm/llvm-project/pull/123648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/dmpolukhin updated
https://github.com/llvm/llvm-project/pull/123648
>From e5cd06ddbc4193f9d5910eba93f0eb309d67063c Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Mon, 20 Jan 2025 09:03:25 -0800
Subject: [PATCH 1/7] [C++20][Modules] Fix crash/compiler error due broken AST
links
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Summary:
This PR fixes bugreport https://github.com/llvm/llvm-project/issues/122493
The root problem is the same as before lambda function and DeclRefExpr
references
a variable that does not belong to the same module as the enclosing function
body.
Therefore iteration over the function body doesn’t visit the VarDecl. Before
this
change RelatedDeclsMap was created only for canonical decl but in reality it
has to
be done for the definition of the function that does not always match the
canonical decl.
Test Plan: check-clang
---
clang/lib/Serialization/ASTWriterDecl.cpp | 5 +-
...ash-instantiated-in-scope-cxx-modules5.cpp | 92 +++
2 files changed, 95 insertions(+), 2 deletions(-)
create mode 100644
clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp
b/clang/lib/Serialization/ASTWriterDecl.cpp
index f8ed155ca389d7..371b4b29e85991 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back(
Writer.GetDeclRef(D));
}
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
new file mode 100644
index 00..ca80a3fd21dd00
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
@@ -0,0 +1,92 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=mock_resolver -o mock_resolver.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=sql_internal -o sql_internal.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -fmodule-file=mock_resolver.pcm
-fmodule-file=sql_internal.pcm main.cc -o main.o
+
+//--- module.cppmap
+module "mock_resolver" {
+ export *
+ module "mock_resolver.h" {
+export *
+header "mock_resolver.h"
+ }
+}
+
+module "sql_internal" {
+ export *
+ module "sql_transform_builder.h" {
+export *
+header "sql_transform_builder.h"
+ }
+}
+
+//--- set_bits2.h
+// expected-no-diagnostics
+#pragma once
+
+template
+void fwd(const T& x) {}
+
+namespace vox::bitset {
+
+template
+void ForEachSetBit2(const TFunc&) {
+ fwd([](int) {
+const int bit_index_base = 0;
+(void)[&](int) {
+ int v = bit_index_base;
+};
+ });
+}
+
+} // namespace vox::bitset
+
+//--- sql_transform_builder.h
+// expected-no-diagnostics
+#pragma once
+
+#include "set_bits2.h"
+
+class QualifyingSet3 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+template
+void DoTransform() {
+ vox::bitset::ForEachSetBit2([]() {});
+}
+
+//--- mock_resolver.h
+// expected-no-diagnostics
+#pragma once
+#include "set_bits2.h"
+
+class QualifyingSet2 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+//--- main.cc
+// expected-no-diagnostics
+#include "sql_transform_builder.h"
+
+template
+void get(const Callable& fn) {
+ fwd(fn);
+}
+
+namespace {
+
+void test() {
+ get([]() {});
+ DoTransform();
+}
+
+} // namespace
>From 01254792e2b75e00cc44259b27fcb4e38e719629 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Tue, 21 Jan 2025 02:32:33 -0800
Subject: [PATCH 2/7] Add -Werror=uninitialized to add compilation error in
builds without assserts All improve comments
---
clang/include/clang/Serialization/ASTReader.h | 11 ++-
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +++---
.../crash-instantiated-in-scope-cxx-modules5.cpp | 6 +++---
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 9f978762a6fb6b..d27a49863fc557 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -539,11 +539,12 @@ class ASTReader
/// Mapping from main decl ID to the related decls IDs.
//
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
@@ -799,14 +813,14 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
}
if (D->getFriendObjectKind()) {
-// For a function defined inline within a class template, we have to force
-// the canonical definition to be the one inside the canonical definition
of
-// the template. Remember this relation to deserialize them together.
-if (auto *RD = dyn_cast(D->getLexicalParent()))
- if (RD->isDependentContext() && RD->isThisDeclarationADefinition()) {
-Writer.RelatedDeclsMap[Writer.GetDeclRef(RD)].push_back(
-Writer.GetDeclRef(D));
- }
+// For a friend function defined inline within a class template, we have to
+// force the definition to be the one inside the definition of the template
+// class. Remember this relation to deserialize them together.
+if (auto *RD = dyn_cast(D->getLexicalParent());
+isDefinitionInDependentContext(RD)) {
hokein wrote:
nit: we seem to miss checking `RD` is not `nullptr`
https://github.com/llvm/llvm-project/pull/123648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
@@ -539,11 +539,12 @@ class ASTReader /// Mapping from main decl ID to the related decls IDs. /// - /// These related decls have to be loaded right after the main decl. - /// It is required to have canonical declaration for related decls from the - /// same module as the enclosing main decl. Without this, due to lazy - /// deserialization, canonical declarations for the main decl and related can - /// be selected from different modules. + /// The key is the main decl ID, and the value is a vector of related decls + /// that must be loaded immediately after the main decl. This is necessary + /// to ensure that the definition for related decls comes from the same module + /// as the enclosing main decl. Without this, due to lazy deserialization, + /// the definition for the main decl and related decls may come from different + /// modules. hokein wrote: Thanks for confirming. I think it would be worth documenting this (it took me some time to fully understand what `RelatedDeclsMap` is and how it's used). Clear documentation could help others in the future. https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/hokein approved this pull request. Thanks for the fix! It looks good from my side, just small nits. Feel free to land it. https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/hokein edited https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
@@ -799,14 +817,12 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
}
if (D->getFriendObjectKind()) {
-// For a function defined inline within a class template, we have to force
-// the canonical definition to be the one inside the canonical definition
of
-// the template. Remember this relation to deserialize them together.
-if (auto *RD = dyn_cast(D->getLexicalParent()))
- if (RD->isDependentContext() && RD->isThisDeclarationADefinition()) {
-Writer.RelatedDeclsMap[Writer.GetDeclRef(RD)].push_back(
-Writer.GetDeclRef(D));
- }
+// For a friend function defined inline within a class template, we have to
+// force the definition to be the one inside the definition of the template
+// class. Remember this relation to deserialize them together.
+if (auto *RD = getDependentDeclContext(D))
dmpolukhin wrote:
You are right, it was unintended change in behaviour, reverted to tested
version. Thank you for catching it!
https://github.com/llvm/llvm-project/pull/123648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/dmpolukhin updated
https://github.com/llvm/llvm-project/pull/123648
>From e5cd06ddbc4193f9d5910eba93f0eb309d67063c Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Mon, 20 Jan 2025 09:03:25 -0800
Subject: [PATCH 1/6] [C++20][Modules] Fix crash/compiler error due broken AST
links
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Summary:
This PR fixes bugreport https://github.com/llvm/llvm-project/issues/122493
The root problem is the same as before lambda function and DeclRefExpr
references
a variable that does not belong to the same module as the enclosing function
body.
Therefore iteration over the function body doesn’t visit the VarDecl. Before
this
change RelatedDeclsMap was created only for canonical decl but in reality it
has to
be done for the definition of the function that does not always match the
canonical decl.
Test Plan: check-clang
---
clang/lib/Serialization/ASTWriterDecl.cpp | 5 +-
...ash-instantiated-in-scope-cxx-modules5.cpp | 92 +++
2 files changed, 95 insertions(+), 2 deletions(-)
create mode 100644
clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp
b/clang/lib/Serialization/ASTWriterDecl.cpp
index f8ed155ca389d7..371b4b29e85991 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back(
Writer.GetDeclRef(D));
}
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
new file mode 100644
index 00..ca80a3fd21dd00
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
@@ -0,0 +1,92 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=mock_resolver -o mock_resolver.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=sql_internal -o sql_internal.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -fmodule-file=mock_resolver.pcm
-fmodule-file=sql_internal.pcm main.cc -o main.o
+
+//--- module.cppmap
+module "mock_resolver" {
+ export *
+ module "mock_resolver.h" {
+export *
+header "mock_resolver.h"
+ }
+}
+
+module "sql_internal" {
+ export *
+ module "sql_transform_builder.h" {
+export *
+header "sql_transform_builder.h"
+ }
+}
+
+//--- set_bits2.h
+// expected-no-diagnostics
+#pragma once
+
+template
+void fwd(const T& x) {}
+
+namespace vox::bitset {
+
+template
+void ForEachSetBit2(const TFunc&) {
+ fwd([](int) {
+const int bit_index_base = 0;
+(void)[&](int) {
+ int v = bit_index_base;
+};
+ });
+}
+
+} // namespace vox::bitset
+
+//--- sql_transform_builder.h
+// expected-no-diagnostics
+#pragma once
+
+#include "set_bits2.h"
+
+class QualifyingSet3 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+template
+void DoTransform() {
+ vox::bitset::ForEachSetBit2([]() {});
+}
+
+//--- mock_resolver.h
+// expected-no-diagnostics
+#pragma once
+#include "set_bits2.h"
+
+class QualifyingSet2 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+//--- main.cc
+// expected-no-diagnostics
+#include "sql_transform_builder.h"
+
+template
+void get(const Callable& fn) {
+ fwd(fn);
+}
+
+namespace {
+
+void test() {
+ get([]() {});
+ DoTransform();
+}
+
+} // namespace
>From 01254792e2b75e00cc44259b27fcb4e38e719629 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Tue, 21 Jan 2025 02:32:33 -0800
Subject: [PATCH 2/6] Add -Werror=uninitialized to add compilation error in
builds without assserts All improve comments
---
clang/include/clang/Serialization/ASTReader.h | 11 ++-
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +++---
.../crash-instantiated-in-scope-cxx-modules5.cpp | 6 +++---
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 9f978762a6fb6b..d27a49863fc557 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -539,11 +539,12 @@ class ASTReader
/// Mapping from main decl ID to the related decls IDs.
//
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
@@ -799,14 +817,12 @@ void ASTDeclWriter::VisitFunctionDecl(FunctionDecl *D) {
}
if (D->getFriendObjectKind()) {
-// For a function defined inline within a class template, we have to force
-// the canonical definition to be the one inside the canonical definition
of
-// the template. Remember this relation to deserialize them together.
-if (auto *RD = dyn_cast(D->getLexicalParent()))
- if (RD->isDependentContext() && RD->isThisDeclarationADefinition()) {
-Writer.RelatedDeclsMap[Writer.GetDeclRef(RD)].push_back(
-Writer.GetDeclRef(D));
- }
+// For a friend function defined inline within a class template, we have to
+// force the definition to be the one inside the definition of the template
+// class. Remember this relation to deserialize them together.
+if (auto *RD = getDependentDeclContext(D))
hokein wrote:
this seems like a behavior change: before, it uses `D->getLexicalParent()`; now
it uses `D->getDeclContext()`.
https://github.com/llvm/llvm-project/pull/123648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/dmpolukhin edited https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/dmpolukhin commented: Added release note https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
@@ -539,11 +539,12 @@ class ASTReader /// Mapping from main decl ID to the related decls IDs. /// - /// These related decls have to be loaded right after the main decl. - /// It is required to have canonical declaration for related decls from the - /// same module as the enclosing main decl. Without this, due to lazy - /// deserialization, canonical declarations for the main decl and related can - /// be selected from different modules. + /// The key is the main decl ID, and the value is a vector of related decls + /// that must be loaded immediately after the main decl. This is necessary + /// to ensure that the definition for related decls comes from the same module + /// as the enclosing main decl. Without this, due to lazy deserialization, + /// the definition for the main decl and related decls may come from different + /// modules. dmpolukhin wrote: Yes, that's right understanding. Initially it was only for lambda functions but later case with inline friend functions were identified. The mechanism in its current form is very generic so it can be used for other case if we find them. https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/dmpolukhin updated
https://github.com/llvm/llvm-project/pull/123648
>From e5cd06ddbc4193f9d5910eba93f0eb309d67063c Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Mon, 20 Jan 2025 09:03:25 -0800
Subject: [PATCH 1/5] [C++20][Modules] Fix crash/compiler error due broken AST
links
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Summary:
This PR fixes bugreport https://github.com/llvm/llvm-project/issues/122493
The root problem is the same as before lambda function and DeclRefExpr
references
a variable that does not belong to the same module as the enclosing function
body.
Therefore iteration over the function body doesn’t visit the VarDecl. Before
this
change RelatedDeclsMap was created only for canonical decl but in reality it
has to
be done for the definition of the function that does not always match the
canonical decl.
Test Plan: check-clang
---
clang/lib/Serialization/ASTWriterDecl.cpp | 5 +-
...ash-instantiated-in-scope-cxx-modules5.cpp | 92 +++
2 files changed, 95 insertions(+), 2 deletions(-)
create mode 100644
clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp
b/clang/lib/Serialization/ASTWriterDecl.cpp
index f8ed155ca389d7..371b4b29e85991 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back(
Writer.GetDeclRef(D));
}
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
new file mode 100644
index 00..ca80a3fd21dd00
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
@@ -0,0 +1,92 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=mock_resolver -o mock_resolver.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=sql_internal -o sql_internal.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -fmodule-file=mock_resolver.pcm
-fmodule-file=sql_internal.pcm main.cc -o main.o
+
+//--- module.cppmap
+module "mock_resolver" {
+ export *
+ module "mock_resolver.h" {
+export *
+header "mock_resolver.h"
+ }
+}
+
+module "sql_internal" {
+ export *
+ module "sql_transform_builder.h" {
+export *
+header "sql_transform_builder.h"
+ }
+}
+
+//--- set_bits2.h
+// expected-no-diagnostics
+#pragma once
+
+template
+void fwd(const T& x) {}
+
+namespace vox::bitset {
+
+template
+void ForEachSetBit2(const TFunc&) {
+ fwd([](int) {
+const int bit_index_base = 0;
+(void)[&](int) {
+ int v = bit_index_base;
+};
+ });
+}
+
+} // namespace vox::bitset
+
+//--- sql_transform_builder.h
+// expected-no-diagnostics
+#pragma once
+
+#include "set_bits2.h"
+
+class QualifyingSet3 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+template
+void DoTransform() {
+ vox::bitset::ForEachSetBit2([]() {});
+}
+
+//--- mock_resolver.h
+// expected-no-diagnostics
+#pragma once
+#include "set_bits2.h"
+
+class QualifyingSet2 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+//--- main.cc
+// expected-no-diagnostics
+#include "sql_transform_builder.h"
+
+template
+void get(const Callable& fn) {
+ fwd(fn);
+}
+
+namespace {
+
+void test() {
+ get([]() {});
+ DoTransform();
+}
+
+} // namespace
>From 01254792e2b75e00cc44259b27fcb4e38e719629 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Tue, 21 Jan 2025 02:32:33 -0800
Subject: [PATCH 2/5] Add -Werror=uninitialized to add compilation error in
builds without assserts All improve comments
---
clang/include/clang/Serialization/ASTReader.h | 11 ++-
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +++---
.../crash-instantiated-in-scope-cxx-modules5.cpp | 6 +++---
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 9f978762a6fb6b..d27a49863fc557 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -539,11 +539,12 @@ class ASTReader
/// Mapping from main decl ID to the related decls IDs.
//
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/dmpolukhin updated
https://github.com/llvm/llvm-project/pull/123648
>From e5cd06ddbc4193f9d5910eba93f0eb309d67063c Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Mon, 20 Jan 2025 09:03:25 -0800
Subject: [PATCH 1/4] [C++20][Modules] Fix crash/compiler error due broken AST
links
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Summary:
This PR fixes bugreport https://github.com/llvm/llvm-project/issues/122493
The root problem is the same as before lambda function and DeclRefExpr
references
a variable that does not belong to the same module as the enclosing function
body.
Therefore iteration over the function body doesn’t visit the VarDecl. Before
this
change RelatedDeclsMap was created only for canonical decl but in reality it
has to
be done for the definition of the function that does not always match the
canonical decl.
Test Plan: check-clang
---
clang/lib/Serialization/ASTWriterDecl.cpp | 5 +-
...ash-instantiated-in-scope-cxx-modules5.cpp | 92 +++
2 files changed, 95 insertions(+), 2 deletions(-)
create mode 100644
clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp
b/clang/lib/Serialization/ASTWriterDecl.cpp
index f8ed155ca389d7..371b4b29e85991 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back(
Writer.GetDeclRef(D));
}
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
new file mode 100644
index 00..ca80a3fd21dd00
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
@@ -0,0 +1,92 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=mock_resolver -o mock_resolver.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=sql_internal -o sql_internal.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -fmodule-file=mock_resolver.pcm
-fmodule-file=sql_internal.pcm main.cc -o main.o
+
+//--- module.cppmap
+module "mock_resolver" {
+ export *
+ module "mock_resolver.h" {
+export *
+header "mock_resolver.h"
+ }
+}
+
+module "sql_internal" {
+ export *
+ module "sql_transform_builder.h" {
+export *
+header "sql_transform_builder.h"
+ }
+}
+
+//--- set_bits2.h
+// expected-no-diagnostics
+#pragma once
+
+template
+void fwd(const T& x) {}
+
+namespace vox::bitset {
+
+template
+void ForEachSetBit2(const TFunc&) {
+ fwd([](int) {
+const int bit_index_base = 0;
+(void)[&](int) {
+ int v = bit_index_base;
+};
+ });
+}
+
+} // namespace vox::bitset
+
+//--- sql_transform_builder.h
+// expected-no-diagnostics
+#pragma once
+
+#include "set_bits2.h"
+
+class QualifyingSet3 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+template
+void DoTransform() {
+ vox::bitset::ForEachSetBit2([]() {});
+}
+
+//--- mock_resolver.h
+// expected-no-diagnostics
+#pragma once
+#include "set_bits2.h"
+
+class QualifyingSet2 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+//--- main.cc
+// expected-no-diagnostics
+#include "sql_transform_builder.h"
+
+template
+void get(const Callable& fn) {
+ fwd(fn);
+}
+
+namespace {
+
+void test() {
+ get([]() {});
+ DoTransform();
+}
+
+} // namespace
>From 01254792e2b75e00cc44259b27fcb4e38e719629 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Tue, 21 Jan 2025 02:32:33 -0800
Subject: [PATCH 2/4] Add -Werror=uninitialized to add compilation error in
builds without assserts All improve comments
---
clang/include/clang/Serialization/ASTReader.h | 11 ++-
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +++---
.../crash-instantiated-in-scope-cxx-modules5.cpp | 6 +++---
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 9f978762a6fb6b..d27a49863fc557 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -539,11 +539,12 @@ class ASTReader
/// Mapping from main decl ID to the related decls IDs.
//
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/dmpolukhin updated
https://github.com/llvm/llvm-project/pull/123648
>From e5cd06ddbc4193f9d5910eba93f0eb309d67063c Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Mon, 20 Jan 2025 09:03:25 -0800
Subject: [PATCH 1/3] [C++20][Modules] Fix crash/compiler error due broken AST
links
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Summary:
This PR fixes bugreport https://github.com/llvm/llvm-project/issues/122493
The root problem is the same as before lambda function and DeclRefExpr
references
a variable that does not belong to the same module as the enclosing function
body.
Therefore iteration over the function body doesn’t visit the VarDecl. Before
this
change RelatedDeclsMap was created only for canonical decl but in reality it
has to
be done for the definition of the function that does not always match the
canonical decl.
Test Plan: check-clang
---
clang/lib/Serialization/ASTWriterDecl.cpp | 5 +-
...ash-instantiated-in-scope-cxx-modules5.cpp | 92 +++
2 files changed, 95 insertions(+), 2 deletions(-)
create mode 100644
clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp
b/clang/lib/Serialization/ASTWriterDecl.cpp
index f8ed155ca389d7..371b4b29e85991 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back(
Writer.GetDeclRef(D));
}
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
new file mode 100644
index 00..ca80a3fd21dd00
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
@@ -0,0 +1,92 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=mock_resolver -o mock_resolver.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=sql_internal -o sql_internal.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -fmodule-file=mock_resolver.pcm
-fmodule-file=sql_internal.pcm main.cc -o main.o
+
+//--- module.cppmap
+module "mock_resolver" {
+ export *
+ module "mock_resolver.h" {
+export *
+header "mock_resolver.h"
+ }
+}
+
+module "sql_internal" {
+ export *
+ module "sql_transform_builder.h" {
+export *
+header "sql_transform_builder.h"
+ }
+}
+
+//--- set_bits2.h
+// expected-no-diagnostics
+#pragma once
+
+template
+void fwd(const T& x) {}
+
+namespace vox::bitset {
+
+template
+void ForEachSetBit2(const TFunc&) {
+ fwd([](int) {
+const int bit_index_base = 0;
+(void)[&](int) {
+ int v = bit_index_base;
+};
+ });
+}
+
+} // namespace vox::bitset
+
+//--- sql_transform_builder.h
+// expected-no-diagnostics
+#pragma once
+
+#include "set_bits2.h"
+
+class QualifyingSet3 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+template
+void DoTransform() {
+ vox::bitset::ForEachSetBit2([]() {});
+}
+
+//--- mock_resolver.h
+// expected-no-diagnostics
+#pragma once
+#include "set_bits2.h"
+
+class QualifyingSet2 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+//--- main.cc
+// expected-no-diagnostics
+#include "sql_transform_builder.h"
+
+template
+void get(const Callable& fn) {
+ fwd(fn);
+}
+
+namespace {
+
+void test() {
+ get([]() {});
+ DoTransform();
+}
+
+} // namespace
>From 01254792e2b75e00cc44259b27fcb4e38e719629 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Tue, 21 Jan 2025 02:32:33 -0800
Subject: [PATCH 2/3] Add -Werror=uninitialized to add compilation error in
builds without assserts All improve comments
---
clang/include/clang/Serialization/ASTReader.h | 11 ++-
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +++---
.../crash-instantiated-in-scope-cxx-modules5.cpp | 6 +++---
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 9f978762a6fb6b..d27a49863fc557 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -539,11 +539,12 @@ class ASTReader
/// Mapping from main decl ID to the related decls IDs.
//
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
hokein wrote:
> But types are different and isThisDeclarationADefinition is not a virtual
> function.
We can use a template function.
> Moreover I'm not sure that conditions will be the same in future so
> extracting them to a function might be a wrong hint.
OK, that makes sense.
(this change seems to be an oversight in
https://github.com/llvm/llvm-project/pull/111992, where it was applied to
`RecordDecl` but not to `FunctionDecl`. Extracting this into a dedicated
function might be helpful and could potentially be reused in other parts of the
code.)
https://github.com/llvm/llvm-project/pull/123648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/hokein commented: We have initiated tests to verify this fix within our codebase. It will take some time to get the results. https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/hokein edited https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/dmpolukhin updated
https://github.com/llvm/llvm-project/pull/123648
>From e5cd06ddbc4193f9d5910eba93f0eb309d67063c Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Mon, 20 Jan 2025 09:03:25 -0800
Subject: [PATCH 1/2] [C++20][Modules] Fix crash/compiler error due broken AST
links
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Summary:
This PR fixes bugreport https://github.com/llvm/llvm-project/issues/122493
The root problem is the same as before lambda function and DeclRefExpr
references
a variable that does not belong to the same module as the enclosing function
body.
Therefore iteration over the function body doesn’t visit the VarDecl. Before
this
change RelatedDeclsMap was created only for canonical decl but in reality it
has to
be done for the definition of the function that does not always match the
canonical decl.
Test Plan: check-clang
---
clang/lib/Serialization/ASTWriterDecl.cpp | 5 +-
...ash-instantiated-in-scope-cxx-modules5.cpp | 92 +++
2 files changed, 95 insertions(+), 2 deletions(-)
create mode 100644
clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp
b/clang/lib/Serialization/ASTWriterDecl.cpp
index f8ed155ca389d7..371b4b29e85991 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back(
Writer.GetDeclRef(D));
}
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
new file mode 100644
index 00..ca80a3fd21dd00
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
@@ -0,0 +1,92 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=mock_resolver -o mock_resolver.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=sql_internal -o sql_internal.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -fmodule-file=mock_resolver.pcm
-fmodule-file=sql_internal.pcm main.cc -o main.o
+
+//--- module.cppmap
+module "mock_resolver" {
+ export *
+ module "mock_resolver.h" {
+export *
+header "mock_resolver.h"
+ }
+}
+
+module "sql_internal" {
+ export *
+ module "sql_transform_builder.h" {
+export *
+header "sql_transform_builder.h"
+ }
+}
+
+//--- set_bits2.h
+// expected-no-diagnostics
+#pragma once
+
+template
+void fwd(const T& x) {}
+
+namespace vox::bitset {
+
+template
+void ForEachSetBit2(const TFunc&) {
+ fwd([](int) {
+const int bit_index_base = 0;
+(void)[&](int) {
+ int v = bit_index_base;
+};
+ });
+}
+
+} // namespace vox::bitset
+
+//--- sql_transform_builder.h
+// expected-no-diagnostics
+#pragma once
+
+#include "set_bits2.h"
+
+class QualifyingSet3 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+template
+void DoTransform() {
+ vox::bitset::ForEachSetBit2([]() {});
+}
+
+//--- mock_resolver.h
+// expected-no-diagnostics
+#pragma once
+#include "set_bits2.h"
+
+class QualifyingSet2 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+//--- main.cc
+// expected-no-diagnostics
+#include "sql_transform_builder.h"
+
+template
+void get(const Callable& fn) {
+ fwd(fn);
+}
+
+namespace {
+
+void test() {
+ get([]() {});
+ DoTransform();
+}
+
+} // namespace
>From 01254792e2b75e00cc44259b27fcb4e38e719629 Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Tue, 21 Jan 2025 02:32:33 -0800
Subject: [PATCH 2/2] Add -Werror=uninitialized to add compilation error in
builds without assserts All improve comments
---
clang/include/clang/Serialization/ASTReader.h | 11 ++-
clang/lib/Serialization/ASTWriterDecl.cpp | 6 +++---
.../crash-instantiated-in-scope-cxx-modules5.cpp | 6 +++---
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/clang/include/clang/Serialization/ASTReader.h
b/clang/include/clang/Serialization/ASTReader.h
index 9f978762a6fb6b..d27a49863fc557 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -539,11 +539,12 @@ class ASTReader
/// Mapping from main decl ID to the related decls IDs.
//
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
dmpolukhin wrote:
But types are different and `isThisDeclarationADefinition` is not a virtual
function. Moreover I'm not sure that conditions will be the same in future so
extracting them to a function might be a wrong hint.
https://github.com/llvm/llvm-project/pull/123648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/hokein commented: > before this change RelatedDeclsMap was created only for canonical decl but in > reality it has to be done for the definition of the function that does not > always match the canonical decl. I'm new to the module codebase, and it is not obvious to me. Could you elaborate on this? Perhaps you could use the provided test case to illustrate the distinction. https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
hokein wrote:
This line looks identical to the one on line
[807](https://github.com/llvm/llvm-project/blob/0f9e913466982b92776d6ff748df48af28631517/clang/lib/Serialization/ASTWriterDecl.cpp#L807).
I guess we can pull out a small function.
https://github.com/llvm/llvm-project/pull/123648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/hokein edited https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/ChuanqiXu9 approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/123648 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
llvmbot wrote:
@llvm/pr-subscribers-clang-modules
Author: Dmitry Polukhin (dmpolukhin)
Changes
Summary:
This PR fixes bugreport https://github.com/llvm/llvm-project/issues/122493 The
root problem is the same as before lambda function and DeclRefExpr references a
variable that does not belong to the same module as the enclosing function
body. Therefore iteration over the function body doesn’t visit the VarDecl.
Before this change RelatedDeclsMap was created only for canonical decl but in
reality it has to be done for the definition of the function that does not
always match the canonical decl.
Test Plan: check-clang
---
Full diff: https://github.com/llvm/llvm-project/pull/123648.diff
2 Files Affected:
- (modified) clang/lib/Serialization/ASTWriterDecl.cpp (+3-2)
- (added) clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp (+92)
``diff
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp
b/clang/lib/Serialization/ASTWriterDecl.cpp
index f8ed155ca389d7..371b4b29e85991 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back(
Writer.GetDeclRef(D));
}
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
new file mode 100644
index 00..ca80a3fd21dd00
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
@@ -0,0 +1,92 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=mock_resolver -o mock_resolver.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=sql_internal -o sql_internal.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -fmodule-file=mock_resolver.pcm
-fmodule-file=sql_internal.pcm main.cc -o main.o
+
+//--- module.cppmap
+module "mock_resolver" {
+ export *
+ module "mock_resolver.h" {
+export *
+header "mock_resolver.h"
+ }
+}
+
+module "sql_internal" {
+ export *
+ module "sql_transform_builder.h" {
+export *
+header "sql_transform_builder.h"
+ }
+}
+
+//--- set_bits2.h
+// expected-no-diagnostics
+#pragma once
+
+template
+void fwd(const T& x) {}
+
+namespace vox::bitset {
+
+template
+void ForEachSetBit2(const TFunc&) {
+ fwd([](int) {
+const int bit_index_base = 0;
+(void)[&](int) {
+ int v = bit_index_base;
+};
+ });
+}
+
+} // namespace vox::bitset
+
+//--- sql_transform_builder.h
+// expected-no-diagnostics
+#pragma once
+
+#include "set_bits2.h"
+
+class QualifyingSet3 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+template
+void DoTransform() {
+ vox::bitset::ForEachSetBit2([]() {});
+}
+
+//--- mock_resolver.h
+// expected-no-diagnostics
+#pragma once
+#include "set_bits2.h"
+
+class QualifyingSet2 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+//--- main.cc
+// expected-no-diagnostics
+#include "sql_transform_builder.h"
+
+template
+void get(const Callable& fn) {
+ fwd(fn);
+}
+
+namespace {
+
+void test() {
+ get([]() {});
+ DoTransform();
+}
+
+} // namespace
``
https://github.com/llvm/llvm-project/pull/123648
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash/compiler error due broken AST links (PR #123648)
https://github.com/dmpolukhin created
https://github.com/llvm/llvm-project/pull/123648
Summary:
This PR fixes bugreport https://github.com/llvm/llvm-project/issues/122493 The
root problem is the same as before lambda function and DeclRefExpr references a
variable that does not belong to the same module as the enclosing function
body. Therefore iteration over the function body doesn’t visit the VarDecl.
Before this change RelatedDeclsMap was created only for canonical decl but in
reality it has to be done for the definition of the function that does not
always match the canonical decl.
Test Plan: check-clang
>From e5cd06ddbc4193f9d5910eba93f0eb309d67063c Mon Sep 17 00:00:00 2001
From: Dmitry Polukhin
Date: Mon, 20 Jan 2025 09:03:25 -0800
Subject: [PATCH] [C++20][Modules] Fix crash/compiler error due broken AST
links
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Summary:
This PR fixes bugreport https://github.com/llvm/llvm-project/issues/122493
The root problem is the same as before lambda function and DeclRefExpr
references
a variable that does not belong to the same module as the enclosing function
body.
Therefore iteration over the function body doesn’t visit the VarDecl. Before
this
change RelatedDeclsMap was created only for canonical decl but in reality it
has to
be done for the definition of the function that does not always match the
canonical decl.
Test Plan: check-clang
---
clang/lib/Serialization/ASTWriterDecl.cpp | 5 +-
...ash-instantiated-in-scope-cxx-modules5.cpp | 92 +++
2 files changed, 95 insertions(+), 2 deletions(-)
create mode 100644
clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
diff --git a/clang/lib/Serialization/ASTWriterDecl.cpp
b/clang/lib/Serialization/ASTWriterDecl.cpp
index f8ed155ca389d7..371b4b29e85991 100644
--- a/clang/lib/Serialization/ASTWriterDecl.cpp
+++ b/clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1571,9 +1571,10 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl *D)
{
} else {
Record.push_back(0);
}
-// For lambdas inside canonical FunctionDecl remember the mapping.
+// For lambdas inside template functions, remember the mapping to
+// deserialize them together.
if (auto FD = llvm::dyn_cast_or_null(D->getDeclContext());
-FD && FD->isCanonicalDecl()) {
+FD && FD->isDependentContext() && FD->isThisDeclarationADefinition()) {
Writer.RelatedDeclsMap[Writer.GetDeclRef(FD)].push_back(
Writer.GetDeclRef(D));
}
diff --git a/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
new file mode 100644
index 00..ca80a3fd21dd00
--- /dev/null
+++ b/clang/test/Headers/crash-instantiated-in-scope-cxx-modules5.cpp
@@ -0,0 +1,92 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=mock_resolver -o mock_resolver.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -emit-module module.cppmap
-fmodule-name=sql_internal -o sql_internal.pcm
+// RUN: %clang_cc1 -verify -std=c++20 -xc++ -fmodule-file=mock_resolver.pcm
-fmodule-file=sql_internal.pcm main.cc -o main.o
+
+//--- module.cppmap
+module "mock_resolver" {
+ export *
+ module "mock_resolver.h" {
+export *
+header "mock_resolver.h"
+ }
+}
+
+module "sql_internal" {
+ export *
+ module "sql_transform_builder.h" {
+export *
+header "sql_transform_builder.h"
+ }
+}
+
+//--- set_bits2.h
+// expected-no-diagnostics
+#pragma once
+
+template
+void fwd(const T& x) {}
+
+namespace vox::bitset {
+
+template
+void ForEachSetBit2(const TFunc&) {
+ fwd([](int) {
+const int bit_index_base = 0;
+(void)[&](int) {
+ int v = bit_index_base;
+};
+ });
+}
+
+} // namespace vox::bitset
+
+//--- sql_transform_builder.h
+// expected-no-diagnostics
+#pragma once
+
+#include "set_bits2.h"
+
+class QualifyingSet3 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+template
+void DoTransform() {
+ vox::bitset::ForEachSetBit2([]() {});
+}
+
+//--- mock_resolver.h
+// expected-no-diagnostics
+#pragma once
+#include "set_bits2.h"
+
+class QualifyingSet2 {
+ public:
+ void GetIndexes() const {
+vox::bitset::ForEachSetBit2([]() {});
+ }
+};
+
+//--- main.cc
+// expected-no-diagnostics
+#include "sql_transform_builder.h"
+
+template
+void get(const Callable& fn) {
+ fwd(fn);
+}
+
+namespace {
+
+void test() {
+ get([]() {});
+ DoTransform();
+}
+
+} // namespace
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
