Details
- Reviewers
aaron.ballman erichkeane rsmith
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 188255 Build 284414: arc lint + arc unit
Event Timeline
Thank you for this! I think it's heading in the right direction, but we're missing some significant test coverage for it. I had some suggestions for specific things we should be thinking about, but another useful test would be to modify an existing constexpr test to add a RUN line enabling ms extensions, and use a macro to switch between constexpr and [[msvc::constexpr]] based on those RUN lines. Basically, ensure that [[msvc::constexpr]] gives the same behavior (both in terms of evaluation and in terms of semantic checking) as constexpr. WDYT?
| clang/lib/Sema/SemaDeclAttr.cpp | ||
|---|---|---|
| 4995–4997 | This looks incorrect to me -- constexpr isn't a calling convention, so I think this code should be removed. | |
| 7057–7058 | We can use cast here because we know the declaration must have already been determined to be a function (from the subjects list in Attr.td). That said, I think there's more semantic checking we want to do here. For example, can you use this attribute on a virtual function? What about a function already marked constexpr? Even more scary, what about a function marked consteval? Also, I presume a constexpr function should be able to call an [[msvc::constexpr]] function and vice versa? | |
Add more tests, don't alter constexprKind of [[msvc::constexpr]] functions - instead change implementation of isConstexpr
Regarding existing tests, I was able to find only clang/test/AST/Interp/functions.cpp which uses constexpr only with functions - I added RUN lines there. We cannot do that with most of tests, because -Dconstexpr=[[msvc::constexpr]] would break code in case there are constexpr variables.
| clang/lib/Sema/SemaDeclAttr.cpp | ||
|---|---|---|
| 4995–4997 | Oops, removed. | |
| 7057–7058 |
Indeed. Changed.
Good questions. By running strings c1xx.dll I was able to find only the following diagnostic messages regarding [[msvc::constexpr]]: [[msvc::constexpr]] may only be applied to statements and functions" [[msvc::constexpr]] cannot be applied to a 'constexpr' or 'consteval' function" I've made a similar check here and added relevant test cases in msvc-attrs-invalid.cpp. But in order to make it possible, I had to change FunctionDecl::isConstexpr(), please check new changes. TODO: I think, I'll need to read more about constexpr for functions in standard (and LLVM code), to add relevant restrictions here. | |
TODO: I think, I'll need to read more about constexpr for functions in standard (and LLVM code), to add relevant restrictions here.
In the standard I was able to find the following paragraphs about constexpr:
- Neither constructor nor destructor can be constexpr if the class has some virtual base class; this holds for [[msvc::constexpr]] in MSVC 14.33
- constexpr is not a part of function signature, so same non-constexpr function cannot be defined. The same holds for C++11 attributes, and [[msvc::constexpr]] in MSVC 14.33
I will add two relevant tests in msvc-attrs-invalid.cpp
Regarding LLVM code, I made the following observations:
- FunctionDecl::isConstexprSpecified() is mostly used for printing code, in other words, to check that the function is constexpr (and not consteval).
- FunctionDecl::getConstexprKind() is used by:
- ASTImporter.cpp - seems that, it should take care of attributes independently from the getConstexprKind()
- ASTWriterDecl.cpp - seems that, it should take care of attributes independently from the getConstexprKind()
- SemaDecl.cpp / SemaDeclCXX.cpp - checks that re-declaration was not performed with different constexpr specifier. MSVC and clang with current implementation allows re-declaration with different attributes (there are tests with f2-f5 in msvc-attrs.cpp)
- SemaDeclCXX - enables C++ constexpr constructor inheritance - problem - my current implementation supports it, whereas MSVC doesn't:
// Check '[[msvc::constexpr]]' is not taken into account during constructor inheritance
struct s2 {
[[msvc::constexpr]] s2() {}
constexpr operator bool() { return false; };
};
struct s3 : s2 {
constexpr operator bool() { return true; };
};
static_assert(s3()); // MSVC: C2131: expression did not evaluate to a constant
static_assert(s3()); // clang with my patch: OK- SemaTemplate.cpp - enables C++ template re-specialization with different constexpr specifier, not a problem, because we are allowed to do the same with attributes (don't we?)
- SemaTemplateInstantiateDecl.cpp (TemplateDeclInstantiator) - enables C++ template instantiation with same constexpr specifier
template<typename T>
struct s1 {
[[msvc::constexpr]] s1() {}
constexpr operator bool() { return true; };
};
static_assert(s1<void>()); // MSVC: C2131: expression did not evaluate to a constant
static_assert(s1<void>()); // clang with my patch: OK- TreeTransform.h - code is related to lambdas - out-of-scope of the current patch.
Regarding MSVC:C2131 I found one more problem:
[[msvc::constexpr]] int C() { return 0; }
constexpr int V = C(); // MSVC: C2131: expression did not evaluate to a constant
constexpr int W = C(); // clang with my patch: OKI am starting to question my patch - does LLVM really need it, if
- There's no documentation for [[msvc::constexpr]]
- The trivial implementation makes it an alias, but the semantic behavior is different - we could fix that, but there are lots of things to check
@aaron.ballman WDYT?
P.S. I'd rather abandon it, than trying to match behavior of MSVC.
Good, thank you!
Regarding LLVM code, I made the following observations:
- FunctionDecl::isConstexprSpecified() is mostly used for printing code, in other words, to check that the function is constexpr (and not consteval).
Sort of. A templated function marked with constexpr remains a constexpr function even if it can't be executed at compile time, so isConstexprSpecified() is really telling you "did the user write constexpr on this declaration?" instead of "is this function really constexpr?" (http://eel.is/c++draft/dcl.constexpr#4)
- FunctionDecl::getConstexprKind() is used by:
- ASTImporter.cpp - seems that, it should take care of attributes independently from the getConstexprKind()
- ASTWriterDecl.cpp - seems that, it should take care of attributes independently from the getConstexprKind()
- SemaDecl.cpp / SemaDeclCXX.cpp - checks that re-declaration was not performed with different constexpr specifier. MSVC and clang with current implementation allows re-declaration with different attributes (there are tests with f2-f5 in msvc-attrs.cpp)
- SemaDeclCXX - enables C++ constexpr constructor inheritance - problem - my current implementation supports it, whereas MSVC doesn't:
// Check '[[msvc::constexpr]]' is not taken into account during constructor inheritance struct s2 { [[msvc::constexpr]] s2() {} constexpr operator bool() { return false; }; }; struct s3 : s2 { constexpr operator bool() { return true; }; }; static_assert(s3()); // MSVC: C2131: expression did not evaluate to a constant static_assert(s3()); // clang with my patch: OK
Hmmm, that's interesting! I wonder if that's intentional behavior for MSVC or a bug. I'm guessing there are no open issues on Microsoft's bug tracker because this isn't a documented feature? Might be worth filing one to see what they think though. (Might not be worth it either, see below.)
- SemaTemplate.cpp - enables C++ template re-specialization with different constexpr specifier, not a problem, because we are allowed to do the same with attributes (don't we?)
Shake a magic 8-ball, unfortunately. There's a core issue open currently about template specializations and attributes: http://wg21.link/cwg2604
- SemaTemplateInstantiateDecl.cpp (TemplateDeclInstantiator) - enables C++ template instantiation with same constexpr specifier
template<typename T> struct s1 { [[msvc::constexpr]] s1() {} constexpr operator bool() { return true; }; }; static_assert(s1<void>()); // MSVC: C2131: expression did not evaluate to a constant static_assert(s1<void>()); // clang with my patch: OK
Can you do: constexpr s1<void> s; in MSVC or does that also fail? (e.g., is the issue with the constructor or with the conversion operator?)
- TreeTransform.h - code is related to lambdas - out-of-scope of the current patch.
Regarding MSVC:C2131 I found one more problem:
[[msvc::constexpr]] int C() { return 0; } constexpr int V = C(); // MSVC: C2131: expression did not evaluate to a constant constexpr int W = C(); // clang with my patch: OK
What the heck, that's a surprise! I would have expected that to work the same in Clang and MSVC (accepted by both).
I am starting to question my patch - does LLVM really need it, if
- There's no documentation for [[msvc::constexpr]]
- The trivial implementation makes it an alias, but the semantic behavior is different - we could fix that, but there are lots of things to check
@aaron.ballman WDYT?
P.S. I'd rather abandon it, than trying to match behavior of MSVC.
If you don't want to work on the feature, that's totally fine (you're under no obligation to finish this if it turns out to be more than you wanted to take on)! I think we're going to need to support this attribute at some point because <vcruntime.h> has #define _MSVC_CONSTEXPR [[msvc::constexpr]] and that macro is used by the STL headers. But the usage I am seeing there is even more interesting than what we've discovered so far. From <memory>:
constexpr _Ty* operator()(_Ty* _Location, _Types&&... _Args) const
noexcept(noexcept(::new (const_cast<void*>(static_cast<const volatile void*>(_Location)))
_Ty(_STD forward<_Types>(_Args)...))) /* strengthened */ {
// clang-format on
_MSVC_CONSTEXPR return ::new (const_cast<void*>(static_cast<const volatile void*>(_Location)))
_Ty(_STD forward<_Types>(_Args)...);
}It's using the attribute on a return statement, not on a declaration! So I suspect there's some more reverse engineering needed unless Microsoft wanted to document their extension (which might be the bug we want to file instead of "is it intentional that this works in this weird way?" bug reports).
I am sorry for protracting implementation. I am still interested to finish this (if no volunteer takes over sooner).
They did answer! Thank you Cody! For sake of backup, I'll re-post Cody Miller's comment here
In Microsoft Developer Community / msvc::constexpr-specification, Cody Miller wrote:
Hello!We added [[msvc::constexpr]] to support std::construct_at and std::ranges::construct_at. These functions are used in lieu of placement new in constexpr contexts to support the forms of dynamic memory allocation that C++20 supports.
We originally implemented this by intercepting the names of the invoked functions and implemented them within the compiler. We (the compiler team) ultimately weren’t satisfied with this approach because it added complexity to the frontend and led to some surprising behavior (such as users implementing their own std::construct_at (technically invoking undefined-behavior, I think) and seeing behavior that may have differed from their implementation).
We added the attribute to allow limited contexts to invoke placement new during constexpr evaluation, which allowed the compiler to handle the standard library’s implementation of the two special functions mentioned above.
The semantics are (from memory on a Sunday evening, apologies for unclear or forgotten explanations):
- A function may be either [[msvc::constexpr]], constexpr, or neither, but no combination of these is allowed.
- A function annotated with [[msvc::constexpr]] is unofficially called an “extended constexpr” function and may call other constexpr or extended constexpr functions.
- An extended constexpr function has the same restrictions as a constexpr function, except when otherwise noted here.
- A return statement may be annotated with [[msvc::constexpr]] to allow its containing constexpr function to call an extended constexpr function.
- An extended constexpr function can call other extended constexpr functions without a statement-level annotation.
- A constexpr function may use placement new within a return statement annotated with [[msvc::constexpr]].
We aimed to add a general mechanism to support this feature, but we only are officially supporting it for use in std::construct_at and std::ranges::construct_at at the moment – feel free to file bugs if you find issues through experimentation, though!
The restriction to the return statement is reflective of us wanting to keep the scope of the attribute small.
Here’s a godbolt link that hopefully clearly captures the above.
We use the annotation (behind the _MSVC_CONSTEXPR macro) in our STL implementation in two places.
Hope this explanation helps! Let me know if anything is unclear.
Currently I am lingered to decide how to implement checks for constant evaluation in clang/lib/AST/ExprConstant.cpp.
At the moment I have two ideas:
Idea 1: Using helper RAII classes like BlockScopeRAII keep track of AttributedStmt and ReturnStmt in EvalInfo and/or CallStackFrame; then in CheckConstexprFunction check whether above rules are followed. However I don't see that EvalInfo or CallStackFrame allow storing references to arbitrary statements, and extending these classes only to support [[msvc::constexpr]] does not sound like a reasonable idea.
Idea 2: Pass Expr* CallerExpr to CheckConstexprFunction, and then traverse parents of CallerExpr to check that above rules are followed. This would require implementing a simple ParentVisitor class which would use ParentMapContext. I'm not completely sure about this idea, because I see that ParentMapContext is used seldom and its docs mention about "opportunities for optimization" (sounds like performance problems).
By the way, it feels weird that we have to implement complex checks (for attributed return statement) only to limit the possibilities of constant evaluation; on the other hand without these checks clang won't match MSVC implementation.
P.S. Based on Cody's comment and experiments in Compiler Explorer, I made a summary with three rules; however for some reason, in the evening, I cannot replicate them anymore; once I have time and figure it out, I'll post an update. Note: STL has [[msvc::constexpr]] void* operator new(size_t, void*).
I don't have a good idea, I don't think any sort of RAII object is the right way (since it will be wrong on child calls/etc), and the ParentMap is definitely not the right way. Perhaps just a flag for CheckConstexprFunction? I've not spent too much time in this section of the compiler, so hopefully someone else can come along and help. I suspect it is a property of CallStackFrame? But that already contains a link to the FucntionDecl, right? As far as the 'on return statement', I think the bit on CallStackFrame/EvalInfo is probably what is necessary.
A proper implementation
But that already contains a link to the FucntionDecl, right?
Yes, but we need to know current context (whether we're in [[msvc::constexpr]] return statement).
As far as the 'on return statement', I think the bit on CallStackFrame/EvalInfo is probably what is necessary.
That's what I ended-up doing. This was the most reasonable approach I could come-up with.
I added a property to CallStackFrame, because it's a property of current function: whether we are in valid context which permits evaluation of [[msvc::constexpr]]. Additionally I added MSConstexprContextRAII which is used in case Stmt::AttributedStmtClass: to alter the property.
Still suspicious about this/whether it models the MSVC implementation correctly, but no real implementation concerns. I REALLY want us to document the attribute extensively however.
| clang/include/clang/Basic/AttrDocs.td | ||
|---|---|---|
| 3486 | Does it prohibit the inverse? I think this documentation overall needs a much better description of what the semantics are here, particularly anything that you found in experimentation on the MS implementation. | |
Does it prohibit the inverse? I think this documentation overall needs a much better description of what the semantics are here, particularly anything that you found in experimentation on the MS implementation.
Do you mean, can [[msvc::constexpr]] function call constexpr and remain const-evaluated? Yes.
From my experiments all Cody's points are valid, except for "An extended constexpr function can call other extended constexpr functions without a statement-level annotation".
I tried to make doc notes as compact as possible. Recapping his points, which were confirmed by tests:
- [[msvc::constexpr]] can be applied only to function definition or a return statement.
- A function can be either constexpr or consteval or [[msvc::constexpr]], but no combination of these.
- [[msvc::constexpr]] function is treated the same way as constexpr function if it is evaluated in terms of [[msvc::constexpr]] return statement, otherwise it's a regular function.
By function I mean anything function-like: functions, member-functions, constructors, destructors, operators.
Thanks to your concern I decided to test each point of [dcl.constexpr] : 9.2.6 The constexpr and consteval specifiers in Compiler Explorer: https://godbolt.org/z/qj3fnK5aW I am going to update tests accordingly.
Regarding on absolute matching MSVC implementation, I am not sure we will be able to achieve it with reasonable changes. I am skeptical, because I discovered the following test case:
struct S2 {
[[msvc::constexpr]] S2() {}
[[msvc::constexpr]] bool value() { return true; }
static constexpr bool check() { [[msvc::constexpr]] return S2{}.value(); }
};
static_assert(S2::check());It's a valid code for MSVC; nothing special https://godbolt.org/z/znnaonEhM
However supporting this code seems to be difficult in Clang:
- S2 fails checks of "literal type" in this callstack:
- [[ https://github.com/llvm/llvm-project/blob/bc73ef0/clang/include/clang/AST/DeclCXX.h#L1419-L1445 | clang::CXXRecordDecl::isLiteral ]]
- [[ https://github.com/llvm/llvm-project/blob/e98776a/clang/lib/AST/Type.cpp#L2746 | clang::Type::isLiteralType ]]
- [[ https://github.com/llvm/llvm-project/blob/a4edc2c/clang/lib/AST/ExprConstant.cpp#L2317-L2320 | CheckLiteralType ]]
- We have information about CanEvalMSConstexpr only in CheckLiteralType. So, currently, I don't see how we can reasonably check whether S2 is a valid literal type in context of [[msvc::constexpr]]. Two obvious ugly solutions are:
- Copy all checks from clang::CXXRecordDecl::isLiteral to CheckLiteralType - violates DRY; two places shall be maintained.
- Propagate CanEvalMSConstexpr down to clang::CXXRecordDecl::isLiteral. I don't think it's reasonable for supporting rare vendor-specific attribute.
| clang/include/clang/Basic/AttrDocs.td | ||
|---|---|---|
| 3486 |
Do you mean, can [[msvc::constexpr]] function call constexpr and remain const-evaluated? Yes. I elaborated on semantics in non-inline comment. | |
| clang/lib/Sema/SemaDeclAttr.cpp | ||
| 7057–7058 | Added a check that [[msvc::constexpr]] function has no constexpr or consteval specifiers. A member function can be [[msvc::constexpr]] the same way as constexpr. Regarding virtual - MSVC does not complain about [[msvc::constexpr]], but it fails during constant evaluation (in case of C++17 and earlier). | |
Added diagnostics for [[msvc::constexpr]] virtual
Regarding [[msvc::constexpr]] constructors, unfortunatelly I cannot find a reasonable way to support it.
During development I found out about Clang's experimental Constant Interpreter (-fexperimental-new-constant-interpreter),
I suppose it would be much easier to implement complete support for [[msvc::constexpr]] there.
I am not aware about policy regarding constant evaulation/interpreter:
do you approve new constant evaluation features in the current (AST-based) approach without extending the experimental one?
Because I'd rather tacke with experimental Constant Interpreter in a follow-up (if I ever find time for this).
Currently the experimental evaluator isn't required to get patches committed. It is mostly the labor-of-love of @tbaeder and is still in the beginning stages of development.
Bump
Renamed clang/test/AST/msvc-* to ms-*
Tried to improve doc note.
Rebased onto one of stable commits from main.
| clang/include/clang/Basic/AttrDocs.td | ||
|---|---|---|
| 3486 | Tried to make doc notes more clear. | |
| clang/lib/AST/ExprConstant.cpp | ||
|---|---|---|
| 5615–5627 ↗ | (On Diff #540803) | Given that the intended use case is for usage behind the scenes in the standard library, I don't think we should be changing our diagnostic output at all here. If the library, as an implementation detail, marks a non-constexpr function as [[msvc::constexpr]], we shouldn't tell the user to add [[msvc::constexpr]] to their code to allow it to be called, after all, the annotation is an implementation detail of the MS standard library. |
| clang/lib/AST/ExprConstant.cpp | ||
|---|---|---|
| 9591–9594 ↗ | (On Diff #540803) | Do we really need this change? Was our existing check of whether the caller is in namespace std not sufficient for MS' standard library? I'd strongly prefer not to have a documented, user-visible attribute that gives permission to use placement new directly. |
Thank you for your quick response.
Given that the intended use case is for usage behind the scenes in the standard library...
I'd strongly prefer not to have a documented, user-visible attribute that gives permission to use placement new directly.
I understand your concerns, and I cannot disagree. My reasoning behind the current implementation is to allow clang with -fms-extensions to parse and compile the same code with the [[msvc::constexpr]] attribute as MSVC can. Given that any user of the MSVC compiler can use this attribute and compile their code with it, it seems to me that clang-cl should also have this capability. However, we don't have any guarantees that Microsoft won't modify anything related to this attribute. I am not aware of the policies the Clang project has in place for such cases. Regardless, I will adjust the patch to comply as necessary.
TODO for my next patch: pre-define _MSVC_CONSTEXPR_ATTRIBUTE
| clang/include/clang/Basic/AttrDocs.td | ||
|---|---|---|
| 3494–3496 | As per latest comments, maybe I should remove this note? | |
| clang/lib/AST/ExprConstant.cpp | ||
| 5615–5627 ↗ | (On Diff #540803) | Sounds fair, I will adjust this code if I'm unable to convince you with my new comments. |
| 9591–9594 ↗ | (On Diff #540803) | Yes, STL's operator new is defined in global namespace in vcruntime_new.h (and all includes of this file are made from global namespace). |
| clang/lib/AST/ExprConstant.cpp | ||
|---|---|---|
| 9591–9594 ↗ | (On Diff #540803) | The existing code is checking whether the caller of operator new (eg, std::construct_at) is in namespace std, not whether the operator new itself is. Does MSVC's construct_at need this? It might if it uses a placement new indirectly via a function in a different namespace, but it seems likely to me that it doesn't. |
Addressed review comments and rebased onto main.
I have limited support of this attribute to -fms-compatibility-version 19.33+ (Visual Studio version, when this attribute first appeared).
Added definition of _MSVC_CONSTEXPR_ATTRIBUTE.
I am not sure whether and where to mention about adding support of this attribute in release notes.
Additionally, bumped -fms-compatibility-version to 19.33 in a separate commit. Although, I am not aware of all the consequences of this change.
Changed test files their names and locations.
| clang/include/clang/Basic/AttrDocs.td | ||
|---|---|---|
| 3494–3496 | Removed. | |
| clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
| 2817 ↗ | (On Diff #540803) |
How about other places? Theoretically I could re-use existing warnings/errors with diagnostics saying constexpr instead of [[msvc::constexpr]]. |
| clang/lib/AST/ExprConstant.cpp | ||
| 5615–5627 ↗ | (On Diff #540803) | Removed, and made the check as a single if statement. |
| 9591–9594 ↗ | (On Diff #540803) | Yes, sorry, my bad. This change is not needed, unless we want to support this attribute in user-code, which I guess we don't want to. Removed. |
Rebase onto main. Run lit clang/test. Bump.
Any chance we continue the review? I know it looked stale for a while I couldn't address review comments, but now I have time to work at this patch.
While I hate that we'll lose all of the Phabriator history on this one, I don't see being able to complete this review in the next two weeks before Phab closes down. The C++ Standards meeting is in 2 weeks, which makes it particularly difficult.
If you can properly rebase, submit this to github, and include a summary of the Phab discussion in a way that easy enough to catch back up on, I'll do a deep dive into this in the next week or 3.
Sorry for the delay, this fell off my radar somehow.
| clang/test/SemaCXX/ms-constexpr.cpp | ||
|---|---|---|
| 30 ↗ | (On Diff #557270) | I see this is still TODO, part of the reason I have been putting off review, I thought this patch wasn't ready for review. Whats going on here? When is this intended to be implemented? |
Youch... I'm sorry this hasn't managed to get up to the top of my queue yet! I agree with Erich, as unfortunate as it is not to complete the review in Phab, we probably should put it up on GitHub so we don't run into the deadline for putting phab into read-only mode. I should hopefully be able to dedicate time to review to this in the next several weeks.
Sure, I'll try to write a summary of comments, and submit it to GitHub.
No problems regarding delays, I couldn't work at this patch in the beginning of this year, so it was delayed for so long.
| clang/test/SemaCXX/ms-constexpr.cpp | ||
|---|---|---|
| 30 ↗ | (On Diff #557270) | Unfortunately, I don't think it's feasible with reasonable changes. I explained it in comment https://reviews.llvm.org/D134475#4288759 - search for "literal type". I think, I could create a follow up issue on GitHub, and make a reference to the issue here. |
As per agreement, migrating to Github: https://github.com/llvm/llvm-project/pull/71300
Does it prohibit the inverse? I think this documentation overall needs a much better description of what the semantics are here, particularly anything that you found in experimentation on the MS implementation.