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

Standard Library Modules: Fix time_put<wchar_t> linker errors #3232

Merged
merged 4 commits into from Nov 18, 2022

Conversation

StephanTLavavej
Copy link
Member

  • Related cleanup: don't mark time_put partial specializations as _CRTIMP2_PURE_IMPORT.
    • The primary template isn't marked:

      STL/stl/inc/xloctime

      Lines 651 to 652 in 0098818

      _EXPORT_STD extern "C++" template <class _Elem, class _OutIt = ostreambuf_iterator<_Elem, char_traits<_Elem>>>
      class time_put : public locale::facet { // facet for converting encoded times to text
      @cdacamar made the surprising discovery that the compiler has always ignored marked partial specializations in this scenario - see VSO-1674115 "__declspec(dllimport) on partial specialization has no semantic meaning". It's deeply confusing to retain something that has no effect, so I'm removing it.
    • I audited for other occurrences, and these appear to be the only ones.
    • I verified that the release and debug dllexports are unchanged. (Additionally, we have dllexport validation in the MSVC-internal build.)
  • Mark time_put partial specializations as extern "C++" like their primary template.
    • This fixes VSO-1593165 "Standard Library Modules: time_put<wchar_t> emits bogus error LNK2019: unresolved external symbol", which I originally thought was a compiler bug. Thanks again @cdacamar for analyzing this.
  • Add test coverage for the fixed time_put<wchar_t> scenario.
  • Mark codecvt/ctype explicit specializations as extern "C++" like their primary templates:

    STL/stl/inc/xlocale

    Lines 668 to 669 in 0098818

    _EXPORT_STD extern "C++" template <class _Elem, class _Byte, class _Statype>
    class codecvt : public codecvt_base { // facet for converting between _Elem and _Byte sequences

    STL/stl/inc/xlocale

    Lines 2385 to 2386 in 0098818

    _EXPORT_STD extern "C++" template <class _Elem>
    class ctype : public ctype_base { // facet for classifying elements, converting cases
    • This doesn't appear to affect correctness, but it's what I originally intended. (I naively thought that extern "C++" on a primary template would affect all explicit/partial specializations, which is actually how export works.)

The primary template isn't marked. See VSO-1674115 "__declspec(dllimport) on partial specialization has no semantic meaning".

Verified that release and debug exports are unchanged.
…ary template.

See VSO-1593165 "Standard Library Modules: time_put<wchar_t> emits bogus error LNK2019: unresolved external symbol".
…r primary templates.

This doesn't appear to affect correctness, but it's what I originally intended.
@StephanTLavavej StephanTLavavej added bug Something isn't working modules C++23 modules, C++20 header units labels Nov 16, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner November 16, 2022 02:34
@StephanTLavavej StephanTLavavej added this to Final Review in Code Reviews Nov 16, 2022
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Nov 17, 2022
@CaseyCarter CaseyCarter removed their assignment Nov 17, 2022
@CaseyCarter
Copy link
Member

CaseyCarter commented Nov 17, 2022

I can't wrap my brain around why we need extern "C++" for partial specializations but not explicit specializations of extern "C++" templates. (I suspect you are similarly confused, hence annotating explicit specializations despite that they don't seem to need it.) It makes sense to me that e.g. program-defined specializations of STL templates (e.g., iterator_traits) would need to attach to a different module than the library-defined primary template, but I can't see why the behavior for explicit and partial specializations would differ. Any idea/intuition here?

(To be clear: this isn't a concern with the content of the PR - I'm happy to apply this on the basis that it "works" - so much as trying to get my head straight so I can spot similar problems in the future.)

@StephanTLavavej
Copy link
Member Author

I suspect that extern "C++" works the same way for partial specializations and explicit specializations (i.e. they affect whether the specialization is attached to the global module via name mangling), and that the difference is in how the STL has separately compiled its facets (which I have never properly understood). It appears that both time_put and ctype are dllexported, but perhaps there is a behavior difference that I haven't found yet.

@StephanTLavavej
Copy link
Member Author

Aha, I think I have a better guess! time_put's _OutIt defaults to ostreambuf_iterator, but the test uses wstring::iterator. Thus, we need to instantiate a specialization for wstring::iterator (I think those are the right words), there's no way we can consume it from the DLL. My guess is that when the primary template is marked extern "C++" but not the partial specialization, something goes wrong (not sure exactly what) and we don't get an implicit instantiation. Therefore we end up with an unresolved external.

Again, not totally sure what's really happening here, but this does seem like the relevant difference that would explain why ctype seemed to work as-is.

@StephanTLavavej StephanTLavavej self-assigned this Nov 18, 2022
@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 8ffd4a4 into microsoft:main Nov 18, 2022
Code Reviews automation moved this from Ready To Merge to Done Nov 18, 2022
@StephanTLavavej StephanTLavavej deleted the time-put branch November 18, 2022 23:48
@CaseyCarter
Copy link
Member

Thanks for taking the time to put this right!

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
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants