Repository navigation
Pragma "section(".CRT$XCU", read)" can prevent global variable dynamic initialization #41852
Description
Activity
- changed the title
[-]The pragma "section(".CRT$XCU", read)" can prevent global variable dynamic initialization[/-][+]Pragma "section(".CRT$XCU", read)" can prevent global variable dynamic initialization[/+]on 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)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
@nodejs/node-api
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.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:
#if defined(__cplusplus) #define NAPI_C_CTOR(fn) \ static void __cdecl fn(); \ struct napi_init_helper { \ napi_init_helper() { fn(); } \ }; \ static napi_init_helper init_helper; \ static void __cdecl fn() #else #pragma section(".CRT$XCU", read) #define NAPI_C_CTOR(fn) \ static void __cdecl fn(void); \ __declspec(dllexport, allocate(".CRT$XCU")) void(__cdecl * fn##_)(void) = \ fn; \ static void __cdecl fn(void) #endif
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.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:
- make the class name to be dependent on the
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.
#define NAPI_C_CTOR(fn) \ static void __cdecl fn(); \ struct napi_init_helper_##fn { \ napi_init_helper() { fn(); } \ } napi_init_helper_inst_##fn; \ static void __cdecl fn()
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.- make the class name to be dependent on the
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.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.Reacted by Michael Dawsonmake 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.
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.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.
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.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.Closing as PR to address was landed
Closing as PR to address was landed
Awesome! Thanks for working on this.
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)
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 theinlinevariabletestStringis optimized out. Apparently it is optimized out because of the line#pragma section(".CRT$XCU", read)innode_api.h. When I remove that pragma, the program works as expected.See this blog for more details on why pragma
sectioncan 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
#ifdefcheck?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_CTORmacro by using the standard_Pragmadirective like this:Either removing the pragma or placing it inside of
NAPI_C_CTORfixes the bug (though the bug would probably reappear if an inline was used below the module registration logic).