Page MenuHomePhabricator

[clang] Allow consteval functions in default arguments
ClosedPublic

Authored by ChuanqiXu on Feb 12 2022, 11:50 AM.

Details

Reviewers
aaron.ballman
erichkeane
andreasfertig
Izaron
Group Reviewers
Restricted Project
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

Diff Detail

Event Timeline

Izaron created this revision.Feb 12 2022, 11:50 AM
Izaron requested review of this revision.Feb 12 2022, 11:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2022, 11:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
erichkeane added inline comments.Feb 12 2022, 7:41 PM
clang/lib/AST/Decl.cpp
2816

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.

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.

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
2816

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 added a reviewer: Restricted Project.May 30 2022, 7:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2022, 7:27 PM
ChuanqiXu accepted this revision as: ChuanqiXu.May 30 2022, 7:28 PM
ChuanqiXu added a subscriber: ChuanqiXu.

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

This revision is now accepted and ready to land.May 30 2022, 7:28 PM
erichkeane accepted this revision.May 31 2022, 7:06 AM

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

aaron.ballman accepted this revision.Jun 1 2022, 9:08 AM

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

ChuanqiXu commandeered this revision.Jun 5 2022, 7:57 PM
ChuanqiXu edited reviewers, added: Izaron; removed: 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 updated this revision to Diff 434373.Jun 5 2022, 7:58 PM

Add release notes.

aaron.ballman accepted this revision.Jun 6 2022, 5:30 AM

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

clang/docs/ReleaseNotes.rst
166 ↗(On Diff #434373)

Minor corrections

ChuanqiXu updated this revision to Diff 434680.Jun 6 2022, 7:55 PM

Address comments.

ChuanqiXu closed this revision.Jun 6 2022, 7:57 PM