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

constexpr mutex constructor #3824

Merged
merged 7 commits into from Jul 20, 2023
Merged

constexpr mutex constructor #3824

merged 7 commits into from Jul 20, 2023

Conversation

cpplearner
Copy link
Contributor

@cpplearner cpplearner commented Jun 23, 2023 •
edited

Fixes #2285.

Because this might cause problem in complicated (and formally forbidden) scenarios (see discussion in #3770), I chose to make this opt-in via the _USE_CONSTEXPR_MUTEX_CONSTRUCTOR macro. If _USE_CONSTEXPR_MUTEX_CONSTRUCTOR is not defined, mutex will call _Mtx_init_in_situ as before.

For the same reason, I changed stl_critical_section_win7 and stl_condition_variable_win7 back to polymorphic classes, even though their vptrs are no longer accessed. (If _USE_CONSTEXPR_MUTEX_CONSTRUCTOR is defined, no stl_critical_section_win7 object will be created.)

@cpplearner cpplearner requested a review from a team as a code owner June 23, 2023 10:00
@github-actions github-actions bot added this to Initial Review in Code Reviews Jun 23, 2023
@cpplearner cpplearner marked this pull request as draft June 23, 2023 10:27
@cpplearner cpplearner marked this pull request as ready for review June 23, 2023 11:01
@StephanTLavavej

This comment was marked as resolved.

Sign in to view
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Jun 23, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jun 23, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Work In Progress in Code Reviews Jun 23, 2023
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Initial Review in Code Reviews Jul 4, 2023
StephanTLavavej
StephanTLavavej reviewed Jul 7, 2023
View reviewed changes
stl/inc/mutex Outdated Show resolved Hide resolved
stl/inc/xthreads.h Outdated Show resolved Hide resolved
stl/src/primitives.hpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks, this looks great! I pushed extremely minor changes.

Having thought about this extensively during the previous PR, I think that this change is correct and safe, and it's a good simplification too.

@StephanTLavavej StephanTLavavej removed their assignment Jul 7, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Jul 7, 2023
@frederick-vs-ja frederick-vs-ja mentioned this pull request Jul 7, 2023
Merged
@strega-nil-ms strega-nil-ms moved this from Final Review to Ready To Merge in Code Reviews Jul 7, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jul 13, 2023
@StephanTLavavej

This comment was marked as outdated.

Sign in to view
@StephanTLavavej StephanTLavavej removed their assignment Jul 13, 2023
@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Work In Progress in Code Reviews Jul 13, 2023
@StephanTLavavej
Copy link
Member

Unfortunately, I need to pull this from the merge batch. It's causing the internal build to consistently fail with:

D:\msvc\src\vctools\asan\mt\llvm-symbolizer.nativeproj(79,5): error MSB3073:
The command ""D:\msvc\src\ExternalAPIs\cmake\bin\ninja.exe" "bin/llvm-symbolizer.exe"   " exited with code 1.

I don't know what's causing this yet.

@StephanTLavavej
Copy link
Member

We're very close to understanding the root cause - the MSVC-internal build is using fresh STL headers, but a previously installed copy of msvcp140.dll on the system. The fix will also be MSVC-internal; it appears that this PR is working by design (we knew that it would require an updated DLL).

@CaseyCarter
Copy link
Member

We're very close to understanding the root cause - the MSVC-internal build is using fresh STL headers, but a previously installed copy of msvcp140.dll on the system. The fix will also be MSVC-internal; it appears that this PR is working by design (we knew that it would require an updated DLL).

In other words: Thanks for finding a bug in our internal build!

@StephanTLavavej
Copy link
Member

We'll need to ensure that the VCRedist is unlocked for this.

@StephanTLavavej StephanTLavavej added the affects redist Results in changes to separately compiled bits label Jul 14, 2023
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Ready To Merge in Code Reviews Jul 19, 2023
@StephanTLavavej
Copy link
Member

⚠️ Note to self: This requires MSVC-internal changes...

... found by @zacklj89 (:heart_eyes_cat:); I will investigate adding a complete set of DLLs to that list.

@MahmoudGSaleh will notify the appropriate people that 17.8 Preview 2 needs to have an unlocked VCRedist.

@StephanTLavavej StephanTLavavej self-assigned this Jul 20, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej
Copy link
Member

I've had to push an additional commit to avoid a /clr compiler error:

.\test.cpp(479): error C2127: 'obj2': illegal initialization of 'constinit' entity with a non-constant expression

/clr just hates constant initialization so there's nothing we can do about this except skip the test coverage.

@StephanTLavavej StephanTLavavej merged commit 8d18fec into microsoft:main Jul 20, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Jul 20, 2023
@StephanTLavavej
Copy link
Member

Thanks for fixing this unfixable bug!!! 😻 😻 😻

@cpplearner cpplearner deleted the constexpr-mutex branch July 21, 2023 07:05
@achabense achabense mentioned this pull request Aug 16, 2023
Merged
strega-nil added a commit to strega-nil/llvm-project that referenced this pull request Aug 18, 2023
@strega-nil
[llvm-lit] copy CRT/STL DLLs into the output directory
f620022 
It turns out that the STL requires you to place the DLLs into the
directory of the executable; else, msvcp140.dll will be taken from
C:\Windows\System32 (which is unsupported).

This was discovered because of the recent constexpr mutex change;
see [microsoft/STL#3824][]. Especially the fuzzer unit tests fail
completely with the mutex changes.

[microsoft/STL#3824]: microsoft/STL#3824

Differential Revision: https://reviews.llvm.org/D158221
CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Aug 30, 2023
@CaseyCarter
Revert "constexpr mutex constructor (microsoft#3824)"
dc550c4 
This reverts commit 8d18fec.

Conflicts:
* stl/inc/xthreads.h
* stl/src/cond.cpp
* stl/src/mutex.cpp
* stl/src/primitives.hpp
@CaseyCarter CaseyCarter mentioned this pull request Aug 30, 2023
Merged
@CaseyCarter CaseyCarter mentioned this pull request Jan 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frederick-vs-ja frederick-vs-ja frederick-vs-ja left review comments

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

@strega-nil-ms strega-nil-ms strega-nil-ms approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
affects redist Results in changes to separately compiled bits bug Something isn't working
Projects
No open projects
Code Reviews
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

<mutex> : std::mutex::mutex is not constexpr
5 participants
@cpplearner @StephanTLavavej @CaseyCarter @frederick-vs-ja @strega-nil-ms