[Clang] Fix a crash when recursively callig a default member initializer.

[cor3ntin]

Summary

This fixes a regression introduced by ca61961380, that would lead
to a segfault due to stack exhaustion when recursively calling
a default member initializer.

Fixes #60082

I'm not able to get clang to emit a stack exhaustion warning,
which it seems like it should be able to.

[shafik]

I could not find any tests that test for this warning.

clang/lib/Sema/SemaExpr.cpp

5930

[cor3ntin]

In D142401#4074959, @shafik wrote:

I could not find any tests that test for this warning.

I'm unable to trigger the warning at all. I feel i should but just using that function resolves the crash, probably because it allocates more stack in a separate thread, and there is actually some code that avoids marking the same object used twice.
But the warning might still trigger on some platform, so i suspect the test i added might, sometimes, trigger a warning. I'm not exactly sure how to handle that

[aaron.ballman]

In D142401#4082356, @cor3ntin wrote:

In D142401#4074959, @shafik wrote:

I could not find any tests that test for this warning.

I'm unable to trigger the warning at all. I feel i should but just using that function resolves the crash, probably because it allocates more stack in a separate thread, and there is actually some code that avoids marking the same object used twice.
But the warning might still trigger on some platform, so i suspect the test i added might, sometimes, trigger a warning. I'm not exactly sure how to handle that

Tests for stack space are always really challenging because triggering the failure is so system specific. We only have one test for it (test/SemaTemplate/stack-exhaustion.cpp)

But do we need to run these on separate threads in all of those places to address the issue?

"and there is actually some code that avoids marking the same object used twice." makes me wonder if we're missing an early return somewhere that would reduce stack usage more and avoid needing to spin up threads for this. (The upside to this change is that the code compiles more often but the downside is that we're not addressing the memory pressure issue, just kicking the can down the road a bit.)

[erichkeane]

In D142401#4096478, @aaron.ballman wrote:

In D142401#4082356, @cor3ntin wrote:

In D142401#4074959, @shafik wrote:

I could not find any tests that test for this warning.

I'm unable to trigger the warning at all. I feel i should but just using that function resolves the crash, probably because it allocates more stack in a separate thread, and there is actually some code that avoids marking the same object used twice.
But the warning might still trigger on some platform, so i suspect the test i added might, sometimes, trigger a warning. I'm not exactly sure how to handle that

Tests for stack space are always really challenging because triggering the failure is so system specific. We only have one test for it (test/SemaTemplate/stack-exhaustion.cpp)

But do we need to run these on separate threads in all of those places to address the issue?

"and there is actually some code that avoids marking the same object used twice." makes me wonder if we're missing an early return somewhere that would reduce stack usage more and avoid needing to spin up threads for this. (The upside to this change is that the code compiles more often but the downside is that we're not addressing the memory pressure issue, just kicking the can down the road a bit.)

I agree for the most part, but the 'runWithSufficientStackSpace' doesn't run in a separate thread, it just wraps it in a function that makes our diagnostic for running out of memory better. But I think you're right, we probably should have SOMETHING that lets us exit early or defer constructing the initializers or something.

[aaron.ballman]

In D142401#4096498, @erichkeane wrote:

In D142401#4096478, @aaron.ballman wrote:

In D142401#4082356, @cor3ntin wrote:

In D142401#4074959, @shafik wrote:

I could not find any tests that test for this warning.

I'm unable to trigger the warning at all. I feel i should but just using that function resolves the crash, probably because it allocates more stack in a separate thread, and there is actually some code that avoids marking the same object used twice.
But the warning might still trigger on some platform, so i suspect the test i added might, sometimes, trigger a warning. I'm not exactly sure how to handle that

Tests for stack space are always really challenging because triggering the failure is so system specific. We only have one test for it (test/SemaTemplate/stack-exhaustion.cpp)

But do we need to run these on separate threads in all of those places to address the issue?

"and there is actually some code that avoids marking the same object used twice." makes me wonder if we're missing an early return somewhere that would reduce stack usage more and avoid needing to spin up threads for this. (The upside to this change is that the code compiles more often but the downside is that we're not addressing the memory pressure issue, just kicking the can down the road a bit.)

