winapisupp.cpp: Dramatically shrink the function pointer table

[StephanTLavavej]

Now that we've merged #2317, there are only a couple of functions we're dynamically loading.

• Dramatically shrink the __KERNEL32Functions table.
  • The only direct uses of __KERNEL32Functions are in STOREFUNCTIONPOINTER and DYNAMICGETCACHEDFUNCTION.
  • The only use of DYNAMICGETCACHEDFUNCTION is in IFDYNAMICGETCACHEDFUNCTION.
  • There are only two uses of IFDYNAMICGETCACHEDFUNCTION, loading GetCurrentPackageId and GetSystemTimePreciseAsFileTime.
  • Therefore, all other table entries are unused.
  • The comment // This enum should not change was introduced by Remove XP (and Server 2003) support from STL #1194, but we were being overly cautious. The enum and table aren't mentioned outside of stl/src. Also, while the table was extern "C", it wasn't exported. Therefore, they're purely internal to the separately compiled STL.
• De Morgan the guards around GetCurrentPackageId.
• Move __KERNEL32Functions machinery to winapisupp.cpp.
  • It was within _CRT_BEGIN_C_HEADER / _CRT_END_C_HEADER.
• Guard __KERNEL32Functions machinery with #if !defined(_ONECORE). Fuse declaration and definition.
  • The definition and initialization of __KERNEL32Functions were effectively guarded by #if !defined(_ONECORE), so we should extend that guard to all of the machinery.
  • When fusing the declaration and definition, we're within _CRT_BEGIN_C_HEADER so we don't need extern "C". We also don't need extern (the definition already has external linkage, and other TUs don't need this anyways).
  • For style, initialize with {} instead of = {nullptr}. They have the same effect here, but {} emphasizes that we want to value-initialize the entire table.
• Use an unnamed namespace instead of extern "C" for the __KERNEL32Functions machinery.
  • We don't need this in other TUs. The unnamed namespace emphasizes that it's purely internal.
• Cache GetSystemTimePreciseAsFileTime only when we need it.
  • Now we need to guard against eMaxKernel32Function being 0.
• Fix return type in PFNGETCURRENTPACKAGEID typedef.
  • See: https://docs.microsoft.com/en-us/windows/win32/api/appmodel/nf-appmodel-getcurrentpackageid
  • Verified that this is now identical to decltype(&GetCurrentPackageId).
• Fuse DYNAMICGETCACHEDFUNCTION into IFDYNAMICGETCACHEDFUNCTION.
  • Update comment.
  • Style: Directly test the function pointer.
• Always use decltype in IFDYNAMICGETCACHEDFUNCTION.
  • We no longer need PFNGETSYSTEMTIMEPRECISEASFILETIME.
  • Also, always emit pf##function_name.
• Use C++17 if-statement with initializer to improve scoping.
• Avoid emitting empty statements after STOREFUNCTIONPOINTER.
• winapisupp.cpp isn't using <cstdint> at all.
• Simplify __crt_IsPackagedAppHelper() with range-for and auto.
• awint.hpp doesn't need to declare __crtIsPackagedApp().
  • We still need to export it for bincompat.
• We can always use decltype for GetCurrentPackageId.
  • Verified with an internal build.

This shrinks the STL's DLL by a couple of KB on x64:

File Size (bytes)	Before	After	Difference
msvcp140_oss.dll	551,936	549,888	2,048
msvcp140d_oss.dll	914,432	911,872	2,560
libcpmt.lib	35,669,646	35,622,078	47,568
libcpmtd.lib	36,966,642	36,926,400	40,242

[AlexGuteniev]

In the past there were calls from other TUs, that's why macros and extern "C". I'm not sure ifvthis won't be neeeded if we'll expand the table again.

[StephanTLavavej]

Going forward:

• The table and its associated machinery can remain in winapisupp.cpp's unnamed namespace.
• Functions using the table via IFDYNAMICGETCACHEDFUNCTION can be centrally defined in winapisupp.cpp.
• Such functions can be declared in awint.hpp for other TUs to use.
  • This is what we did for __crtGetSystemTimePreciseAsFileTime(), except that we unnecessarily marked it _CRTIMP2 (i.e. exported), even though only stl/src/xtime.cpp needs it.

I expect that this will be uncommon, since it's rare for us to need new WinAPIs in the STL's DLL (since we can't add new exports to it); #2302 may be an example that needs it.

Note that because the table wasn't exported, it is useless to anything outside of the STL's DLL (in particular, it cannot be accessed by the import lib), so isolating it to one TU in stl/src isn't restricting its power. This is why stacktrace.cpp in the import lib needed to call GetProcAddress() directly, and so did tzdb.cpp in the atomic_wait satellite DLL.

[StephanTLavavej]

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

[barcharcraz]

Approved with suggestions.

[barcharcraz]

it strikes me as not the best idea to preprocess out enumerators like this without explicitly assigning values. If this is ever passed across ABI boundaries (it does not appear to be now) it might blow up pretty immediately

Pre-existing though

[StephanTLavavej]

Agreed, pre-existing. I believe that the risk is low (as you mentioned, this table and its enumerators aren't part of the ABI surface area - also, the conditions are inherently of the form "how is the STL being built"), and it's further mitigated by this PR, as the table and its enumerators are now confined to an unnamed namespace in a single TU.

That said, it now seems to me that the case for having a table is weaker than ever. We never iterate over the table, so we could simply use separate function pointers (which the compiler could still store densely). That would avoid any concerns about varying enumerator values, and actually make debugging potentially easier. I may explore this in a followup PR.

[barcharcraz]

now that we are only doing this a few times it might be easier to just expand these macros.

If we're going to use macros for this we could also use xmacros to define the above enumeration and the below initialization function at the same time.

[StephanTLavavej]

Hmm - the primary value of the macros is now enforcing the consistent use of decltype, so I would hesitate to eliminate them completely.

X-macros are definitely a potential technique, although they are quite powerful/opaque. As there are a small number of locations (definition, initialization, usage) and they're slowly-changing, I tend to think that we don't need such machinery at this time - especially now that the number of enumerators is so much smaller.

I'll think about refactoring this in a followup PR. I think it might be feasible to have one macro for definition (using decltype for individual variables) and one for initialization (ensuring that the GetProcAddress uses the correct spelling), and then directly refer to the variable for usage.

Thanks for bringing this up!