Repository navigation
Pragma "section(".CRT$XCU", read)" can prevent global variable dynamic initialization #41852
Description
Version
v16.6.0
Platform
Microsoft Windows NT 10.0.19043.0 x64
Subsystem
Node API
What steps will reproduce the bug?
Compile this C++ with MSVC (c++17 required)
#include <node_api.h>
#include <iostream>
inline std::string testString = "123";
napi_value entry_point(napi_env, napi_value exports)
{
std::cout << "The value of the string is " << testString;
return exports;
}
NAPI_MODULE(inline_test, entry_point)Then run
node -e "require('./path/to/addon')"How often does it reproduce? Is there a required condition?
Always
What is the expected behavior?
The expected behavior is for the reproduction steps above to print out The value of the string is 123.
What do you see instead?
The reproduction steps above print out The value of the string is. This happens because the initializer for the inline variable testString is optimized out. Apparently it is optimized out because of the line #pragma section(".CRT$XCU", read) in node_api.h. When I remove that pragma, the program works as expected.
See this blog for more details on why pragma section can cause this behavior: https://devblogs.microsoft.com/cppblog/new-compiler-warnings-for-dynamic-initialization/
Additional information
Why is the pragma needed? Can it be removed? If it is required only in certain conditions, maybe it could be inside an #ifdef check?
If the pragma is always required but only for the definition of the module registration logic, perhaps the pragma could go inside the NAPI_C_CTOR macro by using the standard _Pragma directive like this:
#define NAPI_C_CTOR(fn) \
_Pragma("section(\".CRT$XCU\", read)") \
static void __cdecl fn(void); \
__declspec(dllexport, allocate(".CRT$XCU")) void(__cdecl * fn##_)(void) = \
fn; \
static void __cdecl fn(void)Either removing the pragma or placing it inside of NAPI_C_CTOR fixes the bug (though the bug would probably reappear if an inline was used below the module registration logic).
Activity
[-]The pragma "section(".CRT$XCU", read)" can prevent global variable dynamic initialization[/-][+]Pragma "section(".CRT$XCU", read)" can prevent global variable dynamic initialization[/+]ekraihan commentedon Feb 4, 2022
I also wondered, is there any workaround for this in the meantime? Is there any other way to register a node module that doesn't touch this #pragma logic? (Or I can just not use
inlinevariables for now)ekraihan commentedon Feb 4, 2022
I also opened a ticket with Microsoft about the "pragma section" doing this garbage collection of variable initializers: https://developercommunity.visualstudio.com/t/pragma-sectionCRTXCU-read-garbag/1654499
Flarna commentedon Feb 9, 2022
@nodejs/node-api
KevinEady commentedon Feb 11, 2022
We briefly discussed this in the 11 Feb Node API meeting. This pragma has been present in
node_api.hsince its inception for ABI stable methods, introduced by @gabrielschulhof . We will need to discuss with him (not present at the meeting today) to see why / if this pragma was needed. Tabling until next week.Flarna commentedon Feb 11, 2022
I guess this is just a copy of the corresponding code in node.h.
Maybe it could be fixed by something like this which uses C++ style initialization in case file is compiled as C++ instead the crt section hack:
Moving the pragma into the macro didn't work for me. Besides that it seems to be just a partial fix as it likely stops working if
NAPI_MODULE()is placed at the top of the file.vmoroz commentedon Mar 11, 2022
In the last Node-API meeting on 3/4/2022 I was tasked to look into details of this issue.
It seems that the only purpose of using the
".CRT$XCU"section is to do dynamic initialization of static variables.It is documented in https://docs.microsoft.com/en-us/cpp/c-runtime-library/crt-initialization?view=msvc-170
The issue is that the
".CRT$XCU"is exactly the same section as what MSVC compiler generates for initializing C++ static variables and thus it causes a conflict. It is not clear how it ever worked for C++ code.The best fix is to completely remove use of the section and replace it with a normal static variable initialization similar to what @Flarna proposes above. I would suggest a few small corrections to that code:
fnname.make the class name local to the .cpp file by using an anonymous namespace to match theI realized that this code must be C-only and the anonymous namespaces do not make sense.statickeyword behavior used by the function declaration.As a minimal change, we must replace use of the
".CRT$XCU"with".CRT$XCT"or".CRT$XCV"as suggested by the MS docs. Depending on the choice the initialization will happen before or after the standard".CRT$XCU"since CRT sections are sorted alphabetically, but I doubt that we ever cared about the order before.vmoroz commentedon Mar 11, 2022
This StackOverflow article is addressing the same issue:
https://stackoverflow.com/questions/1113409/attribute-constructor-equivalent-in-vc
One of the key additions on top of what was discussed above is that they add
__pragma(comment(linker,"/include:" p #f "_"))for C code to ensure that the optimizer does not remove the pointer declaration.vmoroz commentedon Mar 11, 2022
This issue was discussed in Node-API meeting today 3/11/2022.
I am going to work on a PR to address this issue based on the discussion above.
Flarna commentedon Mar 15, 2022
I don't think it's needed to be C only. There must be a C variant but by using
#if defined(__cplusplus)an additional C++ variant (without any#pragma) should be fine.vmoroz commentedon Mar 19, 2022
Edited: @Flarna, yes, you are right: the struct-based approach can work only for C++ and thus we better use the anonymous namespace to match the
statickeyword for the function. The C-based code must continue to use the.CRT$XCUbased approach as in your code snipped above. I hope to submit the PR in the next few days.mhdawson commentedon May 6, 2022
Closing as PR to address was landed
ekraihan commentedon May 15, 2022
Awesome! Thanks for working on this.