Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tracking Issue for constexpr Containers: P0784, P0980, P1004 #1566

Open
mnatsuhara opened this issue Jan 11, 2021 · 11 comments
Open

Tracking Issue for constexpr Containers: P0784, P0980, P1004 #1566

mnatsuhara opened this issue Jan 11, 2021 · 11 comments
Labels
compiler Compiler work involved

Comments

@mnatsuhara
Copy link
Contributor

mnatsuhara commented Jan 11, 2021 •
edited by CaseyCarter
Loading

Last updated 1/11/2022

📄 Relevant Papers

🚧 Status

The various papers involved in this broader feature are in various stages of completion:

🎈 Fixed Clang Bugs

Resolved in Clang 12

  • LLVM-48587: is_constant_evaluated() can incorrectly evaluate to true in the destructor of a non-constexpr variable
    • Fixed and cherry-picked to Clang 12 🟢
  • LLVM-48606: Clang rejects creation of struct with mutable member during constant evaluation - blocking for Clang ❌
    • Believed to be fixed in PR48606 and cherry-picked to Clang 12 here.
  • LLVM-48582: Clang crashes compiling the MSVC standard library
    • Fixed and cherry-picked to Clang 12 🟢

Resolved in Clang 13

  • LLVM-49342: -Wdangling-gsl incorrect warns on return by value of std::string::iterator() -= n - workaround applied ⚠
    • Fixed 🟢
    • Note that workaround is not transition; it will be kept even after bug fix for NRVO optimizations.

🐞 Active C1XX Bugs (MSVC Front-End)

  • DevCom-1487178 - cl permits object reference to object outside of its lifetime during constant evaluation - not blocking 🟢
  • DevCom-1318307 - ICE during constexpr evaluation - not blocking for MSVC, found workaround ⚠️
  • Microsoft VSO-1269037 - Compiler ICE on reference to pointer in constexpr - not blocking for MSVC, have workaround ⚠️
    • Workaround: next = &(*next)->s.p; --> U* temp = *next; next = &temp->s.p;
  • DevCom-1347020 - MSVC accepts mutating of const variables during constant evaluation - not blocking 🟢
  • DevCom-1348647 - cl permits copying into memory before object lifetimes have begun - not blocking 🟢
  • Microsoft VSO-1284799 - constexpr ICE: binding a reference to a temporary emits Assertion failed: Nerrors > 0 - not blocking for MSVC, have workaround ⚠️
    • Workaround: _INLINE_VAR constexpr _Fake_allocator _Fake_alloc{};
  • DevCom-1516290 "C++20 constexpr dynamic allocation doesn't properly understand std::construct_at()"
    • Affects vector<Base*>, no workarounds in STL product/test code.
    • Microsoft VSO-1388781
  • DevCom-1540737 "constexpr std::string (with SSO) bug with user-defined char-like type"
  • DevCom-1634430 "cl incorrectly rejects call to std::{ranges::}construct_at when passed a string literal as argument"

🎉 Fixed C1XX Bugs (MSVC Front-End)

🦗 Active EDG Bugs

  • Microsoft VSO-1273296 - EDG: complaint about _wassert not constexpr - large portions of tests disabled for EDG 😢

🥳 Fixed EDG Bugs

  • DevCom-1318318 - ICE during constant evaluation with /BE - related to std::destroy
  • DevCom-1318321 - cl Incorrectly rejects class with destructor for constant evaluation with /BE
  • Microsoft VSO-1285539 - EDG ICE with constexpr std::string in do_constexpr_std_construct_at
    • Reported upstream (EDGcpfe/23990)
    • Fixed by VS-PR-310478 in 16.10 Preview 2 ✔️
  • Microsoft VSO-1601168 - EDG's __builtin_memcmp emits bogus errors with move-constructed strings and constexpr dynamic allocations
    • Reported upstream (EDGcpfe/25606)
    • Fixed by VS-PR-422901 in 17.4 Preview 3 ✔️

📑 Compiler Approaches to constexpr Dynamic Allocation

