Skip to content

src: mitigate MSVC dynamic initialization bug#25596

Merged
refack merged 1 commit into
nodejs:masterfrom
refack:fix-static-order-windows
Jan 31, 2019
Merged

src: mitigate MSVC dynamic initialization bug#25596
refack merged 1 commit into
nodejs:masterfrom
refack:fix-static-order-windows

Conversation

@refack

@refack refack commented Jan 20, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot commented Jan 20, 2019

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

refack commented Jan 20, 2019

Copy link
Copy Markdown
Contributor Author

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

@addaleax

addaleax commented Jan 20, 2019

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

seishun commented Jan 20, 2019

Copy link
Copy Markdown
Contributor

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

refack commented Jan 20, 2019

Copy link
Copy Markdown
Contributor Author

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

seishun commented Jan 20, 2019

Copy link
Copy Markdown
Contributor

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

@bzoz bzoz left a comment

Copy link
Copy Markdown
Contributor

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

seishun commented Jan 22, 2019

Copy link
Copy Markdown
Contributor

we need to do something to make Node work again

FYI it works - just not the debug build.

@seishun

seishun commented Jan 22, 2019

Copy link
Copy Markdown
Contributor

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

@seishun

seishun commented Jan 23, 2019

Copy link
Copy Markdown
Contributor

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

@addaleax

addaleax commented Jan 23, 2019

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

seishun commented Jan 23, 2019

Copy link
Copy Markdown
Contributor

(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

refack commented Jan 23, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

refack commented Jan 23, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@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

@addaleax addaleax addaleax approved these changes

+1 more reviewer

@bzoz bzoz bzoz approved these changes

Reviewers whose approvals may not affect merge requirements

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