Export VCRuntime entities properly#4375
Merged
StephanTLavavej merged 11 commits intomicrosoft:mainfrom Feb 12, 2024
Merged
Conversation
CaseyCarter
suggested changes
Feb 8, 2024
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
`<yvals_core.h>` includes `<vcruntime.h>`. If it emits an upcoming macro `_EXPORT_VCR`, we'll assume that VCRuntime has been updated to export its own machinery.
`NULL` is unfortunately necessary to avoid warnings C28252 and C28253 "Inconsistent annotation for 'new'".
CaseyCarter
approved these changes
Feb 12, 2024
| // N4971 [module.interface]/6: "A redeclaration of an entity X is implicitly exported | ||
| // if X was introduced by an exported declaration; otherwise it shall not be exported." | ||
|
|
||
| // Therefore, we'll need to introduce exported declarations of <vcruntime_new.h> machinery before including it. |
Contributor
There was a problem hiding this comment.
Mega nitpick (not worth resetting testing):
Suggested change
| // Therefore, we'll need to introduce exported declarations of <vcruntime_new.h> machinery before including it. | |
| // Therefore, we need to introduce exported declarations of <vcruntime_new.h> machinery before including it. | |
Member
Author
There was a problem hiding this comment.
Sure, I can handle this in a followup. This code is destined to be removed after the 17.10p3 toolset update anyways.
| export extern "C++" const nothrow_t nothrow; | ||
|
|
||
| #ifdef __cpp_aligned_new | ||
| export extern "C++" enum class align_val_t : size_t; |
Contributor
There was a problem hiding this comment.
The #ifdef __EDG__ // TRANSITION, VSO-1618988 workaround has gone missing. Is it not needed since the std module is always built with C1XX?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4350. Mirrored as MSVC-PR-527804 with VCRuntime changes.
🐞 The Problem
The Standard has a rule WG21-N4971 [module.interface]/6: "A redeclaration of an entity X is implicitly exported if X was introduced by an exported declaration; otherwise it shall not be exported." followed by an example:
Clang enforces this rule, but MSVC let me be a bad kitty 😼, jumping up on the table where I'm not allowed, reported as VSO-1953157 "Modules: MSVC should emit an error when an exported declaration follows a non-exported declaration".
🩹 The Short-Term Fix
For types, we can export aliases, which also avoids the need to mention
extern "C++". This technique looks unusual, so we'll comment it with Standard citations.The
std::nothrowobject and::operator new/new[]/delete/delete[]functions are subject to the same rule, but they're trickier to handle because we can't just alias them. So, I'm fusing<new>'s#ifdef _BUILD_STD_MODULEmachinery directly intostd.ixx(it expanded to nothing elsewhere).In
std.ixx, we have to wait for the exact right moment. We begin with the Global Module Fragment, we define_BUILD_STD_MODULEimmediately after, and then include CRT headers. (These won't drag in VCRuntime machinery.) Then we sayexport module std;to start the party, and we silence the warning about including headers within a named module.Now, before we include any STL headers (which will drag in VCRuntime headers), we include
<yvals_core.h>. This will observe the definition of_BUILD_STD_MODULEabove (we're assuming classic headers, as commented), and drags in<vcruntime.h>(but not any<vcruntime_MEOW.h>).We check whether that emits
_VCRT_EXPORT_STD(see the long-term fix below). If not, we're working with an older VCRuntime that won't export its own machinery, so we need to introduce exported declarations first.For laziness, I'm using the full set of push/pop guards, even though
std.ixxisn't a header and all we really need is the packing. Then I can declare thenothrow_ttype, thenothrowobject, and thealign_val_ttype. Finally, I need to replicate the (incredibly verbose) declarations of all of the global allocation/deallocation operators, complete with SAL annotations (we're the first declarations, so we can't omit them). They mentionNULLwhich I need to preserve 🤮 because VCRuntime says that and they need to match. I adjusted the declarations to use our_STDand westconst.Because this is
std.ixxitself, I'm sayingexportdirectly, there's no reason to_EXPORT_STDhere.🛠️ The Long-Term Fix
This is an unusual PR because it takes two forms simultaneously. The GitHub portion is the short-term fix, but it'll be simultaneously merged with VCRuntime changes that are the long-term fix. The VCRuntime changes will make
<vcruntime.h>detect_BUILD_STD_MODULEand define_VCRT_EXPORT_STD, so it'll export its own machinery. When this happens, the presence of the_VCRT_EXPORT_STDmacro will automatically disable the STL's workarounds, so it won't try to redundantlyexportthings. Instd.ixx, we'll be left with an unguarded inclusion of<yvals_core.h>which is harmless (it's immediately followed by including<algorithm>).After this PR is merged, and ships in (expected) VS 2022 17.10 Preview 3, I'll be able to remove all of the workarounds in the STL. Having VCRuntime export its own machinery is what I should have done in the first place, and this will finally get us there.