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

Debug build doesn't work on Windows #25593

Closed
seishun opened this issue Jan 20, 2019 · 16 comments · Fixed by #26280 or #25596
Closed

Debug build doesn't work on Windows #25593

seishun opened this issue Jan 20, 2019 · 16 comments · Fixed by #26280 or #25596
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.

Comments

@seishun
Copy link
Contributor

seishun commented Jan 20, 2019

  • 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 seishun added the windows Issues and PRs related to the Windows platform. label Jan 20, 2019
@seishun
Copy link
Contributor Author

seishun commented Jan 20, 2019

cc @nodejs/platform-windows

@refack
Copy link
Contributor

refack commented Jan 20, 2019

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.

const DebugOptionsParser DebugOptionsParser::instance;

@seishun
Copy link
Contributor Author

seishun commented Jan 20, 2019

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

@refack
Copy link
Contributor

refack commented Jan 20, 2019

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: #25596

@seishun
Copy link
Contributor Author

seishun commented Jan 20, 2019

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

@refack
Copy link
Contributor

refack commented Jan 20, 2019

they are all in the same translation unit here.

IIUC, not necessarily since they are used in two other places:

options_parser::PerIsolateOptionsParser::instance.Parse(

and
options_parser::PerProcessOptionsParser::instance.Parse(

@seishun
Copy link
Contributor Author

seishun commented Jan 20, 2019

That's a usage, not a definition.

@refack
Copy link
Contributor

refack commented Jan 20, 2019

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

const DebugOptionsParser DebugOptionsParser::instance;
isn't optimized away?

@addaleax
Copy link
Member

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
Copy link
Contributor Author

seishun commented Jan 20, 2019

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
Copy link
Contributor Author

seishun commented Jan 20, 2019

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
Copy link
Member

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

@seishun
Copy link
Contributor Author

seishun commented Jan 20, 2019

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

@seishun
Copy link
Contributor Author

seishun commented Jan 20, 2019

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
Copy link
Contributor

refack commented Jan 20, 2019

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
Copy link
Contributor Author

seishun commented Jan 21, 2019

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

@Fishrock123 Fishrock123 added the build Issues and PRs related to build files or the CI. label Jan 24, 2019
refack added a commit to refack/node that referenced this issue Jan 31, 2019
refack added a commit to refack/node that referenced this issue Mar 4, 2019
refack added a commit to refack/node that referenced this issue Mar 4, 2019
* header explicit usage, order, and reduce use of `*-inl.h`
* pointer -> const reference when possible
* no variable recyclicng
* `std::begin/end` prefered over `instance.begin/end`
* `USE` for explicit unused resaults

PR-URL: nodejs#26280
Fixes: nodejs#25593
Reviewed-By: Joyee Cheung <[email protected]>
BridgeAR pushed a commit that referenced this issue Mar 4, 2019
BridgeAR pushed a commit that referenced this issue Mar 4, 2019
* header explicit usage, order, and reduce use of `*-inl.h`
* pointer -> const reference when possible
* no variable recyclicng
* `std::begin/end` prefered over `instance.begin/end`
* `USE` for explicit unused resaults

PR-URL: #26280
Fixes: #25593
Reviewed-By: Joyee Cheung <[email protected]>
refack added a commit to refack/node that referenced this issue Mar 14, 2019
refack added a commit to refack/node that referenced this issue Mar 14, 2019
* header explicit usage, order, and reduce use of `*-inl.h`
* pointer -> const reference when possible
* no variable recyclicng
* `std::begin/end` prefered over `instance.begin/end`
* `USE` for explicit unused resaults

PR-URL: nodejs#26280
Fixes: nodejs#25593
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this issue Mar 28, 2019
Backport-PR-URL: #26649
PR-URL: #26280
Fixes: #25593
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this issue Mar 28, 2019
* header explicit usage, order, and reduce use of `*-inl.h`
* pointer -> const reference when possible
* no variable recyclicng
* `std::begin/end` prefered over `instance.begin/end`
* `USE` for explicit unused resaults

Backport-PR-URL: #26649
PR-URL: #26280
Fixes: #25593
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this issue Mar 30, 2019
* header explicit usage, order, and reduce use of `*-inl.h`
* pointer -> const reference when possible
* no variable recyclicng
* `std::begin/end` prefered over `instance.begin/end`
* `USE` for explicit unused resaults

Backport-PR-URL: #26649
PR-URL: #26280
Fixes: #25593
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
4 participants