release/20.x: [modules] Handle friend function that was a definition but became only a declaration during AST deserialization (#132214) #134232
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…y a declaration during AST deserialization (llvm#132214) Fix for regression llvm#130917, changes in llvm#111992 were too broad. This change reduces scope of previous fix. Added `ExternalASTSource::wasThisDeclarationADefinition` to detect cases when FunctionDecl lost body due to declaration merges.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Dmitry Polukhin (dmpolukhin) ChangesFix for regression #130917, changes in #111992 were too broad. This change reduces scope of previous fix. Added Full diff: https://github.com/llvm/llvm-project/pull/134232.diff 11 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f4befc242f28b..e57fa9786e6f2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1065,6 +1065,7 @@ Bug Fixes to C++ Support
- Fixed an incorrect pointer access when checking access-control on concepts. (#GH131530)
- Fixed various alias CTAD bugs involving variadic template arguments. (#GH123591), (#GH127539), (#GH129077),
(#GH129620), and (#GH129998).
+- Fixed the false compilation error "redefinition of default argument" for friend functions with default parameters. (#GH130917)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/ExternalASTSource.h b/clang/include/clang/AST/ExternalASTSource.h
index 42aed56d42e07..f45e3af7602c1 100644
--- a/clang/include/clang/AST/ExternalASTSource.h
+++ b/clang/include/clang/AST/ExternalASTSource.h
@@ -191,6 +191,10 @@ class ExternalASTSource : public RefCountedBase<ExternalASTSource> {
virtual ExtKind hasExternalDefinitions(const Decl *D);
+ /// True if this function declaration was a definition before in its own
+ /// module.
+ virtual bool wasThisDeclarationADefinition(const FunctionDecl *FD);
+
/// Finds all declarations lexically contained within the given
/// DeclContext, after applying an optional filter predicate.
///
diff --git a/clang/include/clang/Sema/MultiplexExternalSemaSource.h b/clang/include/clang/Sema/MultiplexExternalSemaSource.h
index 921bebe3a44af..391c2177d75ec 100644
--- a/clang/include/clang/Sema/MultiplexExternalSemaSource.h
+++ b/clang/include/clang/Sema/MultiplexExternalSemaSource.h
@@ -92,6 +92,8 @@ class MultiplexExternalSemaSource : public ExternalSemaSource {
ExtKind hasExternalDefinitions(const Decl *D) override;
+ bool wasThisDeclarationADefinition(const FunctionDecl *FD) override;
+
/// Find all declarations with the given name in the
/// given context.
bool FindExternalVisibleDeclsByName(const DeclContext *DC,
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 47301419c76c6..23c98282f228f 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -1392,6 +1392,10 @@ class ASTReader
llvm::DenseMap<const Decl *, bool> DefinitionSource;
+ /// Friend functions that were defined but might have had their bodies
+ /// removed.
+ llvm::DenseSet<const FunctionDecl *> ThisDeclarationWasADefinitionSet;
+
bool shouldDisableValidationForFile(const serialization::ModuleFile &M) const;
/// Reads a statement from the specified cursor.
@@ -2375,6 +2379,8 @@ class ASTReader
ExtKind hasExternalDefinitions(const Decl *D) override;
+ bool wasThisDeclarationADefinition(const FunctionDecl *FD) override;
+
/// Retrieve a selector from the given module with its local ID
/// number.
Selector getLocalSelector(ModuleFile &M, unsigned LocalID);
diff --git a/clang/lib/AST/ExternalASTSource.cpp b/clang/lib/AST/ExternalASTSource.cpp
index e2451f294741d..3e865cb7679b5 100644
--- a/clang/lib/AST/ExternalASTSource.cpp
+++ b/clang/lib/AST/ExternalASTSource.cpp
@@ -38,6 +38,10 @@ ExternalASTSource::hasExternalDefinitions(const Decl *D) {
return EK_ReplyHazy;
}
+bool ExternalASTSource::wasThisDeclarationADefinition(const FunctionDecl *FD) {
+ return false;
+}
+
void ExternalASTSource::FindFileRegionDecls(FileID File, unsigned Offset,
unsigned Length,
SmallVectorImpl<Decl *> &Decls) {}
diff --git a/clang/lib/Sema/MultiplexExternalSemaSource.cpp b/clang/lib/Sema/MultiplexExternalSemaSource.cpp
index 6d945300c386c..fbfb242598c24 100644
--- a/clang/lib/Sema/MultiplexExternalSemaSource.cpp
+++ b/clang/lib/Sema/MultiplexExternalSemaSource.cpp
@@ -107,6 +107,14 @@ MultiplexExternalSemaSource::hasExternalDefinitions(const Decl *D) {
return EK_ReplyHazy;
}
+bool MultiplexExternalSemaSource::wasThisDeclarationADefinition(
+ const FunctionDecl *FD) {
+ for (const auto &S : Sources)
+ if (S->wasThisDeclarationADefinition(FD))
+ return true;
+ return false;
+}
+
bool MultiplexExternalSemaSource::FindExternalVisibleDeclsByName(
const DeclContext *DC, DeclarationName Name,
const DeclContext *OriginalDC) {
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 0c25b87439a95..7158e204db427 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -2175,11 +2175,13 @@ Decl *TemplateDeclInstantiator::VisitFunctionDecl(
// Friend function defined withing class template may stop being function
// definition during AST merges from different modules, in this case decl
// with function body should be used for instantiation.
- if (isFriend) {
- const FunctionDecl *Defn = nullptr;
- if (D->hasBody(Defn)) {
- D = const_cast<FunctionDecl *>(Defn);
- FunctionTemplate = Defn->getDescribedFunctionTemplate();
+ if (ExternalASTSource *Source = SemaRef.Context.getExternalSource()) {
+ if (isFriend && Source->wasThisDeclarationADefinition(D)) {
+ const FunctionDecl *Defn = nullptr;
+ if (D->hasBody(Defn)) {
+ D = const_cast<FunctionDecl *>(Defn);
+ FunctionTemplate = Defn->getDescribedFunctionTemplate();
+ }
}
}
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 427b3c82c4737..fcf25f5b66e07 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -9664,6 +9664,10 @@ ExternalASTSource::ExtKind ASTReader::hasExternalDefinitions(const Decl *FD) {
return I->second ? EK_Never : EK_Always;
}
+bool ASTReader::wasThisDeclarationADefinition(const FunctionDecl *FD) {
+ return ThisDeclarationWasADefinitionSet.contains(FD);
+}
+
Selector ASTReader::getLocalSelector(ModuleFile &M, unsigned LocalID) {
return DecodeSelector(getGlobalSelectorID(M, LocalID));
}
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index b4ff71c8958a4..2562756db1570 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -520,6 +520,9 @@ void ASTDeclReader::ReadFunctionDefinition(FunctionDecl *FD) {
}
// Store the offset of the body so we can lazily load it later.
Reader.PendingBodies[FD] = GetCurrentCursorOffset();
+ // For now remember ThisDeclarationWasADefinition only for friend functions.
+ if (FD->getFriendObjectKind())
+ Reader.ThisDeclarationWasADefinitionSet.insert(FD);
}
void ASTDeclReader::Visit(Decl *D) {
diff --git a/clang/test/SemaCXX/friend-default-parameters-modules.cpp b/clang/test/SemaCXX/friend-default-parameters-modules.cpp
new file mode 100644
index 0000000000000..9c4aff9f1964a
--- /dev/null
+++ b/clang/test/SemaCXX/friend-default-parameters-modules.cpp
@@ -0,0 +1,39 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -std=c++20 -fmodule-map-file=modules.map -xc++ -emit-module -fmodule-name=foo modules.map -o foo.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-map-file=modules.map -O1 -emit-obj main.cc -verify -fmodule-file=foo.pcm
+
+//--- modules.map
+module "foo" {
+ export *
+ module "foo.h" {
+ export *
+ header "foo.h"
+ }
+}
+
+//--- foo.h
+#pragma once
+
+template <int>
+void Create(const void* = nullptr);
+
+template <int>
+struct ObjImpl {
+ template <int>
+ friend void ::Create(const void*);
+};
+
+template <int I>
+void Create(const void*) {
+ (void) ObjImpl<I>{};
+}
+
+//--- main.cc
+// expected-no-diagnostics
+#include "foo.h"
+
+int main() {
+ Create<42>();
+}
diff --git a/clang/test/SemaCXX/friend-default-parameters.cpp b/clang/test/SemaCXX/friend-default-parameters.cpp
new file mode 100644
index 0000000000000..7190477ac496a
--- /dev/null
+++ b/clang/test/SemaCXX/friend-default-parameters.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -std=c++20 -verify -emit-llvm-only %s
+
+template <int>
+void Create(const void* = nullptr);
+
+template <int>
+struct ObjImpl {
+ template <int>
+ friend void ::Create(const void*);
+};
+
+template <int I>
+void Create(const void*) {
+ (void) ObjImpl<I>{};
+}
+
+int main() {
+ Create<42>();
+}
+
+// expected-no-diagnostics
|
nikic
requested changes
Apr 13, 2025
| @@ -1392,6 +1392,10 @@ class ASTReader | |||
|
|
|||
| llvm::DenseMap<const Decl *, bool> DefinitionSource; | |||
|
|
|||
| /// Friend functions that were defined but might have had their bodies | |||
| /// removed. | |||
| llvm::DenseSet<const FunctionDecl *> ThisDeclarationWasADefinitionSet; | |||
|
@nikic what do you mean by ABI change in this case? It doesn't change ABI of generated code, moreover it doesn't even change PCM serialized format because it is in memory only filed and attribute. |
It changes the ABI of libclang-cpp, by changing the layout of an exported type. |
It looks a bit strange requirement to me and significantly reduces ability to fix regression in release compilers. But if it is the release requirement, this fix cannot be cherry-pick to clang-20 so I abandon this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix for regression #130917, changes in #111992 were too broad. This change reduces scope of previous fix. Added
ExternalASTSource::wasThisDeclarationADefinitionto detect cases when FunctionDecl lost body due to declaration merges.