Page MenuHomePhabricator

Add C++11 attribute msvc::constexpr
Changes PlannedPublic

Authored by RIscRIpt on Sep 22 2022, 1:48 PM.

Details

Diff Detail

Event Timeline

RIscRIpt created this revision.Sep 22 2022, 1:48 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: jdoerfert. · View Herald Transcript
RIscRIpt requested review of this revision.Sep 22 2022, 1:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2022, 1:48 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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.

7054–7055

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?

RIscRIpt updated this revision to Diff 464522.EditedOct 1 2022, 1:56 PM
RIscRIpt retitled this revision from [AST] Add C++11 attribute msvc::constexpr to Add C++11 attribute msvc::constexpr.

Add more tests, don't alter constexprKind of [[msvc::constexpr]] functions - instead change implementation of isConstexpr

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

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.

RIscRIpt planned changes to this revision.Oct 1 2022, 1:56 PM
RIscRIpt added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
4995–4997

Oops, removed.

7054–7055

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.

RIscRIpt marked an inline comment as not done.Oct 1 2022, 1:57 PM
RIscRIpt added a comment.EditedOct 2 2022, 9:00 AM

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:

  1. Neither constructor nor destructor can be constexpr if the class has some virtual base class; this holds for [[msvc::constexpr]] in MSVC 14.33
  2. 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: OK

I am starting to question my patch - does LLVM really need it, if

  1. There's no documentation for [[msvc::constexpr]]
  2. 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.

Pulling a comment out of https://reviews.llvm.org/D133853 that's relevant here:

Now I'm wondering why the attribute exists at all. If it's functionally equivalent to constexpr as a keyword, what are the use cases for the attribute?

It appears that [[msvc::constexpr]] does not make a function constexpr, but if [[msvc::constexpr]] is used in a function definition and in a call to that function, then the annotated function call can be evaluated during constant evaluation: https://godbolt.org/z/3MPTsz6Yn

Apparently this is used to implement constexpr std::construct_at, which needs to call placement operator new, but the latter is not constexpr.

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:

  1. Neither constructor nor destructor can be constexpr if the class has some virtual base class; this holds for [[msvc::constexpr]] in MSVC 14.33
  2. 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

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

  1. There's no documentation for [[msvc::constexpr]]
  2. 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).