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

<chrono>: Including header pulls in a dynamic initializer, incompatible with /kernel #1926

Closed
kobykahane opened this issue May 18, 2021 · 7 comments · Fixed by #3403
Closed
Labels
bug Something isn't working chrono C++20 chrono fixed Something works now, yay!

Comments

@kobykahane
Copy link

kobykahane commented May 18, 2021

Describe the bug
With Visual Studio 16.10 (Preview 3), just including <chrono> now pulls in a dynamic initializer for std::numpunct<char>::id and std::numpunct<wchar_t>::id, which was previously not the case in 16.9. Presumably this is the result of #1821 or #1870.
Since we use <chrono> as part of kernel mode driver projects, this now results in a linker warning:

repro.obj : warning LNK4210: .CRT section exists; there may be unhandled static initializers or terminators

Since the kernel mode build environment does not have startup code that calls these initializers, this is problematic. For these specific initializers the warning is benign in the sense formatting-related chrono functionality cannot be consumed anyway. However, suppressing the warning globally (in our project) is also undesirable because it would allow developers to sneak in their own initializers which are never invoked.

It's clear to us that <chrono> isn't officially supported for consumption in the kernel mode build environment and isn't really a part of the freestanding subset. However, since this is a regression, it seems appropriate to bring it to your attention for consideration.

Command-line test case

C:\...\repro>type repro.cpp
#include <chrono>

extern "C" int atexit(void (__cdecl *func)(void));

int atexit(void (__cdecl *func)(void))
{
  return 0;
}

C:\...\repro>cl /EHsc /Z7 /std:c++latest /permissive- /GS- /LDd repro.cpp /link /noentry /nodefaultlib
Microsoft (R) Incremental Linker Version 14.29.30035.0
Copyright (C) Microsoft Corporation.  All rights reserved.
...
repro.obj : warning LNK4210: .CRT section exists; there may be unhandled static initializers or terminators

while with 16.9:

C:\...\repro>cl /EHsc /Z7 /std:c++latest /permissive- /GS- /LDd repro.cpp /link /noentry /nodefaultlib
Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29915 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.
...
repro.cpp
...
repro.obj

with no warning.

Expected behavior
Ideally just including <chrono> without consuming any of the new formatting related functionality won't pull in any redundant dynamic initializers.

STL version

Microsoft Visual Studio Community 2019 Preview
Version 16.10.0 Preview 3.0

Additional context
The repro command lines above do not use /kernel or the kernel mode build environment, but simulate the same situation by not linking with the usual user mode startup code.

@StephanTLavavej StephanTLavavej added bug Something isn't working chrono C++20 chrono labels May 18, 2021
@kobykahane
Copy link
Author

Would it be possible to mitigate this issue by modifying std::locale::id to have a parameterless default constructor and an NSDMI for _Id? E.g., as follows:

    // CLASS id
    class _CRTIMP2_PURE_IMPORT id { // identifier stamp, unique for each distinct kind of facet
    public:
        constexpr __CLR_OR_THIS_CALL id() noexcept = default;
        __CLR_OR_THIS_CALL id(size_t _Val) : _Id(_Val) {}

        id(const id&) = delete;
        id& operator=(const id&) = delete;

        __CLR_OR_THIS_CALL operator size_t() { // get stamp, with lazy allocation
            if (_Id == 0) { // still zero, allocate stamp
                _BEGIN_LOCK(_LOCK_LOCALE)
                if (_Id == 0) {
                    _Id = static_cast<size_t>(++_Id_cnt);
                }
                _END_LOCK()
            }
            return _Id;
        }

    private:
        size_t _Id{}; // the identifier stamp

        __PURE_APPDOMAIN_GLOBAL static int _Id_cnt;
    };

i.e., removing the default argument value 0 from _Val and adding an additional constructor. With a change like this, global std::locale::id objects would no longer have dynamic initializers.

@StephanTLavavej
Copy link
Member

I believe that would break ABI because this is separately compiled (as obscurely indicated by the _CRTIMP2_PURE_IMPORT macro), so that wouldn't be possible until vNext.

@kobykahane
Copy link
Author

I believe that would break ABI because this is separately compiled (as obscurely indicated by the _CRTIMP2_PURE_IMPORT macro), so that wouldn't be possible until vNext.

What if instead of the entire class being __declspec(dllimport), all the existing members (including the "legacy" constructor) are marked that way, but the new default constructor isn't? Previously compiled code would import the old separately compiled constructor (which has the same signature and remains ABI compatible). Newly compiled code would see the new additional constructor which is inline. No new export would be introduced to the separately compiled code.

@StephanTLavavej
Copy link
Member

That might work, but I would be very hesitant to attempt such a change - the locale IDs are basically the most fragile, most poorly understood part of the entire STL, and it's very easy to damage things in a subtle way (as I did back in the VS 2010 era). We've never removed __declspec(dllimport) from a class and moved it to its member functions in that way.

@kobykahane
Copy link
Author

That might work, but I would be very hesitant to attempt such a change - the locale IDs are basically the most fragile, most poorly understood part of the entire STL, and it's very easy to damage things in a subtle way (as I did back in the VS 2010 era). We've never removed __declspec(dllimport) from a class and moved it to its member functions in that way.

Just out of curiosity, what happened in the circa VS 2010 incident?

@StephanTLavavej
Copy link
Member

As I vaguely recall (it was a decade ago and I was new so I barely understood anything), I tried to make the machinery header-only, which damaged the order in which the IDs were assigned. I was able to successfully make other machinery header-only (including basic_string) but didn't understand the difference at the time.

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Feb 11, 2023

I used to think this was fixed by #2496 but I was probably wrong then. I think my new PR can fix this, see the analyzation in the PR.

@StephanTLavavej StephanTLavavej added the fixed Something works now, yay! label Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chrono C++20 chrono fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants