Avoid <atomic> in filesystem.cpp

[StephanTLavavej]

Background

@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 <atomic> to cache a function pointer. Unfortunately, <atomic> 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 <yvals.h>

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 <atomic> 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 <atomic> 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

[zacklj89]

🙏 Thanks for the quick fix and help!

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

[CaseyCarter]

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

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

[StephanTLavavej]

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

[cpplearner]

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

[strega-nil-ms]

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