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.

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

Cleanup _Execute_once() #4087

Merged
merged 13 commits into from
Oct 25, 2023
Merged

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Oct 12, 2023

  • Revert XFG: made _Execute_once XFG-compatible. #1318, _Execute_once bug fix. #1356, and Move structs into unnamed namespaces #1398 (xonce.cpp only) while preserving Remove XP (and Server 2003) support from STL #1194's change to directly call InitOnceExecuteOnce().
    • XFG is being removed from the toolset (see internal VSO-1726733, MSVC-PR-462377). Some things like ASan still need to handle XFG hashes, but things like the STL no longer need unusual contortions. I checked with @amyw-msft and she said this should be okay to remove.
  • Drop _Execute_once() usage in ppltasks.cpp, and as an associated change, simplify dllmain.cpp.
    • See below.
  • Use call_once() instead of _Execute_once() in excptptr.cpp.
    • After Formatting thread::id, stacktrace_entry, and basic_stacktrace #3861, they're both declared by xcall_once.h, so we don't need to use the internal _Execute_once() (with a worse interface) anymore.
    • We're using namespace std; so we don't need qualification.
    • Because call_once() supports stateful lambdas, we can simply capture _Storage by reference.
    • We're in stl/src so we don't really need to worry about True Placement New being hijacked, but let's static_cast<void*> to preserve the original code and make it clear that we are indeed invoking True Placement New.
  • Remove declarations from xcall_once.h.
    • They were guarded by _CRTBLD after Various cleanups #3935, so this doesn't affect users.
    • The definition of _Execute_once() exactly repeated the declaration, except for extern "C++" which is fine to drop because stl/src is always built classically.
  • Update comment now that _Execute_once() is unused.
    • Also drop pointless "_Execute_once function" comment.

The code removal in ppltasks.cpp looks like I'm just thoughtlessly annihilating a bunch of weird logic, but I actually performed it carefully step-by-step. This code was _CRT_APP-only, for "async causality tracing". As far as I can tell (looking up TFS changesets 923210 on 2013-04-02 and 1175213 on 2014-02-07, as this code was introduced and moved around), this was implemented in the hopes of improving debugging, back when ppltasks was envisioned as a top-level feature and the new major model for concurrency, instead of the minor implementation detail of <future> that it eventually ended up as. The async causality logging mechanism never existed for Desktop, and was conditionally available for App (it depended on OS version). That is, it's not necessary for any of <future>'s work, and this was so undocumented, I strongly doubt that anyone aside from the original developer in 2013-2014 ever used it (I certainly haven't).

Attempting to use real std::call_once() here ran into problems, because App didn't have access to the necessary Windows API functions that power it instead of _Execute_once(). In theory that could have been solvable with enough MSBuild effort, but I think that we can simply remove this code, and I'm willing to take the risk and bear any potential servicing burden if I'm wrong.

This doesn't affect bincompat because it's internal to stl/src and There's Only One STL (stl/src is always consistent with itself). I also verified that the exports of binaries\x86chk\bin\i386\app\msvcp140d_app.dll are unchanged.

The steps that I followed started by considering what would happen if the causality APIs weren't detected as present, and then performing logical transformations after that.

  • Step 1: Suppose we can never get the causality APIs.
    • In AsyncCausalityTracer, m_causalityAPIs and m_isSupported are private data members, initialized to nullptr and false. Let's pretend that GetActivationFactory() fails. In that case, the data members remain nullptr and false forever. Accordingly, we can hardcode the return values of get() and isCausalitySupported(). Also, release() becomes a no-op.
  • Step 2: There are no causality static factories to cleanup.
    • Now that asyncCausalityTracer.release() is a no-op, __crtCleanupCausalityStaticFactories() does nothing. Note that it wasn't exported - it was used only for cross-TU interaction between ppltasks.cpp and dllmain.cpp in stl/src, so we can remove this function outright. Then, dllmain.cpp becomes a stub.
  • Step 3: asyncCausalityTracer.isCausalitySupported() is always false now.
    • Accordingly, we can drop all of these branches. Note that this drops _M_scheduled = true;.
  • Step 4: _M_scheduled is always false now.
    • In <ppltasks.h>, _TaskEventLogger's constructor assigned _M_scheduled = false;, and we previously removed the only way for it to be set to true. Accordingly, we can drop this branch.
  • Step 5: Remove unused code.
    • From bottom to top:
    • const GUID PPLTaskCausalityPlatformID wasn't exported. It had internal linkage, so we can definitely drop it.
    • Nothing mentions the AsyncCausalityTracer type or the asyncCausalityTracer global variable anymore.
    • Finally, we don't need the using-directives here.
  • Step 6: Unify Vulcan and Romulus.
    • The app implementation of _TaskEventLogger is now identical to the desktop implementation (after we drop the _IsContinuation parameter name). Unify them.
  • Pre-merge with make extern "C" functions explicitly noexcept #4106 by marking extern "C" DllMain() as noexcept.
    • Also drop the commented-out parameter names to avoid wrapping; they aren't useful when the function does nothing.

