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

term-est · 2024-04-13T21:20:30Z

Henlo frens! 🍓

We folks at Armelsan Defense has been using LLVM for quite some time. Recently we encountered an issue with clangd where it tries to allocate 137 gigs of memory in one of our template heavy codebases.

We recompiled the trunk with sanitizers, and got this ->

I[20:24:45.715] <-- reply(1)
LLVM ERROR: SmallVector capacity unable to grow. Requested capacity (4294963200) is larger than maximum value for size type (4294967295)
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.

So this is not a leak. Notice that requested capacity is 4294963200, which is quite near to u32 max.

Further tests has showed that

  /// Prepare to directly deduce arguments of the parameter with index \p Index.
  PackDeductionScope(Sema &S, TemplateParameterList *TemplateParams,
                     SmallVectorImpl<DeducedTemplateArgument> &Deduced,
                     TemplateDeductionInfo &Info, unsigned Index)
      : S(S), TemplateParams(TemplateParams), Deduced(Deduced), Info(Info) {
    addPack(Index);
    finishConstruction(1);
  }
  
private:
  void addPack(unsigned Index) {
    // Save the deduced template argument for the parameter pack expanded
    // by this pack expansion, then clear out the deduction.
    DeducedFromEarlierParameter = !Deduced[Index].isNull();
    DeducedPack Pack(Index);
    Pack.Saved = Deduced[Index];
    Deduced[Index] = TemplateArgument();

    // FIXME: What if we encounter multiple packs with different numbers of
    // pre-expanded expansions? (This should already have been diagnosed
    // during substitution.)
    if (std::optional<unsigned> ExpandedPackExpansions =
            getExpandedPackSize(TemplateParams->getParam(Index)))
      FixedNumExpansions = ExpandedPackExpansions;

    Packs.push_back(Pack);
[clangd-stacktrace.txt](https://github.com/llvm/llvm-project/files/14968656/clangd-stacktrace.txt)

  }

addPack might not initialize the std::optional<unsigned> FixedNumExpansions given that getExpandedPackSize returns a nullopt, which causes the access to FixedNumExpansions via the operator* to be Undefined. PackElements is eventually used in SmallVector::grow_pod, and vector tries to allocate 137 gigs.

Attached, you can find the full stacktrace.
clangd-stacktrace.txt

I can supply the exact code that causes this issue if needed, but I would appreciate if you frends can point me to any tools that can generate an obfuscated minimal reproducible example.

Although this was discovered in clangd, it also appears to affect clang++ as well.

After this change, both seems to work just fine with clangd using only 300mb and clang++ compiling the codebase successfully and correctly.

Thank you for your amazing work and thanks for the review~

PackDeductionScope::PackDeductionScope calls PackDeductionScope::addPack, which might not assign a value to PackDeductionScope::FixedNumExpansions given that getExpandedPackSize returns a nullopt. Access to an empty std::optional via the operator* is UB, and there is a case where IsExpanded is true while FixedNumberExpansions is empty. We access the empty optional, and this value is eventually used to in a call to SmallVector::reserve, which ends up trying to reserve 137 gigs of space and crashes clangd/clang++

github-actions · 2024-04-13T21:20:48Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-04-13T21:21:16Z

@llvm/pr-subscribers-clang

Author: None (term-est)

Changes

Henlo frens! 🍓

We folks at Armelsan Defense has been using LLVM for quite some time. Recently we encountered an issue with clangd where it tries to allocate 137 gigs of memory in one of our template heavy codebases.

We recompiled the trunk with sanitizers, and got this ->

I[20:24:45.715] &lt;-- reply(1)
LLVM ERROR: SmallVector capacity unable to grow. Requested capacity (4294963200) is larger than maximum value for size type (4294967295)
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.

So this is not a leak. Notice that requested capacity is 4294963200, which is quite near to i32 max.

Further tests has showed that

  /// Prepare to directly deduce arguments of the parameter with index \p Index.
  PackDeductionScope(Sema &amp;S, TemplateParameterList *TemplateParams,
                     SmallVectorImpl&lt;DeducedTemplateArgument&gt; &amp;Deduced,
                     TemplateDeductionInfo &amp;Info, unsigned Index)
      : S(S), TemplateParams(TemplateParams), Deduced(Deduced), Info(Info) {
    addPack(Index);
    finishConstruction(1);
  }
  
private:
  void addPack(unsigned Index) {
    // Save the deduced template argument for the parameter pack expanded
    // by this pack expansion, then clear out the deduction.
    DeducedFromEarlierParameter = !Deduced[Index].isNull();
    DeducedPack Pack(Index);
    Pack.Saved = Deduced[Index];
    Deduced[Index] = TemplateArgument();

    // FIXME: What if we encounter multiple packs with different numbers of
    // pre-expanded expansions? (This should already have been diagnosed
    // during substitution.)
    if (std::optional&lt;unsigned&gt; ExpandedPackExpansions =
            getExpandedPackSize(TemplateParams-&gt;getParam(Index)))
      FixedNumExpansions = ExpandedPackExpansions;

    Packs.push_back(Pack);
[clangd-stacktrace.txt](https://github.com/llvm/llvm-project/files/14968656/clangd-stacktrace.txt)

  }

addPack might not initialize the std::optional<unsigned> FixedNumExpansions given that getExpandedPackSize returns a nullopt, which causes the access to FixedNumExpansions via the operator* to be Undefined. PackElements is eventually used in SmallVector::grow_pod, and vector tries to allocate 137 gigs.

Attached, you can find the full stacktrace.
clangd-stacktrace.txt

I can supply the exact code that causes this issue if needed, but I would appreciate if you frends can point me to any tools that can generate an obfuscated minimal reproducible example.

Although this was discovered in clangd, it also appears to affect clang++ as well.

After this change, both seems to work just fine with clangd using only 300mb and clang++ compiling the codebase successfully and correctly.

Thank you for your amazing work and thanks for the review~

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

1 Files Affected:

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

diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index 0b6375001f5326..1679852cdb386b 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -831,7 +831,7 @@ class PackDeductionScope {
     if (IsPartiallyExpanded)
       PackElements += NumPartialPackArgs;
     else if (IsExpanded)
-      PackElements += *FixedNumExpansions;
+      PackElements += FixedNumExpansions.value_or(1);
 
     for (auto &Pack : Packs) {
       if (Info.PendingDeducedPacks.size() > Pack.Index)

github-actions · 2024-04-13T23:03:51Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

Endilll · 2024-04-14T04:05:48Z

I'm not sure this is testable, but if you can think of a test, that would be nice.
Can you also add a release note to clang/ReleaseNotes.rst?

Sirraide · 2024-04-14T04:35:35Z

clang/lib/Sema/SemaTemplateDeduction.cpp

    else if (IsExpanded)
-      PackElements += *FixedNumExpansions;
+      PackElements += FixedNumExpansions.value_or(1);


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.

Could you add an assert:

assert(FixedNumExpansions && "unexpected nullopt");

.value_or(1)

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

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.

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.

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

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.

Sirraide · 2024-04-14T04:39:43Z

I can supply the exact code that causes this issue if needed, but I would appreciate if you frends can point me to any tools that can generate an obfuscated minimal reproducible example.

There’s creduce and cvise from what I recall, but I’m not particularly good at using them. Depending on how much work that would be, you could try doing ‘binary search’ manually (i.e. delete half the code, see if it compiles; if it does, the problem is in the other half). Seeing as it does seem to have something to do w/ template deduction in variadic templates, that might help narrow it down a bit.

term-est · 2024-04-14T07:28:46Z

Thank you for your feedback! Will add a reproducible example and update the ReleaseNotes.rst
I will also try to add a test if I can~

term-est · 2024-04-14T23:24:24Z

Unfortunately I am unable to generate a MRE at this time. I was hoping for a ~100loc reproducable example, but all tools either spit out hundreds of thousands of lines long preprocessed files or doesn't work at all.

I'll talk with my company to see if they would allow me to share the codebase which causes this specific issue, which is just around 2kloc and quite small.

cor3ntin · 2024-04-16T19:16:38Z

@mizvekov are you able to take a look at this? Thanks!

mizvekov · 2024-04-18T20:45:58Z

@term-est It would be helpful if you could post the surrounding code context where this crashed, This is pointed at the top of the stacktrace:

0.	/home/est/Working-Directory/DS/wetend-emulator-new/WetendEmulator.hpp:85:128: current parser token ')'

You said, and I confirm from the stack trace, it looks like this happened in a clangd invocation.
Does it also happen during compilation with clang?

Are you able to share prints and dumps from values surrounding the crash site?
Some examples:

NumNamedPacks
Info.getNumExplicitArgs()
Info.getDeducedDepth()
S.CurrentInstantiationScope != nullptr
PartialPackDepthIndex
Packs[...].Index

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

term-est changed the title ~~Fix an issue where clang++ and clang uses enormous amounts of memory~~ Fix an issue where clang++ and clangd uses enormous amounts of memory Apr 13, 2024

Endilll requested review from cor3ntin, AaronBallman, shafik, erichkeane and Sirraide April 14, 2024 03:57

Endilll changed the title ~~Fix an issue where clang++ and clangd uses enormous amounts of memory~~ [clang] Fix high memory consumption during pack deduction Apr 14, 2024

Sirraide reviewed Apr 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

term-est commented Apr 13, 2024 •

edited

github-actions bot commented Apr 13, 2024

llvmbot commented Apr 13, 2024

github-actions bot commented Apr 13, 2024

Endilll commented Apr 14, 2024

Sirraide Apr 14, 2024

tschuett Apr 14, 2024

tschuett Apr 14, 2024

mizvekov Apr 14, 2024

term-est Apr 14, 2024

Sirraide Apr 15, 2024

erichkeane Apr 16, 2024

Sirraide commented Apr 14, 2024

term-est commented Apr 14, 2024

term-est commented Apr 14, 2024

cor3ntin commented Apr 16, 2024

mizvekov commented Apr 18, 2024

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

Are you sure you want to change the base?

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

Conversation

term-est commented Apr 13, 2024 • edited

github-actions bot commented Apr 13, 2024

llvmbot commented Apr 13, 2024

github-actions bot commented Apr 13, 2024

Endilll commented Apr 14, 2024

Sirraide Apr 14, 2024

Choose a reason for hiding this comment

tschuett Apr 14, 2024

Choose a reason for hiding this comment

tschuett Apr 14, 2024

Choose a reason for hiding this comment

mizvekov Apr 14, 2024

Choose a reason for hiding this comment

term-est Apr 14, 2024

Choose a reason for hiding this comment

Sirraide Apr 15, 2024

Choose a reason for hiding this comment

erichkeane Apr 16, 2024

Choose a reason for hiding this comment

Sirraide commented Apr 14, 2024

term-est commented Apr 14, 2024

term-est commented Apr 14, 2024

cor3ntin commented Apr 16, 2024

mizvekov commented Apr 18, 2024

term-est commented Apr 13, 2024 •

edited