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

Export VCRuntime entities properly #4375

Merged
merged 11 commits into from
Feb 12, 2024

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Feb 8, 2024

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:

export module M;
struct S { int n; };
typedef S S;
export typedef S S; // OK, does not redeclare an entity
export struct S;    // error: exported declaration follows non-exported declaration

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::nothrow object 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_MODULE machinery directly into std.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_MODULE immediately after, and then include CRT headers. (These won't drag in VCRuntime machinery.) Then we say export 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_MODULE above (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.ixx isn't a header and all we really need is the packing. Then I can declare the nothrow_t type, the nothrow object, and the align_val_t type. 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 mention NULL which I need to preserve 🤮 because VCRuntime says that and they need to match. I adjusted the declarations to use our _STD and west const.

Because this is std.ixx itself, I'm saying export directly, there's no reason to _EXPORT_STD here.

🛠️ 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_MODULE and define _VCRT_EXPORT_STD, so it'll export its own machinery. When this happens, the presence of the _VCRT_EXPORT_STD macro will automatically disable the STL's workarounds, so it won't try to redundantly export things. In std.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.

@StephanTLavavej StephanTLavavej added bug Something isn't working modules C++23 modules, C++20 header units labels Feb 8, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner February 8, 2024 12:39
@StephanTLavavej StephanTLavavej changed the title Export VCRuntime types properly Export VCRuntime entities properly Feb 8, 2024
stl/inc/exception Show resolved Hide resolved
stl/inc/typeinfo Show resolved Hide resolved
stl/inc/new Outdated Show resolved Hide resolved
@CaseyCarter

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej self-assigned this Feb 8, 2024
StephanTLavavej and others added 7 commits February 8, 2024 12:47
`<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'".
// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The #ifdef __EDG__ // TRANSITION, VSO-1618988 workaround has gone missing. Is it not needed since the std module is always built with C1XX?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct.

@StephanTLavavej StephanTLavavej merged commit 7d5d8c3 into microsoft:main Feb 12, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working modules C++23 modules, C++20 header units
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Exported declaration of std::exception follows non-exported declaration
2 participants