src,win: informative stack traces by refack · Pull Request #23822 · nodejs/node · GitHub

Refresh Win32SymbolDebuggingContext::LookupSymbol to use more APIs
Refs: https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-symbol-information-by-address
Refs: https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-undecorated-symbol-names

Before:

DEV D:\code\node>Debug\node.exe test\parallel\test-fs-stat-bigint.js
C:\WINDOWS\system32\cmd.exe - Debug\node.exe  test\parallel\test-fs-stat-bigint.js [37304]: d:\code\node\deps\gsl\gsl\gsl_assert:132: Assertion `unknown' failed.
 1: 00007FF7A3B9F67A
 2: 00007FF7A3B0A1B7
 3: 00007FF7A3B0A307
 4: 00007FF7A394BB25
 5: 00007FF7A394B844
 6: 00007FF7A394AC6A
 7: 00007FF7A394BF91
 8: 00007FF7A3A7C36B
 9: 00007FF7A480D165
10: 00007FF7A480BB5C
11: 00007FF7A480CE1B
12: 00007FF7A480C6C3
13: 0000039475106628

After (with PDB available):

DEV D:\code\node>Debug\node.exe test\parallel\test-fs-stat-bigint.js
C:\WINDOWS\system32\cmd.exe - Debug\node.exe  test\parallel\test-fs-stat-bigint.js [38392]: d:\code\node\deps\gsl\gsl\gsl_assert:132: Assertion `unknown' failed.
 1: 00007FF7A3B9F67A node::DumpBacktrace [d:\code\node\src\debug_utils.cc]:L237+138
 2: 00007FF7A3B0A1B7 node::Abort [d:\code\node\src\node.cc]:L1030+39
 3: 00007FF7A3B0A307 node::Assert [d:\code\node\src\node.cc]:L1049+295
 4: 00007FF7A394BB25 gsl::details::throw_exception<gsl::narrowing_error> [d:\code\node\deps\gsl\gsl\gsl_assert]:L137+149
 5: 00007FF7A394B844 gsl::narrow<double,unsigned __int64> [d:\code\node\deps\gsl\gsl\gsl_util]:L117+148
 6: 00007FF7A394AC6A node::FillStatsArray<double,v8::Float64Array> [d:\code\node\src\node_internals.h]:L328+634
 7: 00007FF7A394BF91 node::FillGlobalStatsArray [d:\code\node\src\node_internals.h]:L360+129
 8: 00007FF7A3A7C36B node::fs::LStat [d:\code\node\src\node_file.cc]:L985+1083
 9: 00007FF7A480D165 v8::internal::FunctionCallbackArguments::Call [d:\code\node\deps\v8\src\api-arguments-inl.h]:L120+469
