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: implement backtrace-on-abort for windows #16951

Closed
wants to merge 2 commits into from
Closed

src: implement backtrace-on-abort for windows #16951

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@addaleax addaleax added wip Issues and PRs that are still a work in progress. windows Issues and PRs related to the Windows platform. labels Nov 11, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform. labels Nov 11, 2017
addaleax
addaleax commented Nov 11, 2017
View reviewed changes
src/backtrace_win32.cc Outdated
@@ -1,8 +1,41 @@
#include "node.h"
#define _WIN32_WINNT 0x0600
Copy link
Member Author

Choose a reason for hiding this comment

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

@nodejs/platform-windows What’s the right way to do this? That’s basically the only question I have left here…

Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually an interesting question. We have multiple version targeting macros:

Copy link
Contributor

Choose a reason for hiding this comment

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

(shirt-return is not the right key-chord. it's not new line, it's Submit)

Copy link
Contributor

@refack refack Nov 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

(grrrr, shift-return again)

  • From uv

    node/deps/uv/include/uv-win.h

    Lines 22 to 24 in 766cd1f

    #ifndef _WIN32_WINNT
    # define _WIN32_WINNT 0x0502
    #endif
  • Our:

    node/src/node.h

    Lines 42 to 47 in a36aa04

    // This should be defined in make system.
    // See issue https://github.com/joyent/node/issues/1236
    #if defined(__MINGW32__) || defined(_MSC_VER)
    #ifndef _WIN32_WINNT
    # define _WIN32_WINNT 0x0501
    #endif

tl;dr this should be injected by the build system, so you should ifndef around it. Also semantically it's used for targeting a minimum platform requirements, and 0x0600 is anyway our platform minimum.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not at my laptop rn but if it's really just ifdef'ing it feel free to push that to this branch

@addaleax
Copy link
Member Author

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

refack
refack approved these changes Nov 11, 2017
View reviewed changes
src/backtrace_win32.cc Outdated
@@ -1,8 +1,41 @@
#include "node.h"
#define _WIN32_WINNT 0x0600
Copy link
Contributor

@refack refack Nov 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

(grrrr, shift-return again)

  • From uv

    node/deps/uv/include/uv-win.h

    Lines 22 to 24 in 766cd1f

    #ifndef _WIN32_WINNT
    # define _WIN32_WINNT 0x0502
    #endif
  • Our:

    node/src/node.h

    Lines 42 to 47 in a36aa04

    // This should be defined in make system.
    // See issue https://github.com/joyent/node/issues/1236
    #if defined(__MINGW32__) || defined(_MSC_VER)
    #ifndef _WIN32_WINNT
    # define _WIN32_WINNT 0x0501
    #endif

tl;dr this should be injected by the build system, so you should ifndef around it. Also semantically it's used for targeting a minimum platform requirements, and 0x0600 is anyway our platform minimum.

jasnell
jasnell approved these changes Nov 12, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@addaleax
Copy link
Member Author

@refack Please take a look at the squashed commit and see if that’s what you had in mind.

I don’t think this would be semver-major because it just aligns the actual values with what our documentation says but, uh, really not enough Windows knowledge to tell that for sure.

CI: https://ci.nodejs.org/job/node-test-commit/13941/

@addaleax addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. and removed wip Issues and PRs that are still a work in progress. labels Nov 12, 2017
@addaleax
Copy link
Member Author

Landed in 7d8fe7d

@addaleax addaleax closed this Nov 15, 2017
@addaleax addaleax deleted the backtrace-windows branch November 15, 2017 18:50
addaleax added a commit that referenced this pull request Nov 15, 2017
@addaleax
src: implement backtrace-on-abort for windows
7d8fe7d 
PR-URL: #16951
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@refack
Copy link
Contributor

refack commented Nov 15, 2017

Just saw this. It looks 💯 to me.

MylesBorins pushed a commit that referenced this pull request Dec 11, 2017
@addaleax @MylesBorins
src: implement backtrace-on-abort for windows
6f805c6 
PR-URL: #16951
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@addaleax @gibfahn
src: implement backtrace-on-abort for windows
728e57b 
PR-URL: #16951
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

This was landed on v8.x-staging, please let us know if it should be backed out.

Does it make sense to land on v6.x? If so please submit a manual backport, if not please change the label appropriately

@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@addaleax @gibfahn
src: implement backtrace-on-abort for windows
7db8f01 
PR-URL: #16951
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. 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.
7 participants
@addaleax @refack @MylesBorins @jasnell @cjihrig @gibfahn @nodejs-github-bot