(from @cdacamar in #1532) Note that [expr.const]/5 states that an expression E is a core constant expression unless the evaluation of E would [...] would evaluate one of the following:

  • a new-expression ([expr.new]), unless the the selected allocation function is a replaceable global allocation function ([new.delete.single], [new.delete.array]) and the allocated storage is deallocated within the evaluation of E;
  • a delete-expression ([expr.delete]), unless it deallocates a region of storage allocated within the evaluation of E;
  • a call to an instance of std::allocator::allocate ([allocator.members]), unless the allocated storage is deallocated within the evaluation of E;
  • a call to an instance of std::allocator::deallocate ([allocator.members]), unless it deallocates a region of storage allocated within the evaluation of E;
  • [...]

So, the expression ::operator new is not a new expression, it is a function call to a global operator new() which is not a constexpr function.

  • Clang has chosen to allow ::operator new in a constexpr function if it is reached transitively from an evaluation of std::allocator::allocate.
  • MSVC has chosen to do "fairy magic" (🧚) when it sees std::allocator::allocate, std::allocator::deallocate, but does not allow ::operator new.
  • EDG (Intellisense) appears to allow ::operator new, but we choose to have the EDG logical paths mirror those for MSVC so they work in tandem.

Development notes, preserved for history

👣 Next Steps (as of Jan 2021)

  • After getting the bug fixes in 16.9 Preview 3, @miscco discovered several new bugs in MSVC FE and BE and EDG (see Active C1XX Bugs, Active MSVC Back-End Bugs, and Active EDG Bugs).
  • We are currently blocked for Clang on Bug 48606: Clang rejects creation of struct with mutable member during constant evaluation - this bug report has not yet been validated by a Clang developer, though @cdacamar has confirmed that this seems to be a Clang bug.
    • This bug is blocking in our iterator debug machinery, see further discussion here. In particular, we need to make some members of our debug iterators mutable, but Clang currently does not allow this due to this bug.
    • More background on the use case:
      • Because global vector variables can exist, we need to enable situations where a variable is constant initialized (so std::is_constant_evaluated() evaluates to true in the constructor), but not necessarily constant evaluated (so std::is_constant_evaluated() evaluates to false in subsequent calls to other member functions). You can see the discussion here.
      • This means that we cannot simply disable all iterator debugging machinery if std::is_constant_evaluated() is true in these containers in general. Hopefully, a fix for Bug 48606: Clang rejects creation of struct with mutable member during constant evaluation can be forthcoming soon, in which case we will be able to move forward with a Clang-facing implementation.
      • If a Clang dev determines that the reported bug is not valid and their behavior is Standards-conforming, we should (1) fix the Standard to allow mutable in this context, or (2) investigate whether requested an additional built-in to detect whether we are in a "constant initialization" context vs. "constant evaluation" context could also help us mitigate the debug iterators issue.
      • NOTE: More discussion and clarification here. Any global string variable will end up using a dynamic initializer as constant initializers will be off-limits. This is because we don't have non-transient allocations and all constant initializers for string now do constant-time allocation (since SSO is turned off if is_constant_evaluated()). So, the motivating argument for keeping the debug iterator proxy machinery active during constant evaluation does not apply to string, but the same iterator logic is used by many of the containers. Only disabling the proxy iterator things for string would require duplication of a lot of code. Given that there are plans to update our debug iterator machinery in vNext anyway, the effort required to disable debug iterators during constant-time for string does not seem worthwhile.

🖥️ Testing (as of Jan 2021)

  • Since there have been a number of compiler issues discovered through the implementation of these features, we are going to enable testing the features' implementation in the internal test harness (via #ifdef MSVC_INTERNAL_TESTING) so as to discover and fix compiler issues more quickly.
    • As we are able to consume the compiler fixes in public previews, we will be able to enable full testing and remove the dependency on MSVC_INTERNAL_TESTING.
  • Current status:
    • P1004R2_constexpr_vector tests are passing internally with the updated compiler -- Enable constexpr vector internal tests #1690 enables the tests internally. When we consume the updated compiler with test fixes, we will be able to remove the defined(MSVC_INTERNAL_TESTING) entirely.
    • P1004R2_constexpr_vector_bool tests have all compiler bugs fixed internally, but there is a pre-existing issue in the debug iterator machinery that needs investigation before we can enable these tests.
    • P0980R1_constexpr_string tests have all compiler bugs fixed internally, and will already have defined(MSVC_INTERNAL_TESTING) throughout test code to enable these tests internally as soon as we merge.
@mnatsuhara mnatsuhara added cxx23 C++23 feature blocked on 16.9p3 compiler Compiler work involved and removed cxx23 C++23 feature labels Jan 11, 2021
@miscco
Copy link
Contributor

miscco commented Jan 11, 2021

Thanks for the amazing writeup!

@cdacamar
Copy link
Contributor

MSVC has chosen to do "fairy magic" (🧚) when it sees std::allocator::allocate, std::allocator::deallocate, but does not allow ::operator new.

Fairy magic might be appropriate :). What the compiler actually does is elide calls to those functions (along with std::construct_at, std::destroy_at and friends) so that their body does not matter--since it will almost certainly do non-constexpr friendly things such as calls to ::operator new.

@mnatsuhara mnatsuhara mentioned this issue Jan 12, 2021
Merged
@StephanTLavavej StephanTLavavej pinned this issue Feb 4, 2021
@AraHaan
Copy link

AraHaan commented Feb 9, 2021 •
edited
Loading

I mean why don't we use constexpr in the global new and delete operators by default too?

@Blackh0leZ
Copy link

Great bookkeeping for feature implementation quality, Miya!

@mnatsuhara
Copy link
Contributor Author

I mean why don't we use constexpr in the global new and delete operators by default too?

This was addressed in the STL discord channel as well, but for the sake of completeness I will echo the reasoning here. Allowing constexpr new/delete is not technically permitted by the Standard. To quote @cdacamar, "One possible reason for not making the global new constexpr is because it does not establish any kind of object lifetime, it just allocates bytes. The abstract machine in C++ is all about working with objects which have lifetime and blobs of bytes do not really fit into that model very well."

Unless new and delete are made constexpr in the Standard, we will not be pursuing that approach.

@miscco

This comment was marked as resolved.

Sign in to view
@StephanTLavavej StephanTLavavej mentioned this issue Feb 25, 2021
Merged
@AraHaan

This comment was marked as resolved.

Sign in to view
@mnatsuhara mnatsuhara unpinned this issue Apr 7, 2021
@miscco miscco mentioned this issue Aug 26, 2021
Merged
@miscco

This comment was marked as resolved.

Sign in to view
@miscco

This comment was marked as resolved.

Sign in to view
@frederick-vs-ja

This comment was marked as resolved.

Sign in to view
@CaseyCarter
Copy link
Member

Someone with access to the Microsoft-internal bug database needs to make a pass over the OP and ensure everything is up-to-date. The "Active EDG bugs" all seem to be closed and resolved, for example. If we can remove any and all workarounds, we can close this issue.

@StephanTLavavej StephanTLavavej mentioned this issue May 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler Compiler work involved
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@CaseyCarter @miscco @cdacamar @AraHaan @frederick-vs-ja @Blackh0leZ @mnatsuhara