I agree for the most part, but the 'runWithSufficientStackSpace' doesn't run in a separate thread, it just wraps it in a function that makes our diagnostic for running out of memory better. But I think you're right, we probably should have SOMETHING that lets us exit early or defer constructing the initializers or something.

That's not entirely accurate: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Stack.h#L40 which calls into https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Stack.cpp#L66

It MIGHT get run in a new thread (if that's enabled) or it might not.

[erichkeane]

Huh.... TIL!

[aaron.ballman]

Aside from the commenting issue pointed out by @shafik, I think we should move the test case into its own test file in SemaCXX and that RUN line should *not* use -verify. This way, we can test that we don't crash but we don't have to worry about spurious test failures due to different machines having different stack exhaustion points.

[cor3ntin]

Address comments, implement Aaron's clever solution, Rebase and add release note

[cor3ntin]

Remove duplicated test

[shafik]

LGTM

[cor3ntin]

I don't think this is actually testable, ultimately we may run out of stack
any way, the warning (when emitted) even say so.

[aaron.ballman]

In D142401#4307493, @cor3ntin wrote:

I don't think this is actually testable, ultimately we may run out of stack
any way, the warning (when emitted) even say so.

Okay, that's a fair point and makes my suggestion invalid. I think you're right, testing this consistently will be tricky -- LGTM once the test is removed.

clang/docs/ReleaseNotes.rst

- Fix crash when generating code coverage information for `PseudoObjectExpr` in
   (`#45481 <https://github.com/llvm/llvm-project/issues/45481>`_)
 - Fix the assertion hit when a template consteval function appears in a nested
   consteval/constexpr call chain.
   (`#61142 <https://github.com/llvm/llvm-project/issues/61142>`_)
 - Clang now better diagnose placeholder types constrained with a concept that is
   not a type concept.
 - Fix crash when a doc comment contains a line splicing.
   (`#62054 <https://github.com/llvm/llvm-project/issues/62054>`_)
-- Work around with a clang coverage crash which happens when visiting 
-  expressions/statements with invalid source locations in non-assert builds. 
+- Work around with a clang coverage crash which happens when visiting
+  expressions/statements with invalid source locations in non-assert builds.
   Assert builds may still see assertions triggered from this.
 - Fix a failed assertion due to an invalid source location when trying to form
   a coverage report for an unresolved constructor expression.
   (`#62105 <https://github.com/llvm/llvm-project/issues/62105>`_)
 - Fix defaulted equality operator so that it does not attempt to compare unnamed
   bit-fields. This fixes:
   (`#61355 <https://github.com/llvm/llvm-project/issues/61335>`_) and
   (`#61417 <https://github.com/llvm/llvm-project/issues/61417>`_)
 - Fix crash after suggesting typo correction to constexpr if condition.
   (`#61885 <https://github.com/llvm/llvm-project/issues/61885>`_)
 - Clang constexpr evaluator now treats comparison of [[gnu::weak]]-attributed
   member pointer as an invalid expression.
 - Fix crash when member function contains invalid default argument.
   (`#62122 <https://github.com/llvm/llvm-project/issues/62122>`_)
 - Fix crash when handling undefined template partial specialization
   (`#61356 <https://github.com/llvm/llvm-project/issues/61356>`_)
 - Fix premature substitution into the constraints of an inherited constructor.
 - Fix crash when attempting to perform parenthesized initialization of an
   aggregate with a base class with only non-public constructors.
   (`#62296 <https://github.com/llvm/llvm-project/issues/62296>`_)
+- Fix a stack overflow issue when evaluating ``consteval`` default arguments.
+  (`#60082` <https://github.com/llvm/llvm-project/issues/60082>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/lib/Sema/SemaExpr.cpp

bool Sema::CheckCXXDefaultArgExpr(SourceLocation CallLoc, FunctionDecl *FD,
   //   is a function parameter scope of an immediate function.
   EnterExpressionEvaluationContext EvalContext(
       *this,
       FD->isConsteval() ? ExpressionEvaluationContext::ImmediateFunctionContext
                         : ExpressionEvaluationContext::PotentiallyEvaluated,
       Param);
   ExprEvalContexts.back().IsCurrentlyCheckingDefaultArgumentOrInitializer =
       SkipImmediateInvocations;
-  MarkDeclarationsReferencedInExpr(Init, /*SkipLocalVariables*/ true);
+  runWithSufficientStackSpace(CallLoc, [&] {
+    MarkDeclarationsReferencedInExpr(Init, /*SkipLocalVariables=*/true);
+  });
   return false;
 }
 
 struct ImmediateCallVisitor : public RecursiveASTVisitor<ImmediateCallVisitor> {
   bool HasImmediateCalls = false;
 
   bool shouldVisitImplicitCode() const { return true; }
 
if (!Init && !Param->hasUnparsedDefaultArg()) {
     // point where the enclosing initializer is used in a function call.
     ImmediateCallVisitor V;
     if (!NestedDefaultChecking)
       V.TraverseDecl(Param);
     if (V.HasImmediateCalls) {
       ExprEvalContexts.back().DelayedDefaultInitializationContext = {
           CallLoc, Param, CurContext};
       EnsureImmediateInvocationInDefaultArgs Immediate(*this);
-      ExprResult Res = Immediate.TransformInitializer(Param->getInit(),
-                                                      /*NotCopy=*/false);
+      ExprResult Res;
+      runWithSufficientStackSpace(CallLoc, [&] {
+        Res = Immediate.TransformInitializer(Param->getInit(),
+      });
       if (Res.isInvalid())
         return ExprError();
       Res = ConvertParamDefaultArgument(Param, Res.get(),
                                         Res.get()->getBeginLoc());
       if (Res.isInvalid())
         return ExprError();
       Init = Res.get();
     }
if (!NestedDefaultChecking)
     V.TraverseDecl(Field);
   if (V.HasImmediateCalls) {
     ExprEvalContexts.back().DelayedDefaultInitializationContext = {Loc, Field,
                                                                    CurContext};
     ExprEvalContexts.back().IsCurrentlyCheckingDefaultArgumentOrInitializer =
         NestedDefaultChecking;
 
     EnsureImmediateInvocationInDefaultArgs Immediate(*this);
-
-    ExprResult Res =
-        Immediate.TransformInitializer(Field->getInClassInitializer(),
+    ExprResult Res;
+    runWithSufficientStackSpace(Loc, [&] {
+      Res = Immediate.TransformInitializer(Field->getInClassInitializer(),
                                            /*CXXDirectInit=*/false);
+    });
     if (!Res.isInvalid())
       Res = ConvertMemberDefaultInitExpression(Field, Res.get(), Loc);
     if (Res.isInvalid()) {
       Field->setInvalidDecl();
       return ExprError();
     }
     Init = Res.get();
   }
 
   if (Field->getInClassInitializer()) {
     Expr *E = Init ? Init : Field->getInClassInitializer();
     if (!NestedDefaultChecking)
+      runWithSufficientStackSpace(Loc, [&] {
         MarkDeclarationsReferencedInExpr(E, /*SkipLocalVariables=*/false);
+      });
     // C++11 [class.base.init]p7:
     //   The initialization of each base and member constitutes a
     //   full-expression.
     ExprResult Res = ActOnFinishFullExpr(E, /*DiscardedValue=*/false);
     if (Res.isInvalid()) {
       Field->setInvalidDecl();
       return ExprError();
     }
void Sema::MarkFunctionReferenced(SourceLocation Loc, FunctionDecl *Func,
   }
 
   // If a constructor was defined in the context of a default parameter
   // or of another default member initializer (ie a PotentiallyEvaluatedIfUsed
   // context), its initializers may not be referenced yet.
   if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(Func)) {
     for (CXXCtorInitializer *Init : Constructor->inits()) {
       if (Init->isInClassMemberInitializer())
+        runWithSufficientStackSpace(Init->getSourceLocation(), [&]() {
           MarkDeclarationsReferencedInExpr(Init->getInit());
+        });
     }
   }
 
   // C++14 [except.spec]p17:
   //   An exception-specification is considered to be needed when:
   //   - the function is odr-used or, if it appears in an unevaluated operand,
   //     would be odr-used if the expression were potentially-evaluated;
   //