Skip to content

[clang] Fix high memory consumption during pack deduction #88637

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

Closed
term-est wants to merge 1 commit into from
Closed

[clang] Fix high memory consumption during pack deduction #88637

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/SemaTemplateDeduction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -831,7 +831,7 @@ class PackDeductionScope {
if (IsPartiallyExpanded)
PackElements += NumPartialPackArgs;
else if (IsExpanded)
PackElements += *FixedNumExpansions;
PackElements += FixedNumExpansions.value_or(1);
Comment on lines 833 to +834
Copy link
Member

@Sirraide Sirraide Apr 14, 2024

Choose a reason for hiding this comment

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

This seems fine as a fix, though ideally, it would be good to know what’s causing this so it’s easier to figure out what’s going wrong; my assumption is that we do something to the pack that causes IsExpanded to be true here but which wouldn’t have been true at the time FixedNumExpansions was (not) set.

Copy link

@tschuett tschuett Apr 14, 2024

Choose a reason for hiding this comment

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

Could you add an assert:

assert(FixedNumExpansions && "unexpected nullopt");

Copy link

@tschuett tschuett Apr 14, 2024

Choose a reason for hiding this comment

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

.value_or(1)

cannot be the solution. FixedNumExpansions is nullopt. Why is 1 an improvement over nullopt?

Copy link
Contributor

@mizvekov mizvekov Apr 14, 2024

Choose a reason for hiding this comment

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

The assert is not necessary, unless it added explanation.
The * operator on the optional here should already assert in that case.

I would advise you run this test case on an llvm build with runtime checks enabled. The compiler is going off the rails before this point, so maybe there is an earlier assert that could narrow it down.

Otherwise, the change lacks explanation, about what is the problem, and what the fix does.

Copy link
Contributor Author

@term-est term-est Apr 14, 2024

Choose a reason for hiding this comment

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

cannot be the solution. FixedNumExpansions is nullopt. Why is 1 an improvement over nullopt?
getExpandedPackSize checks whether the template parameter is a pack, and returns the size of the pack. If the parameter is not a pack, it returns a nullopt. This is why FixedNumExpansions is a nullopt, in which case I think value_or(1) makes sense as the template parameter is not a pack and it's just a single template parameter.

A similar approach seems to be used in getPackIndexForParam as well

    if (PD->isParameterPack()) {
      unsigned NumExpansions =
          S.getNumArgumentsInExpansion(PD->getType(), Args).value_or(1);

Since getExpandedPackSize might return nullopt, I assumed the template parameter not being a pack is a valid case and not a bug. Perhaps we can do

-    if (std::optional<unsigned> ExpandedPackExpansions =
-            getExpandedPackSize(TemplateParams->getParam(Index)))
-      FixedNumExpansions = ExpandedPackExpansions;
+ FixedNumExpansions = getExpandedPackSize(TemplateParams->getParam(Index)).value_or(1);

in addPack instead to make it more neat?

I opened this PR thinking that this was a simple issue, but if this is a deeper bug, we can look at ->

    if (Deduced[I].isNull() && Param->isTemplateParameterPack()) {
      if (auto Result =
              PackDeductionScope(S, TemplateParams, Deduced, Info, I).finish();
          Result != TemplateDeductionResult::Success)
        return Result;
    }

Here, Param->isTemplateParameterPack() seems to return true, while getExpandedPackSize returns nullopt

This is becauseisTemplateParameterPack checks whether the given parameter is a isParameterPack or not, while getExpandedPackSize checks for isExpandedParameterPack. In this specific case, former returns true while the latter returns false, so either the parameter is wrongly marked as a parameter pack, or it is not expanded when PackDeductionScope is constructed in `ConvertDeducedTemplateArguments'?

If this is indeed a deeper bug than I first thought and PackDeductionScope getting constructed with a Param that is not anisExpandedParameterPack is an issue, I think there is a missingTryExpandParameterPacks call at the TreeTransform stage? I am quite new to codebase so this is just a guess.

Please give me your opinion regarding this, and I will continue investigating this further depending on that.

Copy link
Member

@Sirraide Sirraide Apr 15, 2024

Choose a reason for hiding this comment

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

I assumed the template parameter not being a pack is a valid case and not a bug.

Yeah, I’m unfortunately not familiar enough w/ this part of Clang to comment on whether that’s the case or not; I’d have to look into this a bit more.

CC @AaronBallman, @erichkeane

Copy link
Collaborator

@erichkeane erichkeane Apr 16, 2024

Choose a reason for hiding this comment

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

I don't have a great idea here either. It seems to me that we shouldnt get here without a pack, and the bug is sooner, but I dont have a great idea of where we should be catching that case.


for (auto &Pack : Packs) {
if (Info.PendingDeducedPacks.size() > Pack.Index)
Expand Down