10: 00007FF7A480BB5C v8::internal::`anonymous namespace'::HandleApiCallHelper<0> [d:\code\node\deps\v8\src\builtins\builtins-api.cc]:L111+684
11: 00007FF7A480CE1B v8::internal::Builtin_Impl_HandleApiCall [d:\code\node\deps\v8\src\builtins\builtins-api.cc]:L139+507
12: 00007FF7A480C6C3 v8::internal::Builtin_HandleApiCall [d:\code\node\deps\v8\src\builtins\builtins-api.cc]:L127+451
13: 000003BBE9486628

Fallback with no PDB loaded:

 1: 00007FF7A3B9F67A AES_cbc_encrypt+5220586
 2: 00007FF7A3B0A1B7 AES_cbc_encrypt+4609063
 3: 00007FF7A3B0A307 AES_cbc_encrypt+4609399
 4: 00007FF7A394BB25 AES_cbc_encrypt+2780565
 5: 00007FF7A394B844 AES_cbc_encrypt+2779828
 6: 00007FF7A394AC6A AES_cbc_encrypt+2776794
 7: 00007FF7A394BF91 AES_cbc_encrypt+2781697
 8: 00007FF7A3A7C36B AES_cbc_encrypt+4027867
 9: 00007FF7A480D165 AES_cbc_encrypt+18252757
10: 00007FF7A480BB5C AES_cbc_encrypt+18247116
11: 00007FF7A480CE1B AES_cbc_encrypt+18251915
12: 00007FF7A480C6C3 AES_cbc_encrypt+18250035
13: 0000039475106628

Checklist

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

CI: https://ci.nodejs.org/job/node-test-pull-request/18049/
CI: https://ci.nodejs.org/job/node-test-commit/22540/
/CC @nodejs/platform-windows @nodejs/diagnostics

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

Why not just take a FILE* as input parameter?

Polymorphism with the POSIX class:
https://github.com/nodejs/node/blob/0b565eb7f7404fb8774865826afba10c1919788f/src/debug_utils.cc#L253
I could move the cast to https://github.com/nodejs/node/blob/0b565eb7f7404fb8774865826afba10c1919788f/src/debug_utils.cc#L232-L235
(Now that I see it I could assume it's always stderr and eliminate the argument).

Since this is only used for printing diagnostic information, I don’t think an extra parameter is necessary at all; we can always just use stderr

Done

What does the ~= mean?

assign almost all fields... I'll rephrase
(I might have used more fields in an earlier imp)

Rephrased

snake_case for variables.

I wanted to keep the code as similar as possible to https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-symbol-information-by-address

Done

I feel this information about what you changed isn’t particularly useful. Both of the patches are fairly trivial. This also apply to the other places.

I'd rather keep the comment, AFAIK more comment never hurt...
Also they might be useful since the examples on MSDN change from time to time (for example they changed since the last time this code was lifted, and I had to reverse engineer the patches).

Also they might be useful since the examples on MSDN change from time to time

Wouldn’t that be a good reason to drop the comment, since the diff can change on both sides, and so the Patches: list can grow out of date?

I'll sleep on it.

Since MSDN does have a history, I'm thinking the comments might still be useful.
Anyway they are free, and IMHO more info is almost always better then missing info...
I did go over them to make sure they are uniform, and not too obfuscated.

nit: one space after DWORD.

Ack.

cc @gireeshpunathil There may be opportunities here to reuse some code for Windows backtraces w.r.t. node-report in core and this PR.

Can we put this back in the .cc file?

Done.
But may I ask why?

Since this is only used for printing diagnostic information, I don’t think an extra parameter is necessary at all; we can always just use stderr

When is #ifdef _DEBUG defined? Is it the same as our #ifdef DEBUG?

node/common.gypi

Line 109 in f1b9546

'defines': [ 'DEBUG', '_DEBUG', 'V8_ENABLE_CHECKS' ],

I picked it arbitrarily... I'll change

Since I don’t know much about the Windows debugging stuff … how common would these errors be? i.e. do we only print them in debug mode because they would otherwise appear relatively frequently?

i.e. do we only print them in debug mode because they would otherwise appear relatively frequently?

Yes very frequent. If the *.pdb is not available, or is partial. Also the last (deepest) frame almost always fails to resolve.

Done /s/_DEBUG/DEBUG

DWORD64 dw_address

Ack.
But the type stated explicitly on the rhs.
And the name is copied from the example, which I'd rather not change...

Ack.

New CI: https://ci.nodejs.org/job/node-test-commit/22567/
https://ci.nodejs.org/job/node-test-commit/22575/

i.e. do we only print them in debug mode because they would otherwise appear relatively frequently?

Yes very frequent. If the *.pdb is not available, or is partial. Also the last (deepest) frame almost always fails to resolve.

Debug mode exe:

Example output with .pdb present:

DEV D:\code\node>Debug\node.exe test\parallel\test-fs-stat-bigint.js
C:\WINDOWS\system32\cmd.exe - Debug\node.exe  test\parallel\test-fs-stat-bigint.js [37716]: d:\code\node\src\node_file.cc:962: Assertion `(argc) >= (11)' failed.
 1: 00007FF61F72EE8A node::DumpBacktrace+138 [d:\code\node\src\debug_utils.cc]:L254
 2: 00007FF61F6999C7 node::Abort+39 [d:\code\node\src\node.cc]:L1030
 3: 00007FF61F699B17 node::Assert+295 [d:\code\node\src\node.cc]:L1049
 4: 00007FF61F60B7AC node::fs::LStat+108 [d:\code\node\src\node_file.cc]:L962
 5: 00007FF62039C925 v8::internal::FunctionCallbackArguments::Call+469 [d:\code\node\deps\v8\src\api-arguments-inl.h]:L120
 6: 00007FF62039B31C v8::internal::`anonymous namespace'::HandleApiCallHelper<0>+684 [d:\code\node\deps\v8\src\builtins\builtins-api.cc]:L111
 7: 00007FF62039C5DB v8::internal::Builtin_Impl_HandleApiCall+507 [d:\code\node\deps\v8\src\builtins\builtins-api.cc]:L139
 8: 00007FF62039BE83 v8::internal::Builtin_HandleApiCall+451 [d:\code\node\deps\v8\src\builtins\builtins-api.cc]:L127
SymGetLineFromAddr64 returned error : 126
SymFromAddr returned error : 126
 9: 000003B89AE86628

Without .pdb

DEV D:\code\node>Debug\node.exe test\parallel\test-fs-stat-bigint.js
C:\WINDOWS\system32\cmd.exe - Debug\node.exe  test\parallel\test-fs-stat-bigint.js [37020]: d:\code\node\src\node_file.cc:962: Assertion `(argc) >= (11)' failed.
SymGetLineFromAddr64 returned error : 487
 1: 00007FF61F72EE8A AES_cbc_encrypt+5218554
