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.

Sign up for GitHub

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

Jump to bottom

src: mitigate MSVC dynamic initialization bug #25596

Merged
merged 1 commit into from
Jan 31, 2019
Merged

src: mitigate MSVC dynamic initialization bug #25596

merged 1 commit into from
Jan 31, 2019

Conversation

refack
Copy link
Contributor

@refack refack commented Jan 20, 2019 •
edited
Loading

Fixes: #25593
Refs: https://developercommunity.visualstudio.com/content/problem/432157/dynamic-initializers-out-of-order.html

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2306/pipeline

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 20, 2019
@refack refack mentioned this pull request Jan 20, 2019
Closed
@refack
Copy link
Contributor Author

refack commented Jan 20, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/20217/

@addaleax
Copy link
Member

The change LGTM, but as @seishun says in the other thread – it shouldn’t do anything at all? Along the lines of #25593 (comment): This shouldn’t be an issue when the definitions are all in one file, and it’s likely that something else is going on here…

@seishun
Copy link
Contributor

seishun commented Jan 20, 2019

Let's not make magical workarounds. We should figure out why this is happening and (most likely) submit a bug report to Visual Studio.

@refack
Copy link
Contributor Author

refack commented Jan 20, 2019

I agree this is "magical". AFAICR that is why I didn't submit a PR with this till now.
But, it fixes the problem at hand so ¯\(ツ)/¯

@seishun
Copy link
Contributor

seishun commented Jan 20, 2019

If the underlying issue in the compiler is not fixed then it might break in another way in the next version.

bzoz
bzoz approved these changes Jan 22, 2019
View reviewed changes
Copy link
Contributor

@bzoz bzoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until we get new VS with the bug fixed, we need to do something to make Node work again.

@seishun
Copy link
Contributor

seishun commented Jan 22, 2019

we need to do something to make Node work again

FYI it works - just not the debug build.

@seishun
Copy link
Contributor

seishun commented Jan 22, 2019

In any case, the commit message should explain what the problem is and how it's being mitigated.

@seishun
Copy link
Contributor

seishun commented Jan 23, 2019

The problem is now "Under Investigation" so maybe it will be fixed very soon.

@addaleax
Copy link
Member

@refack I think with an updated commit message this should be ready to go 👍

(And I’m assuming we want to merge this even if it gets fixed in the compiler itself)

@seishun
Copy link
Contributor

seishun commented Jan 23, 2019

(And I’m assuming we want to merge this even if it gets fixed in the compiler itself)

I don't think so - I find it slightly more readable when an instance definition follows the class definition.

@refack
Copy link
Contributor Author

refack commented Jan 23, 2019 •
edited
Loading

I've tested master with the VS2019 preview (CL.EXE version 19.20.27027.1), and it seems like the compiler issue does not reproduce...
But IMO this PR is low impact enough to meanwhile be a beneficial workaround (Node 10 & 11 are built with VS2017).

@refack refack force-pushed the fix-static-order-windows branch from a7ad6a8 to 0c256f1 Compare January 23, 2019 19:37
@refack
Copy link
Contributor Author

refack commented Jan 23, 2019 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/20292/
Resume: https://ci.nodejs.org/job/node-test-pull-request/20294/
Resume: https://ci.nodejs.org/job/node-test-pull-request/20469/
Resume: https://ci.nodejs.org/job/node-test-commit/25475/

@refack refack self-assigned this Jan 23, 2019
@refack refack added the windows Issues and PRs related to the Windows platform. label Jan 23, 2019
@refack refack changed the title src: mitigate a "static initialization order problem" on Windows src: mitigate MSVC dynamic initialization bug Jan 23, 2019
@refack refack force-pushed the fix-static-order-windows branch from 0c256f1 to 31f4c4f Compare January 30, 2019 23:14
@refack
src: workaround MSVC compiler bug
e3c4b67 
PR-URL: nodejs#25596
Fixes: nodejs#25593
Refs: https://developercommunity.visualstudio.com/content/problem/432157/dynamic-initializers-out-of-order.html
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@refack refack force-pushed the fix-static-order-windows branch from 31f4c4f to e3c4b67 Compare January 31, 2019 23:48
@refack refack merged commit e3c4b67 into nodejs:master Jan 31, 2019
@refack refack deleted the fix-static-order-windows branch February 1, 2019 00:01
addaleax pushed a commit that referenced this pull request Feb 1, 2019
@refack @addaleax
src: workaround MSVC compiler bug
998cea5 
PR-URL: #25596
Fixes: #25593
Refs: https://developercommunity.visualstudio.com/content/problem/432157/dynamic-initializers-out-of-order.html
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Closed
@refack refack mentioned this pull request Feb 24, 2019
Merged
2 tasks
@refack refack removed their assignment Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzoz bzoz bzoz approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Debug build doesn't work on Windows
5 participants
@refack @nodejs-github-bot @addaleax @seishun @bzoz