[Clang][Sema] fix outline member function template with default align crash #78400

jcsxky · 2024-01-17T06:20:04Z

Try to fix issue and some extented problem. Root cause of current issue is that error handling in instantiation of function parameter with default initialization on sizeof or align expression. When instance an out-of-line template member function, depth of TemplateTypeParmDecl in default initialization doesn't change while depth of other template parameter does and this will lead to some template parameter uninstanced. Also, sometime it will leader to wrong instantiation when it uses the template parameter of class.
Fix it by add template args of context when it's out-of-line. This will make MultiLevelTemplateArgumentList::getNumLevels matching the depth of template parameter. Testcase with some static_assert demonstrates the template parameter has been instanced correctly.

llvmbot · 2024-01-17T06:20:31Z

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

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

1 Files Affected:

(modified) clang/lib/Sema/SemaTemplateInstantiate.cpp (+1-1)

diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index fc80515b45e35b..92bbc4e51cb4c7 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -201,7 +201,7 @@ Response HandleFunction(const FunctionDecl *Function,
     // If this function was instantiated from a specialized member that is
     // a function template, we're done.
     assert(Function->getPrimaryTemplate() && "No function template?");
-    if (Function->getPrimaryTemplate()->isMemberSpecialization())
+    if (Function->getPrimaryTemplate()->isMemberSpecialization() && !Function->isOutOfLine())
       return Response::Done();
 
     // If this function is a generic lambda specialization, we are done.

github-actions · 2024-01-17T06:22:24Z

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

erichkeane

Also needs a release note.

erichkeane · 2024-01-22T14:38:33Z

clang/lib/Sema/SemaTemplateInstantiate.cpp

@@ -3051,6 +3051,7 @@ bool Sema::SubstDefaultArgument(
    //   default argument expression appears.
    ContextRAII SavedContext(*this, FD);
    std::unique_ptr<LocalInstantiationScope> LIS;
+    auto NewTemplateArgs = TemplateArgs;


Don't use auto here.

erichkeane · 2024-01-22T14:38:59Z

clang/lib/Sema/SemaTemplateInstantiate.cpp

+            getASTContext(), TemplateArgs.getInnermost());
+        NewTemplateArgs = getTemplateInstantiationArgs(
+            FD, FD->getDeclContext(), true, CurrentTemplateArgumentList, true,
+            nullptr, false, false);


Needs /*argname=*/ comments on bools/nullptrs.

All review has been applied.

erichkeane · 2024-01-23T14:13:54Z

clang/docs/ReleaseNotes.rst

@@ -859,6 +859,9 @@ Bug Fixes in This Version
  Fixes (`#78290 <https://github.com/llvm/llvm-project/issues/78290>`_)
 - Fixed assertion failure with deleted overloaded unary operators.
  Fixes (`#78314 <https://github.com/llvm/llvm-project/issues/78314>`_)
+- Fix crash when specialize out-of-line member function with default parameter


Suggested change

- Fix crash when specialize out-of-line member function with default parameter

- Fix a crash when specializing an out-of-line member function with a default parameter where we did an incorrect specialization of the initialization of the default parameter.

Is this accurate?

Yeah, thanks for your guidance of grammar and expression! I have update the note according to you suggestion.

erichkeane

LGTM, let me know if you need someone to merge this for you.

… crash

shafik

nitpick

shafik · 2024-01-26T22:31:54Z

clang/lib/Sema/SemaTemplateInstantiate.cpp

-      Result = SubstInitializer(PatternExpr, TemplateArgs,
-                                /*DirectInit*/false);
+      Result = SubstInitializer(PatternExpr, NewTemplateArgs,
+                                /*DirectInit*/ false);


/*DirectInit=*/false)

bgra8 · 2024-01-31T10:25:09Z

@jcsxky this patch introduced a regression. Please see the reproducer here: https://godbolt.org/z/rEfWP75Ta. It compiles before this change and fails at this change. It also compiles with gcc and clang-17.

jcsxky · 2024-01-31T13:35:50Z

@jcsxky this patch introduced a regression. Please see the reproducer here: https://godbolt.org/z/rEfWP75Ta. It compiles before this change and fails at this change. It also compiles with gcc and clang-17.

Sorry for importing new bugs. @erichkeane Could you help me revert this patch in order to not influence others since I am not familiar with github reverting operation? I afraid I would spent a long time to look into the root cause of the issue. Appreciate for you help and many thanks!