SymGetLineFromAddr64 returned error : 487
 2: 00007FF61F6999C7 AES_cbc_encrypt+4607031
SymGetLineFromAddr64 returned error : 487
 3: 00007FF61F699B17 AES_cbc_encrypt+4607367
SymGetLineFromAddr64 returned error : 487
 4: 00007FF61F60B7AC AES_cbc_encrypt+4024860
SymGetLineFromAddr64 returned error : 487
 5: 00007FF62039C925 AES_cbc_encrypt+18250645
SymGetLineFromAddr64 returned error : 487
 6: 00007FF62039B31C AES_cbc_encrypt+18245004
SymGetLineFromAddr64 returned error : 487
 7: 00007FF62039C5DB AES_cbc_encrypt+18249803
SymGetLineFromAddr64 returned error : 487
 8: 00007FF62039BE83 AES_cbc_encrypt+18247923
SymGetLineFromAddr64 returned error : 126
SymFromAddr returned error : 126
 9: 00000074D8C06628

All comments addressed, PTAL.

I am not a fan of this type of comment: What happens is obvious in both branches, and the branches themselves are short. (And I think we have a convention of ending comments with a period.)

This code was lifter verbatim from https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-symbol-information-by-address
I would rather keep changes to minimon, for future reverse engineering.

I am still not sure whether this is going to help us at some point, I suspect it won't.

Comment are free.
Personally I have not been in a situation where I said "I wish there was less comments"

I tend to agree, but someone will either change our code or the original, and updating the comment will mostly be a maintenance burden at that point from my perspective.

LGTM either way, even though I don't see much value in some of the comments.

Refresh `Win32SymbolDebuggingContext::LookupSymbol` to use more APIs PR-URL: nodejs#23822 Refs: https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-symbol-information-by-address Refs: https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-undecorated-symbol-names Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Refresh `Win32SymbolDebuggingContext::LookupSymbol` to use more APIs PR-URL: #23822 Refs: https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-symbol-information-by-address Refs: https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-undecorated-symbol-names Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Notable changes: * child_process: * All child processes will again open up a new Window on Windows by default. [#24034](#24034) * deps: * A new and fast experimental HTTP parser (`llhttp`) is now supported. [#24059](#24059) * **Windows** * A crashing process will now show the names of stack frames if the node.pdb file is available. [#23822](#23822) * Added new collaborators: * [oyyd](https://github.com/oyyd) - Ouyang Yadong. [#24300](#24300) * [psmarshall](https://github.com/psmarshall) - Peter Marshall. [#24170](#24170) * [shisama](https://github.com/shisama) - Masashi Hirano. [#24136](#24136)

Notable changes: * deps: * A new experimental HTTP parser (`llhttp`) is now supported. #24059 * timers: * Fixed an issue that could cause setTimeout to stop working as expected. #24322 * Windows * A crashing process will now show the names of stack frames if the node.pdb file is available. #23822 * Continued effort to improve the installer's new stage that installs native build tools. #23987, #24348 * child_process: * On Windows the `windowsHide` option default was restored to `false`. This means `detached` child processes and GUI apps will once again start in a new window. #24034 * Added new collaborators: * [oyyd](https://github.com/oyyd) - Ouyang Yadong. #24300 * [psmarshall](https://github.com/psmarshall) - Peter Marshall. #24170 * [shisama](https://github.com/shisama) - Masashi Hirano. #24136

