[PATCH] D134475: [Clang] Add support for [[msvc::constexpr]] C++11 attribute

[Richard Dzenis via Phabricator via cfe-commits]

RIscRIpt planned changes to this revision.
RIscRIpt marked 2 inline comments as done.
RIscRIpt added a comment.

> 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:

1. `[[msvc::constexpr]]` can be applied only to function definition or a return statement.
2. A function can be either `constexpr` or `consteval` or `[[msvc::constexpr]]`, but no combination of these.
3. `[[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.

In D134475#4287513 <https://reviews.llvm.org/D134475#4287513>, @erichkeane wrote:

> 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.

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:

1. S2 fails checks of "literal type" in this callstack:
  1. [[ https://github.com/llvm/llvm-project/blob/bc73ef0/clang/include/clang/AST/DeclCXX.h#L1419-L1445 | `clang::CXXRecordDecl::isLiteral` ]]
  2. [[ https://github.com/llvm/llvm-project/blob/e98776a/clang/lib/AST/Type.cpp#L2746 | `clang::Type::isLiteralType` ]]
  3. [[ https://github.com/llvm/llvm-project/blob/a4edc2c/clang/lib/AST/ExprConstant.cpp#L2317-L2320 | `CheckLiteralType` ]]
2. 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:
  1. Copy all checks from `clang::CXXRecordDecl::isLiteral` to `CheckLiteralType` - violates DRY; two places shall be maintained.
  2. Propagate `CanEvalMSConstexpr` down to `clang::CXXRecordDecl::isLiteral`. I don't think it's reasonable for supporting rare vendor-specific attribute.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:3543
+definition or a return statement. It has no effect on function declarations.
+This attribute permits constant evaluation of ``[[msvc::constexpr]]`` functions
+in ``[[msvc::constexpr]] return`` statements inside ``constexpr`` or
----------------
erichkeane wrote:
> 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?
Do you mean, can `[[msvc::constexpr]]` function call `constexpr` and remain const-evaluated? Yes.

I elaborated on semantics in non-inline comment.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7057-7058
+static void handleMSConstexprAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
+  if (auto *FD = dyn_cast<FunctionDecl>(D))
+    FD->setConstexprKind(ConstexprSpecKind::Constexpr);
+  D->addAttr(::new (S.Context) MSConstexprAttr(S.Context, AL));
----------------
RIscRIpt wrote:
> aaron.ballman wrote:
> > 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?
> > 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).
> 
> Indeed. Changed.
> 
> > 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?
> 
> 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.
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).
We have two approaches to address `virtual` case: (1) don't add the attribute to the declaration and emit a warning, or (2) check for `virtual` and `opts.Cplusplus17` during constant evaluation. I am inclining to (2).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134475/new/

https://reviews.llvm.org/D134475