…lt align crash (llvm#78400)" This reverts commit 7b33899. A regression was discovered here: llvm#78400 and the author requested a revert to give time to review.

erichkeane · 2024-01-31T14:27:49Z

@jcsxky this patch introduced a regression. Please see the reproducer here: https://godbolt.org/z/rEfWP75Ta. It compiles before this change and fails at this change. It also compiles with gcc and clang-17.

Sorry for importing new bugs. @erichkeane Could you help me revert this patch in order to not influence others since I am not familiar with github reverting operation? I afraid I would spent a long time to look into the root cause of the issue. Appreciate for you help and many thanks!

No worries, happens all the time :) Reverting because you requested it, feel free to submit a new version of this review that fixes that as well! Revert will be commited from here: #80144

#80144) …lt align crash (#78400)" This reverts commit 7b33899. A regression was discovered here: #78400 and the author requested a revert to give time to review.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 17, 2024

jcsxky force-pushed the fix_member_function_template_with_default_align_crash branch 8 times, most recently from 07b6d21 to 774f1ab Compare January 20, 2024 04:16

jcsxky requested review from cor3ntin, AaronBallman and erichkeane January 20, 2024 04:31

jcsxky force-pushed the fix_member_function_template_with_default_align_crash branch from 774f1ab to e4ac395 Compare January 21, 2024 00:26

erichkeane reviewed Jan 22, 2024

View reviewed changes

jcsxky force-pushed the fix_member_function_template_with_default_align_crash branch 2 times, most recently from b9de8c5 to 852e2d2 Compare January 23, 2024 04:54

jcsxky requested a review from erichkeane January 23, 2024 05:35

erichkeane reviewed Jan 23, 2024

View reviewed changes

jcsxky force-pushed the fix_member_function_template_with_default_align_crash branch from 852e2d2 to 7166061 Compare January 24, 2024 01:16

jcsxky requested a review from erichkeane January 24, 2024 01:20

erichkeane approved these changes Jan 24, 2024

View reviewed changes

[Clang][Sema] fix outline member function template with default align…

fbf5cca

… crash

jcsxky force-pushed the fix_member_function_template_with_default_align_crash branch from 7166061 to fbf5cca Compare January 26, 2024 06:37

jcsxky merged commit 7b33899 into llvm:main Jan 26, 2024

shafik reviewed Jan 26, 2024

View reviewed changes

erichkeane mentioned this pull request Jan 31, 2024

Revert "[Clang][Sema] fix outline member function template with defau… #80144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang][Sema] fix outline member function template with default align crash #78400

[Clang][Sema] fix outline member function template with default align crash #78400

Uh oh!

jcsxky commented Jan 17, 2024 •

edited

Loading

Uh oh!

llvmbot commented Jan 17, 2024

Uh oh!

github-actions bot commented Jan 17, 2024 •

edited

Loading

Uh oh!

erichkeane left a comment

Uh oh!

erichkeane Jan 22, 2024

Uh oh!

erichkeane Jan 22, 2024

Uh oh!

jcsxky Jan 23, 2024

Uh oh!

erichkeane Jan 23, 2024

Uh oh!

jcsxky Jan 24, 2024

Uh oh!

erichkeane left a comment

Uh oh!

shafik left a comment

Uh oh!

shafik Jan 26, 2024

Uh oh!

bgra8 commented Jan 31, 2024

Uh oh!

jcsxky commented Jan 31, 2024

Uh oh!

erichkeane commented Jan 31, 2024

Uh oh!

Uh oh!

	- Fix crash when specialize out-of-line member function with default parameter
	- Fix a crash when specializing an out-of-line member function with a default parameter where we did an incorrect specialization of the initialization of the default parameter.

[Clang][Sema] fix outline member function template with default align crash #78400

[Clang][Sema] fix outline member function template with default align crash #78400

Uh oh!

Conversation

jcsxky commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 17, 2024

Uh oh!

github-actions bot commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

jcsxky Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

jcsxky Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

shafik Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

bgra8 commented Jan 31, 2024

Uh oh!

jcsxky commented Jan 31, 2024

Uh oh!

erichkeane commented Jan 31, 2024

Uh oh!

Uh oh!

jcsxky commented Jan 17, 2024 •

edited

Loading

github-actions bot commented Jan 17, 2024 •

edited

Loading