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

Fix import std; interaction with <intrin.h> emitting bogus error C2733 #4626

Merged
merged 2 commits into from
Apr 26, 2024

Conversation

StephanTLavavej
Copy link
Member

Fixes DevCom-10643328 VSO-2045649 "VS17.10-preview4 C++ Modules import std; fails to compile if boost::unordered::unordered_hash_map member of type also imported" which emitted:

[...]\emmintrin.h(298): error C2733: '_mm_cmpeq_epi8': you cannot overload a function with 'extern "C"' linkage

The root cause was that the STL drags in <intrin.h> (in whole or in part), and it defines a few types outside of extern "C" or extern "C++". This causes them to be attached to the std module, which we don't want. (I missed this in #4154.) The fix is to include <intrin.h> in the Global Module Fragment.

  • I've manually verified the fix with the provided Boost-powered repro. This resisted my attempts at reduction, so I think manual testing with a source comment is sufficient.
  • To improve throughput for classic includes, we often try to include subsets of <intrin.h>. In contrast, building the Standard Library Module is a one-time cost, so the simplest and safest thing to do is to include the whole <intrin.h> here.
  • I audited all of the STL's dependencies:
    • C Standard Library headers are inherently fine (i.e. not problematic in this way) because they're rigorously wrapped in extern "C". In fact, including them in the Global Module Fragment is basically redundant, although @cdacamar advised me that it was a good idea.
    • Microsoft UCRT headers are also inherently fine. <thread> includes <process.h> for _beginthreadex() (this include is necessary) and <xiosbase> includes <share.h> for _SH_DENYNO (this include is redundant; the UCRT's structure guarantees that we'd drag this in already). I considered adding these to the Global Module Fragment, but I thought that it was fine to rely on their extern "C" nature.
    • I've already fixed the VCRuntime and PPLTasks header families to be compatible with modules (i.e. ensuring that all of their machinery is either extern "C" or extern "C++", and VCRuntime directly exports certain things now).
    • Unlike them, the <intrin.h> family is large, ever-growing, and not entirely under our control (it's regularly enhanced by our processor partners). Changing it to wrap things in extern "C++" could bit-rot over time. Putting it in the Global Module Fragment is simple, robust, and non-intrusive. (Also, it is strictly layered - it doesn't ever include STL machinery. <ppltasks.h> had to be modified because it includes STL headers, so we couldn't put it in the GMF.)

During this audit, I noticed that <exception> was including <malloc.h> for absolutely no reason - I checked every single identifier and none are being used. Our "include each header alone" test is also happy with removal. I thought it was reasonable to include a one-line cleanup in this fix.

Thanks to @Klaim and @boris-kolpackov for reporting this, and to @cdacamar for investigating.

@StephanTLavavej StephanTLavavej added bug Something isn't working modules C++23 modules, C++20 header units labels Apr 24, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner April 24, 2024 06:22
@StephanTLavavej StephanTLavavej self-assigned this Apr 26, 2024
@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 7038728 into microsoft:main Apr 26, 2024
39 checks passed
@StephanTLavavej StephanTLavavej deleted the intrinsic-infravision branch April 26, 2024 23:55
huangqinjin added a commit to huangqinjin/cxxmodules that referenced this pull request Nov 11, 2024
Clang 19 allows to export redeclarations within language linkage.
llvm/llvm-project#98583

Exporting implicitly declared `type_info`/`operator new`/`operator delete` is no longer illegal.
llvm/llvm-project#90620

Clang 19.1.1 with VS 17.11 supports `import std`.
- llvm/llvm-project#108732
- microsoft/STL#4626
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.

2 participants