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

Split/fuse xlocinfo.h to fix unintentional export in format.cpp #1927

Merged
merged 3 commits into from
Jun 4, 2021

Conversation

StephanTLavavej
Copy link
Member

This affects separately compiled code, but shouldn't modify the DLL's export surface or break ABI in any other way. The second degree of caution is warranted.

In certain modules scenarios (both Standard Library Header Units, and experimental C++23 modules), compiling EXEs dragging in format.obj would display "Creating library meow.lib and object meow.exp", and the resulting EXE would export _Init_locks::operator=. This was definitely unintentional.

The problem was that format.cpp, injected into the import lib, was including xlocinfo.h. I am not 100% certain of the exact chain of events, but it appears that in certain cases, this would drag in _Init_locks, which we don't need or want here.

Separately, xlocinfo.h was problematic for deduplicated Standard Library Header Units, because building both <xlocinfo> and <xlocinfo.h> wants to create xlocinfo.obj by default. No other STL headers have the same name like this.

The fix to both of these problems is to split xlocinfo.h's contents. I'm adding a minimal header __msvc_xlocinfo_types.hpp (qualifying as a "core" header), which defines just the _Collvec, _Ctypevec, and _Cvtvec structs, which have no further dependencies. The rest of xlocinfo.h is then fused into xlocinfo.

format.cpp needs only _Cvtvec, but by adding the two other structs into the mini-header, we can change all of the src files to include just that mini-header (they don't need the former xlocinfo.h's function declarations to provide their function definitions).

No DLL-exported functions are being modified here. This modifies format.obj in the import lib, which is effectively statically linked, so there's no compatibility concern.

Additional notes:

  • Most src files were already including yvals.h (which provides our export macros), sometimes via awint.hpp, but xmbtowc.cpp and xwctomb.cpp need to explicitly include it.
  • xwcscoll.cpp was including <cstring> for wmemcmp() but that's incorrect - it needs to include <cwchar>. (The VSO-661721 thing about needing to include <cstdio> before <cwchar> applies to header files only, not source files like this one.)
  • Now that we aren't including function declarations, xwctomb.cpp needs to define _Getcvt() and _Wcrtomb() before calling them, so their definitions are being reordered here. There are no other changes (and these aren't something like the initial declarations of virtual functions where the order matters).
  • I'm not adding test coverage here because this is surprisingly hard to encounter; the known repros are experimental modules (which we don't build in the GitHub repo) or my work-in-progress deduplicated header units (which are blocked by a few compiler bugs). The risk of this regressing is low because we don't mess with separately compiled code that often.
  • When mirroring this to the internal repo, we'll need internal Hack These Files changes.

In xwctomb.cpp, need to reorder functions.

Also, <cwchar> provides wmemcmp(), not <cstring>.
@StephanTLavavej StephanTLavavej added bug Something isn't working format C++20/23 format labels May 22, 2021
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner May 22, 2021 03:09
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Looks great - thanks for picking this up.

@barcharcraz
Copy link
Member

Before this change we had both xlocinfo.h and xlocinfo? Was xlocinfo simply separately compiled (despite lacking the traditional extension)

@StephanTLavavej
Copy link
Member Author

Before this change we had both xlocinfo.h and xlocinfo?

Correct - with header guards differing by one crucial underscore 🙀

STL/stl/inc/xlocinfo

Lines 6 to 8 in cb37189

#pragma once
#ifndef _XLOCINFO_
#define _XLOCINFO_

#pragma once
#ifndef _XLOCINFO
#define _XLOCINFO

Was xlocinfo simply separately compiled (despite lacking the traditional extension)

No, it's eventually included by Standard headers; one path is:

#include <xlocinfo>

#include <xlocale>

#include <xiosbase>

At this time, I believe everything in stl/inc is needed by user-includable headers; nothing is meant for separately compiled code only. (I recall cleaning up the last occurrences many years ago; I think what is now src/xmath.hpp might have been the last one.)

@CaseyCarter CaseyCarter self-assigned this May 27, 2021
@CaseyCarter CaseyCarter merged commit 20288f9 into microsoft:main Jun 4, 2021
@CaseyCarter
Copy link
Member

Thanks for fixing this bug in which some STL developer - who we won't name and shame here - added an unintentional export to the import library. (Hint: it was me.)

@CaseyCarter CaseyCarter removed their assignment Jun 4, 2021
@StephanTLavavej StephanTLavavej deleted the format_export branch June 4, 2021 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working format C++20/23 format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants