[modules] Handle friend function that was a definition but became only a declaration during AST deserialization #132214
Conversation
Changes in llvm#111992 was too broad. This change reduces scope of previous fix. Unfortunately in clang there is no way to know when redeclaration was craeted artificially due to AST mergse and when it was the case in the original code.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Dmitry Polukhin (dmpolukhin) ChangesChanges in #111992 was too broad. This change reduces scope of previous fix. Unfortunately in clang there is no way to know when redeclaration was craeted artificially due to AST mergse and when it was the case in the original code. I'm not sure that this fix covers all cases but at least it should fix regression in clang-20. The ideas how to fix it better are very wellcome. Full diff: https://github.com/llvm/llvm-project/pull/132214.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 539c2fdb83797..eda5d1151ab19 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -2572,7 +2572,7 @@ 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) {
+ if (isFriend && D->hasOwningModule()) {
const FunctionDecl *Defn = nullptr;
if (D->hasBody(Defn)) {
D = const_cast<FunctionDecl *>(Defn);
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
|
| @@ -2572,7 +2572,7 @@ 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) { | |||
| if (isFriend && D->hasOwningModule()) { | |||
There was a problem hiding this comment.
Clang AST is open to modifications.
Can't you add a new bit to Redeclarable to keep track of this?
There was a problem hiding this comment.
There is a problem with adding more flags to FunctionDecl, FunctionDeclBitfields is full and I cannot add there anything. Redeclarable doesn't have any flags as the moment and we need this extra flag only for FunctionDecls at the moment. So new version of the code works but most probably it cannot be committed as is. I also added another test case for the same example but inside module. My previous version was not able to compile it correctly.
There was a problem hiding this comment.
We can do this in ASTReader (ExternalASTSource). We can add a new interface to ExternalASTSource and calling it. It will be helpful since the call to wasThisDeclarationADefinition() should be cold.
There was a problem hiding this comment.
@ChuanqiXu9 sorry I'm not sure I fully understand your idea about ExternalASTSource. Are you suggesting to extend ASTReader::DefinitionSource to also keep this information there? Now ASTReader::DefinitionSource maps to bool but I can change value to a struct with two flags (previous DefinitionSource and ThisDeclarationWasADefinition), I think we don't need to store it in PCM file. Does it sound like the right approach?
There was a problem hiding this comment.
Oh yes, I took the last free bit in FunctionDeclBits a couple of months ago :)
This is a tall order, but it may be possible to make room in there. For example:
LLVM_PREFERRED_TYPE(bool)
uint64_t IsDefaulted : 1;
LLVM_PREFERRED_TYPE(bool)
uint64_t IsExplicitlyDefaulted : 1;It looks like IsExplicitlyDefaulted only makes sense if IsDefaulted is true, and maybe your new stuff only makes sense if IsDefaulted is false, so you can
probably share the IsExplicitlyDefaulted bit.
if that doesn't work, there are some other potential candidates.
For example, some bit fields only make sense if the function has a body, like UsesSEHTry, and these bits would probably be free for you as well.
There was a problem hiding this comment.
we can add bits by shrinking
NumCtorInitializers.
however, in the present case it doesn't seem justified. these bits are precious.
There was a problem hiding this comment.
I would prefer ExternalASTSource approach because it won't add any additional memory overhead and we already have hashtable ASTReader::DefinitionSource. Access to the filed should be super cold so it won't slowdown things. I will update this PR soon.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
It may be better to update the title to make it more descriptive. e.g., what it does. |
dmpolukhin
changed the title
| @@ -1390,7 +1390,19 @@ class ASTReader | |||
| /// predefines buffer may contain additional definitions. | |||
| std::string SuggestedPredefines; | |||
|
|
|||
| llvm::DenseMap<const Decl *, bool> DefinitionSource; | |||
| struct DefinitionSourceFlags { | |||
There was a problem hiding this comment.
| struct DefinitionSourceFlags { | |
| struct DefinitionSourceBits { |
nit: Personal taste
| ThisDeclarationWasADefinition(false) {} | ||
| }; | ||
|
|
||
| llvm::DenseMap<const Decl *, DefinitionSourceFlags> DefinitionSource; |
There was a problem hiding this comment.
It might be better to add another new map for it. e.g., Decl is not a function.
There was a problem hiding this comment.
I think memory overhead for another map will be higher. The value_type was bool but in reality at least 4-8 bytes were allocated for it so converting it to struct with 3 bits only shouldn't take extra space.
There was a problem hiding this comment.
But it may introduce unnecessary insertions. I still feel it is better to add another map. And this practice seems to be more scalarble to me, e.g, if we want to add other similar information, it may not be good to add all the information to the struct.
There was a problem hiding this comment.
IMHO it will only increase memory consumption but I don't have strong opinion here so moved to a new map. PTAL
|
LGTM generally. Some other personal suggestion to the title, it might be better to add |
dmpolukhin
changed the title
| }; | ||
|
|
||
| /// A mapping from declarations to extra bits of information about this decl. | ||
| llvm::DenseMap<const Decl *, ExternalDeclarationBits> |
There was a problem hiding this comment.
| llvm::DenseMap<const Decl *, ExternalDeclarationBits> | |
| llvm::DenseMap<const Decl *, bool> |
nit: I think we can avoid the abstraction here.
There was a problem hiding this comment.
In this case we don't even need map, I replaced with with a set.
| }; | ||
|
|
||
| /// A mapping from declarations to extra bits of information about this decl. | ||
| llvm::DenseMap<const Decl *, ExternalDeclarationBits> |
There was a problem hiding this comment.
In this case we don't even need map, I replaced with with a set.
Oh, thanks for the good catch! Fixed now. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/12077 Here is the relevant piece of the build log for the reference |
…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.
Fix for regression #130917, changes in #111992 were too broad. This change reduces scope of previous fix. Unfortunately in clang there is no way to know when redeclaration was craeted artificially due to AST mergse and when it was the case in the original code.
I'm not sure that this fix covers all cases but at least it should fix regression in clang-20. The ideas how to fix it better are very wellcome.