Pragma "section(".CRT$XCU", read)" can prevent global variable dynamic initialization

[ekraihan]

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).

[ekraihan]

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 inline variables for now)

[ekraihan]

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]

@nodejs/node-api

[KevinEady]

We briefly discussed this in the 11 Feb Node API meeting. This pragma has been present in node_api.h since 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]

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.

[vmoroz]

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 fn name.
• make the class name local to the .cpp file by using an anonymous namespace to match the static keyword behavior used by the function declaration. I realized that this code must be C-only and the anonymous namespaces do not make sense.

#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.

[vmoroz]

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]

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]

• make the class name local to the .cpp file by using an anonymous namespace to match the static keyword behavior used by the function declaration. I realized that this code must be C-only and the anonymous namespaces do not make sense.

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]

• make the class name local to the .cpp file by using an anonymous namespace to match the static keyword behavior used by the function declaration. I realized that this code must be C-only and the anonymous namespaces do not make sense.

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 static keyword for the function. The C-based code must continue to use the .CRT$XCU based approach as in your code snipped above. I hope to submit the PR in the next few days.

[mhdawson]

Closing as PR to address was landed

[ekraihan]

Closing as PR to address was landed

Awesome! Thanks for working on this.