Skip to content

[modules] Handle friend function that was a definition but became only a declaration during AST deserialization #132214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Apr 3, 2025
Merged

[modules] Handle friend function that was a definition but became only a declaration during AST deserialization #132214

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
91e057b
[clang] Fix for regression #130917
dmpolukhin Mar 20, 2025
adf6dfd
Add test case with C++ modules
dmpolukhin Mar 25, 2025
a564da2
Add flag to FunctionDecl
dmpolukhin Mar 25, 2025
2b42b4d
Fix formatting
dmpolukhin Mar 25, 2025
8d5ecfd
Add missing initialization
dmpolukhin Mar 25, 2025
df9fd8b
Move ThisDeclarationWasADefinition to ExternalASTSource
dmpolukhin Mar 28, 2025
d7234be
Rename + add only friend functions to the map
dmpolukhin Mar 28, 2025
94efa55
Move ThisDeclarationWasADefinition to separate map
dmpolukhin Apr 1, 2025
a24e8db
Replace map with set
dmpolukhin Apr 2, 2025
b887050
Fix formatting
dmpolukhin Apr 2, 2025
0a99d89
Add corresponding changes in MultiplexExternalSemaSource
dmpolukhin Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Copy link
Contributor

@mizvekov mizvekov Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clang AST is open to modifications.

Can't you add a new bit to Redeclarable to keep track of this?

Copy link
Contributor Author

@dmpolukhin dmpolukhin Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ChuanqiXu9 ChuanqiXu9 Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dmpolukhin dmpolukhin Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@mizvekov mizvekov Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@cor3ntin cor3ntin Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can add bits by shrinking

NumCtorInitializers.

however, in the present case it doesn't seem justified. these bits are precious.

Copy link
Contributor Author

@dmpolukhin dmpolukhin Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const FunctionDecl *Defn = nullptr;
if (D->hasBody(Defn)) {
D = const_cast<FunctionDecl *>(Defn);
Expand Down
21 changes: 21 additions & 0 deletions clang/test/SemaCXX/friend-default-parameters.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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