Node: Debug build doesn't work on Windows

[seishun]

• Version: master
• Platform: Windows
• Subsystem: n/a

A debug build built using vcbuild debug and the latest VS2017 exits immediately. It seems to be a compiler bug: the EnvironmentOptionsParser instance is initialized before the DebugOptionsParser instance, causing a segfault. I would like someone to confirm the issue before I continue looking into it.

[seishun]

cc @nodejs/platform-windows

[refack]

Confirmed.
I'm not sure it's a Compiler bug, it might be Undefined Behavior. I think I was able to fix this by reordering the singleton declarations (*OptionsParser::instance) in node_options.cc by their order of dependency. e.g. https://github.com/nodejs/node/blob/17c6b1d4f78d6050642d3cd3723c08564e67dcd5/src/node_options.cc#L121

[seishun]

They are already in the dependency order, aren't they?

[refack]

They are already in the dependency order, aren't they?

I think there is something more subtle that is related to static init order. But I didn't fully grok this.

I found my patch: https://github.com/nodejs/node/pull/25596

[seishun]

The static init order thing is irrelevant, they are all in the same translation unit here.

[refack]

they are all in the same translation unit here.

IIUC, not necessarily since they are used in two other places:
https://github.com/nodejs/node/blob/01cd21973b26a2cbacbe143c5983cb4adf8e7681/src/node_worker.cc#L440
and
https://github.com/nodejs/node/blob/55e0ad9ae690f6d73b0caff7b813eb4d61195b94/src/node.cc#L908

[seishun]

That's a usage, not a definition.

[refack]

Are we sure those two lines are not part of some other static construction?
Or alternatively that https://github.com/nodejs/node/blob/17c6b1d4f78d6050642d3cd3723c08564e67dcd5/src/node_options.cc#L121 isn't optimized away?

[addaleax]

Are we sure those two lines are not part of some other static construction?

Based on grepping the source code for cli_options, I would say that this is not the case, yes. The way in which @seishun narrowed down the issue might tell us more?

[seishun]

Narrowing it down was easy, I just pressed F5 and saw that the DebugOptionsParser instance is full of zeroes inside the EnvironmentOptionsParser constructor, and then confirmed it by setting a breakpoint in the DebugOptionsParser and seeing that it's never hit.

I'm now trying to come up with a minimal test case.

[seishun]

From dumpbin /all node_options.obj output:

                                                Symbol    Symbol
 Offset    Type              Applied To         Index     Name
 --------  ----------------  -----------------  --------  ------
 00000000  ADDR64            00000000 00000000      5219  ??__E?instance@EnvironmentOptionsParser@options_parser@node@@2V123@B@@YAXXZ (void __cdecl `dynamic initializer for 'public: static class node::options_parser::EnvironmentOptionsParser const node::options_parser::EnvironmentOptionsParser::instance''(void))
 00000008  ADDR64            00000000 00000000      521C  ??__E?instance@PerIsolateOptionsParser@options_parser@node@@2V123@B@@YAXXZ (void __cdecl `dynamic initializer for 'public: static class node::options_parser::PerIsolateOptionsParser const node::options_parser::PerIsolateOptionsParser::instance''(void))
 00000010  ADDR64            00000000 00000000      521F  ??__E?instance@PerProcessOptionsParser@options_parser@node@@2V123@B@@YAXXZ (void __cdecl `dynamic initializer for 'public: static class node::options_parser::PerProcessOptionsParser const node::options_parser::PerProcessOptionsParser::instance''(void))
 00000018  ADDR64            00000000 00000000      5208  ??__Ecli_options_mutex@per_process@node@@YAXXZ (void __cdecl node::per_process::`dynamic initializer for 'cli_options_mutex''(void))
 00000020  ADDR64            00000000 00000000      520F  ??__Ecli_options@per_process@node@@YAXXZ (void __cdecl node::per_process::`dynamic initializer for 'cli_options''(void))
 00000028  ADDR64            00000000 00000000      5216  ??__E?instance@DebugOptionsParser@options_parser@node@@2V123@B@@YAXXZ (void __cdecl `dynamic initializer for 'public: static class node::options_parser::DebugOptionsParser const node::options_parser::DebugOptionsParser::instance''(void))

I wonder if cli_options_mutex or cli_options is throwing it off somehow?

[addaleax]

@seishun I think this means that DebugOptionsParser is actually initialized last, right? If so, I think this is pretty clearly a compiler bug…

[seishun]

Correct, this is from the .CRT$XCU section, which defines the dynamic initialization order (CRT Initialization).

[seishun]

I'm now trying to come up with a minimal test case.

Too difficult, the bug disappears when removing random lines. Is there someone in the Visual Studio team we could ping here?

[refack]

Is there someone in the Visual Studio team we could ping here?

Andrew moved to Facebook. We can submit an issue to https://developercommunity.visualstudio.com/spaces/62/index.html they are much more responsive then they used to be.

[seishun]

Reported here: https://developercommunity.visualstudio.com/content/problem/432157/dynamic-initializers-out-of-order.html