Notable changes: * deps: * A new experimental HTTP parser (`llhttp`) is now supported. #24059 * timers: * Fixed an issue that could cause setTimeout to stop working as expected. #24322 * Windows * A crashing process will now show the names of stack frames if the node.pdb file is available. #23822 * Continued effort to improve the installer's new stage that installs native build tools. #23987, #24348 * child_process: * On Windows the `windowsHide` option default was restored to `false`. This means `detached` child processes and GUI apps will once again start in a new window. #24034 * Added new collaborators: * [oyyd](https://github.com/oyyd) - Ouyang Yadong. #24300 * [psmarshall](https://github.com/psmarshall) - Peter Marshall. #24170 * [shisama](https://github.com/shisama) - Masashi Hirano. #24136 PR-URL: #24350

Refresh `Win32SymbolDebuggingContext::LookupSymbol` to use more APIs PR-URL: #23822 Refs: https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-symbol-information-by-address Refs: https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-undecorated-symbol-names Reviewed-By: Tobias Nießen <tniessen@tnie.de>

This does not land cleanly in v8.x LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

Refresh `Win32SymbolDebuggingContext::LookupSymbol` to use more APIs PR-URL: #23822 Refs: https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-symbol-information-by-address Refs: https://docs.microsoft.com/en-us/windows/desktop/Debug/retrieving-undecorated-symbol-names Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Notable changes: * deps: * upgrade to c-ares v1.15.0 (Ben Noordhuis) #23854 * Windows: * A crashing process will now show the names of stack frames if the node.pdb file is available. (Refael Ackermann) #23822 * Added new collaborators: * Peter Marshall. #24170 * Masashi Hirano. #24136 PR-URL: #24727

Notable changes: * deps: * upgrade to c-ares v1.15.0 (Ben Noordhuis) nodejs#23854 * Windows: * A crashing process will now show the names of stack frames if the node.pdb file is available. (Refael Ackermann) nodejs#23822 * Added new collaborators: * Peter Marshall. nodejs#24170 * Masashi Hirano. nodejs#24136 PR-URL: nodejs#24727

refack requested review from addaleax, bnoordhuis, bzoz and joaocgreis October 22, 2018 19:41

refack self-assigned this Oct 22, 2018

refack added c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform. post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. labels Oct 22, 2018

TimothyGu reviewed Oct 23, 2018

View reviewed changes

addaleax reviewed Oct 23, 2018

View reviewed changes

refack force-pushed the win-native-stacks branch from bf00718 to 073a5be Compare October 23, 2018 13:03

refack requested review from joyeecheung and tniessen October 28, 2018 22:26

tniessen reviewed Oct 28, 2018

View reviewed changes

tniessen approved these changes Oct 29, 2018

View reviewed changes

refack force-pushed the win-native-stacks branch from 073a5be to 247b513 Compare October 30, 2018 01:45

refack merged commit 247b513 into nodejs:master Oct 30, 2018

refack removed their assignment Oct 30, 2018

refack removed request for bnoordhuis, bzoz, joaocgreis and joyeecheung October 30, 2018 01:54

refack deleted the win-native-stacks branch October 30, 2018 01:54

BridgeAR mentioned this pull request Nov 14, 2018

Release proposal: v11.2 #24350

Merged

BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label Nov 15, 2018

MylesBorins added backport-requested-v8.x labels Nov 27, 2018

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

src,win: informative stack traces by refack · Pull Request #23822 · nodejs/node · GitHub

Uh oh!

Uh oh!

src,win: informative stack traces - #23822

src,win: informative stack traces#23822refack merged 1 commit intonodejs:masternodejs/node:masterfrom refack:win-native-stacksrefack/node:win-native-stacksCopy head branch name to clipboard

Conversation

refack commented Oct 22, 2018

Checklist

Uh oh!

refack commented Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richardlau commented Oct 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Oct 23, 2018

src,win: informative stack traces#23822
refack merged 1 commit into
nodejs:masterfrom
refack:win-native-stacks

refack commented Oct 22, 2018 •
edited
Loading

addaleax Oct 23, 2018 •
edited
Loading

refack Oct 23, 2018 •
edited
Loading

refack commented Oct 23, 2018 •
edited
Loading