…g GH 1194's change to directly call InitOnceExecuteOnce().
After GH 3861, they're both declared by xcall_once.h, so we don't need to use the internal _Execute_once() (with a worse interface) anymore.

Drop the "TRANSITION, ABI" comment which referred to _Execute_once().

Qualify std::call_once() like std::once_flag above. (_Execute_once() lives in namespace std and was being found through ADL!)

Since call_once() handles stateful lambdas, we can directly capture `this` and use it.

Finally, call_once() doesn't care about the return value, so we don't need to `return 1;`.
We're `using namespace std;` so we don't need qualification.

Because call_once() supports stateful lambdas, we can simply capture `_Storage` by reference.

We're in stl/src so we don't really need to worry about True Placement New being hijacked, but let's `static_cast<void*>` to preserve the original code and make it clear that we are indeed invoking True Placement New.
They were guarded by `_CRTBLD` after GH 3935, so this doesn't affect users.

The definition of `_Execute_once()` exactly repeated the declaration, except for `extern "C++"` which is fine to drop because stl/src is always built classically.
Also drop pointless "_Execute_once function" comment.
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Oct 12, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner October 12, 2023 23:15
@github-actions github-actions bot added this to Initial Review in Code Reviews Oct 12, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Oct 12, 2023
@AlexGuteniev
Copy link
Contributor

AlexGuteniev commented Oct 13, 2023

What is XFG and why is it removed?

@StephanTLavavej
Copy link
Member Author

According to my limited understanding, it was a variant of the control flow guard feature, but was abandoned because it proved to be unworkable in practice.

@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Oct 18, 2023
@CaseyCarter
Copy link
Member

  • Also drop pointless "_Execute_once function" comment.

🔥🔥🔥 Yes, YES! 🔥🔥🔥

@StephanTLavavej StephanTLavavej self-assigned this Oct 19, 2023
@StephanTLavavej

This comment was marked as outdated.

@StephanTLavavej StephanTLavavej moved this from Ready To Merge to Work In Progress in Code Reviews Oct 20, 2023
@StephanTLavavej

This comment was marked as resolved.

In `AsyncCausalityTracer`, `m_causalityAPIs` and `m_isSupported` are private data members, initialized to `nullptr` and `false`. Let's pretend that `GetActivationFactory()` fails. In that case, the data members remain `nullptr` and `false` forever. Accordingly, we can hardcode the return values of `get()` and `isCausalitySupported()`. Also, `release()` becomes a no-op.
Now that `asyncCausalityTracer.release()` is a no-op, `__crtCleanupCausalityStaticFactories()` does nothing. Note that it wasn't exported - it was used only for cross-TU interaction between ppltasks.cpp and dllmain.cpp in stl/src, so we can remove this function outright. Then, dllmain.cpp becomes a stub.
…e` now.

Accordingly, we can drop all of these branches. Note that this drops `_M_scheduled = true;`.
In `<ppltasks.h>`, `_TaskEventLogger`'s constructor assigned `_M_scheduled = false;`, and we previously removed the only way for it to be set to `true`. Accordingly, we can drop this branch.
From bottom to top:

`const GUID PPLTaskCausalityPlatformID` wasn't exported. It had internal linkage, so we can definitely drop it.

Nothing mentions the `AsyncCausalityTracer` type or the `asyncCausalityTracer` global variable anymore.

Finally, we don't need the using-directives here.
The app implementation of `_TaskEventLogger` is now identical to the desktop implementation (after we drop the `_IsContinuation` parameter name). Unify them.
@StephanTLavavej StephanTLavavej removed their assignment Oct 20, 2023
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Final Review in Code Reviews Oct 20, 2023
…pt`.

Also drop the commented-out parameter names to avoid wrapping; they aren't useful when the function does nothing.
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Oct 24, 2023
@CaseyCarter CaseyCarter removed their assignment Oct 24, 2023
@StephanTLavavej StephanTLavavej self-assigned this Oct 24, 2023
@StephanTLavavej
Copy link
Member Author

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

@StephanTLavavej StephanTLavavej merged commit 2e9c8a8 into microsoft:main Oct 25, 2023
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Oct 25, 2023
@StephanTLavavej StephanTLavavej deleted the ex-xfg branch October 25, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants