Skip to content

Conversation

yozhu
Copy link
Contributor

@yozhu yozhu commented Jun 6, 2024

If default argument list has one more element than actual argument list and the last default argument is a pack expansion, we can ignore it. This is to fix a failed assertion:

clang: llvm-project/clang/lib/Sema/SemaTemplateDeduction.cpp:596: TemplateDeductionResult DeduceTemplateArguments(Sema &, TemplateParameterList *, TemplateName, TemplateName, TemplateDeductionInfo &, ArrayRef, SmallVectorImpl &): Assertion `DefaultArguments.size() <= As->size()' failed.

…of default argument list if actual argument list's length is one less
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 6, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 6, 2024

@llvm/pr-subscribers-clang

Author: None (yozhu)

Changes

If default argument list has one more element than actual argument list and the last default argument is a pack expansion, we can ignore it. This is to fix a failed assertion:

clang: llvm-project/clang/lib/Sema/SemaTemplateDeduction.cpp:596: TemplateDeductionResult DeduceTemplateArguments(Sema &, TemplateParameterList *, TemplateName, TemplateName, TemplateDeductionInfo &, ArrayRef<TemplateArgument>, SmallVectorImpl<DeducedTemplateArgument> &): Assertion `DefaultArguments.size() <= As->size()' failed.


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

1 Files Affected:

  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+8-2)
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index 1011db2d2830d..e66f35c38dfda 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -593,9 +593,15 @@ DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
 
       TemplateParameterList *As = TempArg->getTemplateParameters();
       if (DefaultArguments.size() != 0) {
-        assert(DefaultArguments.size() <= As->size());
+        if ((DefaultArguments.size() > As->size()) &&
+            (DefaultArguments.size() != As->size() + 1 ||
+             DefaultArguments.back().getKind() != TemplateArgument::Type ||
+             !isa<PackExpansionType>(DefaultArguments.back().getAsType()))) {
+          return TemplateDeductionResult::TooFewArguments;
+        }
         SmallVector<NamedDecl *, 4> Params(As->size());
-        for (unsigned I = 0; I < DefaultArguments.size(); ++I)
+        for (unsigned I = 0;
+             I < std::min((size_t)As->size(), DefaultArguments.size()); ++I)
           Params[I] = getTemplateParameterWithDefault(S, As->getParam(I),
                                                       DefaultArguments[I]);
         for (unsigned I = DefaultArguments.size(); I < As->size(); ++I)

Copy link
Contributor

@mizvekov mizvekov left a comment

Choose a reason for hiding this comment

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

Please include test case.

@yozhu
Copy link
Contributor Author

yozhu commented Jun 6, 2024

We started to hit this assertion with this patch #93433.

C-Reduce is taking a really long time, so just put up this quick bandit change and hopefully it would sparkle ideas of a better or proper fix.

@mizvekov
Copy link
Contributor

mizvekov commented Jun 6, 2024

I see, this looks more likely that the underlying issue existed prior to that patch, and only surfaced by happenstance.

In any case, this proposed patch adds a controversial deduction failure case, we never failed deduction here, and I can't see the justification.

FYI, c-vise usually performs faster than c-reduce, and it has more sensible pass defaults.

@yozhu yozhu closed this Jun 7, 2024
@yozhu yozhu deleted the templatedeductionfix branch June 24, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants