[clang] Allow consteval functions in default arguments

[ChuanqiXu]

Summary

We should not mark a function as "referenced" if we call it within a
ConstantExpr, because the expression will be folded to a value in LLVM IR.
To prevent emitting consteval function declarations, we should not "jump over"
a ConstantExpr when it is a top-level ParmVarDecl's subexpression.

Fixes https://github.com/llvm/llvm-project/issues/48230

[erichkeane]

clang/lib/AST/Decl.cpp

2870–2871

So what is happening here? I would still expect us to give the proper default-arg in this case? What subtly am I missing?

clang/lib/Sema/SemaExpr.cpp

18967

This part makes sense to me.

[cor3ntin]

There is also https://reviews.llvm.org/D74130 which tries to address the same issue in a very different way.
I don't really understand the different approaches yet.

[Izaron]

In D119646#3317473, @cor3ntin wrote:

There is also https://reviews.llvm.org/D74130 which tries to address the same issue in a very different way.
I don't really understand the different approaches yet.

Thanks! I spent some time researching this review (but tbh I don't fully understand it yet too). I can say why this patch seems to be so different.

The author aims to make this snippet working

consteval int f1() { return 0; }
consteval auto g() { return f1; }
consteval int h(int (*p)() = g()) { return p(); } // should work

That is, they want to make usable pointers to consteval function in default arguments. My patch doesn't compile this code now (the behaviour is same as in trunk).
As far as I see, the main problem is that the evaluation of ParmVarDecl's ConstantExpr is "isolated" from the evaluation of the consteval function body. Therefore the pointer "breaks the constexpr wall" and "falls out" to run-time, that's illegal. I'm not sure if my explanation is clear...

I wonder if it could be a different pull request later if someone will want to make pointers working? The review you mentioned is 2 years old, pretty massive and still has failing tests.

clang/lib/AST/Decl.cpp

2870–2871

If we have nested ConstantExprs within the default argument expression, like here

consteval int Fun(int x) { return x; }

struct Test {
  Test(int loc = Fun(1) * 3 + Fun(2)) {}
  // or, for example, Test(int loc = 0 + Fun(4));
};

Then the patch fixes it fine with just void VisitConstantExpr... (the snippet where you said "This part makes sense to me.").

The AST for ParmVarDecl looks like this - snippet on gist.github.com. Clang will fold the ConstantExprs.

---

However, if the ConstantExpr is top-level, that is, the code looks like this:

struct Test {
  Test(int loc = Fun(4)) {}
};

Then the AST looks like this - snippet on gist.github.com.

There are some places in the code where we call ParmVarDecl::getDefaultArg() (CodeGen/etc.) The callee will "jump over" the ConstantExpr (since it is derived from FullExpr) and give you the nested CallExpr. That's quite incorrent because we aren't going to evaluate this CallExpr in run-time. For example, if we don't patch this place, the CodeGen will declare Fun at LLVM IR, and everything breaks.

So I decided that we shouldn't "jump over" ConstantExpr, otherwise it incorrectly says to the caller that we are supposed to calculate it in run-time.

[ChuanqiXu]

This change LGTM and I prefer the change than D74130 since this one is much more comprehensible.

[erichkeane]

This seems innocuous enough, and seems to better reflect the intent of the standard, so LGTM.

[aaron.ballman]

LGTM as well, but please add a release note about the bug fix (and reference the issue it closes).

[ChuanqiXu]

Given this is close to complete and @Izaron is not active recently, let me take this over. The authority would be remained, of course.

[ChuanqiXu]

Add release notes.

[aaron.ballman]

LGTM with some small fixes to the release note. Thanks @ChuanqiXu!

clang/docs/ReleaseNotes.rst

166

Minor corrections

[ChuanqiXu]

Address comments.

clang/docs/ReleaseNotes.rst

 - Implement `CWG 2394 <https://wg21.link/cwg2394>`_: Const class members
   may be initialized with a defaulted default constructor under the same
   conditions it would be allowed for a const object elsewhere.
 - ``__has_unique_object_representations`` no longer reports that ``_BitInt`` types
   have unique object representations if they have padding bits.
 - Unscoped and scoped enumeration types can no longer be initialized from a
   brace-init-list containing a single element of a different scoped enumeration
   type.
 - Allow use of an elaborated type specifier as a ``_Generic`` selection
   association in C++ mode. This fixes
   `Issue 55562 <https://github.com/llvm/llvm-project/issues/55562>`_.
+- Clang will allow calling a ``consteval`` function in a default argument. This
+  fixes `Issue 48230 <https://github.com/llvm/llvm-project/issues/48230>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - ``-Wliteral-range`` will warn on floating-point equality comparisons with
   constants that are not representable in a casted value. For example,
   ``(float) f == 0.1`` is always false.
 - ``-Winline-namespace-reopened-noninline`` now takes into account that the
   ``inline`` keyword must appear on the original but not necessarily all

clang/lib/AST/Decl.cpp

 
 Expr *ParmVarDecl::getDefaultArg() {
   assert(!hasUnparsedDefaultArg() && "Default argument is not yet parsed!");
   assert(!hasUninstantiatedDefaultArg() &&
          "Default argument is not yet instantiated!");
 
   Expr *Arg = getInit();
   if (auto *E = dyn_cast_or_null<FullExpr>(Arg))
+    if (!isa<ConstantExpr>(E))
       return E->getSubExpr();
 
   return Arg;
 }
 
 void ParmVarDecl::setDefaultArg(Expr *defarg) {
   ParmVarDeclBits.DefaultArgKind = DAK_Normal;
   Init = defarg;
 }

clang/lib/Sema/SemaExpr.cpp

 
   // C++2a [basic.def.odr]p2:
   //   The set of potential results of an expression e is defined as follows:
   switch (E->getStmtClass()) {
   //   -- If e is an id-expression, ...
   case Expr::DeclRefExprClass: {
     auto *DRE = cast<DeclRefExpr>(E);
     if (DRE->isNonOdrUse() || IsPotentialResultOdrUsed(DRE->getDecl()))
       break;
 
     // Rebuild as a non-odr-use DeclRefExpr.
     MarkNotOdrUsed();
     return DeclRefExpr::Create(
         S.Context, DRE->getQualifierLoc(), DRE->getTemplateKeywordLoc(),
         DRE->getDecl(), DRE->refersToEnclosingVariableOrCapture(),
         DRE->getNameInfo(), DRE->getType(), DRE->getValueKind(),
         DRE->getFoundDecl(), CopiedTemplateArgs(DRE), NOUR);

   }
 
   void Visit(Expr *E) {
     if (llvm::is_contained(StopAt, E))
       return;
     Inherited::Visit(E);
   }
 
+  void VisitConstantExpr(ConstantExpr *E) {
+    // Don't mark declarations within a ConstantExpression, as this expression
+    // will be evaluated and folded to a value.
+    return;
+  }
+
   void VisitDeclRefExpr(DeclRefExpr *E) {
     // If we were asked not to visit local variables, don't.
     if (SkipLocalVariables) {
       if (VarDecl *VD = dyn_cast<VarDecl>(E->getDecl()))
         if (VD->hasLocalStorage())
           return;
     }
 

clang/test/SemaCXX/cxx2a-consteval.cpp

template <typename T> constexpr int baz() {
   return foo(t);
 }
 
 static_assert(bar<15>() == 15);
 static_assert(baz<int>() == sizeof(int));
 
 } // namespace value_dependent
 
+namespace default_argument {
+
+// Previously calls of consteval functions in default arguments were rejected.
+// Now we show that we don't reject such calls.
+consteval int foo() { return 1; }
+consteval int bar(int i = foo()) { return i * i; }
+
+struct Test1 {
+  Test1(int i = bar(13)) {}
+  void v(int i = bar(13) * 2 + bar(15)) {}
+};
+Test1 t1;
+
+struct Test2 {
+  constexpr Test2(int i = bar()) {}
+  constexpr void v(int i = bar(bar(bar(foo())))) {}
+};
+Test2 t2;
+
+} // namespace default_argument
+
 namespace PR50779 {
 struct derp {
   int b = 0;
 };
 
 constexpr derp d;
 
 struct test {