Avoid in filesystem.cpp

[Stephan T. Lavavej (StephanTLavavej)]

Background

Zack Johnson (@zacklj89) found this while updating the STL in Windows. (Thanks Zack, for investigating this for over a week - it wasn't easy! 😸)

#2302 (merged for VS 2022 17.4 Preview 2) made filesystem.cpp (which is injected into the STL's import lib) include to cache a function pointer. Unfortunately, is a non-core header, and it's the first non-core header included by filesystem.cpp:

STL/stl/inc/atomic

Line 9 in 2263d93

#include

This drags in _Init_locks, which is separately compiled and has an implicitly defined assignment operator:

STL/stl/inc/yvals.h

Lines 456 to 475 in 2263d93

	class _CRTIMP2_PURE_IMPORT _Init_locks { // initialize mutexes
	public:
	#ifdef _M_CEE_PURE
	__CLR_OR_THIS_CALL _Init_locks() noexcept {
	_Init_locks_ctor(this);
	}
	__CLR_OR_THIS_CALL ~_Init_locks() noexcept {
	_Init_locks_dtor(this);
	}
	#else // _M_CEE_PURE
	__thiscall _Init_locks() noexcept;
	__thiscall ~_Init_locks() noexcept;
	#endif // _M_CEE_PURE
	private:
	static void __cdecl _Init_locks_ctor(_Init_locks*) noexcept;
	static void __cdecl _Init_locks_dtor(_Init_locks*) noexcept;
	};

In a certain project, this was leading to linker errors, as the STL's DLL exports _Init_locks::operator= and the linker was seeing a duplicate definition in the injected filesystem.obj.

GitHub vs. MSVC difference

Curiously, I'm unable to observe this assignment operator in the import lib produced by microsoft/STL main, but I can see it in the MSVC-internal build. I'm not sure this is due to a very recent compiler change, or some difference in the CMake vs. MSBuild build systems. In any event, dragging in this non-core header is risky, and we should avoid it due to the potential for similar linker errors when 17.4 ships.

microsoft/STL main

D:\GitHub\STL\out\build\x64\out\lib\amd64>dumpbin /symbols msvcprt.lib | grep -P __std_fs_remove
172 00000000 SECT85 notype ()    External     | __std_fs_remove
258 00000000 SECTC3 notype       Static       | $unwind$__std_fs_remove
25B 00000000 SECTC4 notype       Static       | $pdata$__std_fs_remove

D:\GitHub\STL\out\build\x64\out\lib\amd64>dumpbin /symbols msvcprt.lib | grep -P _Init_locks

D:\GitHub\STL\out\build\x64\out\lib\amd64>

MSVC-internal prod/fe

C:\Temp>dumpbin /symbols \\vcfs\builds\msvc\fe\20220808.01\binaries.amd64ret\lib\amd64\msvcprt.lib | grep -P __std_fs_remove
179 00000000 SECT88 notype ()    External     | __std_fs_remove
25F 00000000 SECTC6 notype       Static       | $unwind$__std_fs_remove
262 00000000 SECTC7 notype       Static       | $pdata$__std_fs_remove

C:\Temp>dumpbin /symbols \\vcfs\builds\msvc\fe\20220808.01\binaries.amd64ret\lib\amd64\msvcprt.lib | grep -P _Init_locks
12D 00000000 SECT1C notype ()    External     | ??4_Init_locks@std@@QEAAAEAV01@AEBV01@@Z (public: class std::_Init_locks & __cdecl std::_Init_locks::operator=(class std::_Init_locks const &))
3E9 00000000 SECT131 notype ()    External    | ??4_Init_locks@std@@QEAAAEAV01@AEBV01@@Z (public: class std::_Init_locks & __cdecl std::_Init_locks::operator=(class std::_Init_locks const &))

Fix

I believe the proper fix is to simply avoid entirely. Users should be very rarely calling temp_directory_path(), so the performance impact of always calling GetProcAddress() should be minimal. Zack has confirmed that this change fixes the affected project in Windows.

Because #2302 was merged for 17.4 Preview 2, if we remove the dependency for 17.4 Preview 3, it will never have shipped to users for production.

Size impact

This shrinks the import lib:

File Size (bytes)	main	This PR	Difference
msvcprt.lib	1,843,274	1,826,044	17,230
msvcprtd.lib	1,820,744	1,799,424	21,320

[Zack Johnson (zacklj89)]

🙏 Thanks for the quick fix and help!

Oddly enough, looks like a sporadic ASAN failure as well :(

[Casey Carter (CaseyCarter)]

Oddly enough, looks like a sporadic ASAN failure as well :(

Updated #2908 and re-ran. Also, congratulations on finally running this monster aground!

[Stephan T. Lavavej (StephanTLavavej)]

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

[S. B. Tam (cpplearner)]

It seems that _Init_locks is not used by any public header. Can it be moved out of yvals.h?

[nicole mazzuca (strega-nil-ms)]

Ah, sorry about this; it's really unfortunate that we don't have a core atomic header (real stdatomic.h when?)