src: GetLoadedLibraries routine under NativeSymbolDebuggingContext #24825

gireeshpunathil · 2018-12-04T09:15:16Z

Add a static function GetLoadedLibraries under NativeSymbolDebuggingContext abstraction that provides a list of shared objects - either the current process depended on or loaded.

Used in node-report, but given its independent existence and potential for re-use, placing it under NativeSymbolDebuggingContext API.

/cc @addaleax

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-12-04T09:15:18Z

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

src/debug_utils.h

src/debug_utils.cc

bnoordhuis

Gireesh, are you the original author or did you take this code (or parts of it) from somewhere else?

src/debug_utils.cc

gireeshpunathil · 2018-12-04T12:20:55Z

@bnoordhuis - I am not the original author of this; its concept come from https://github.com/nodejs/node-report then ran some churn through #22712 before it landed up here as a refactoring measure. #22712 's commit metadata lists all the original authors under Co-authored By: tag. Do you see anything more? thanks.

bnoordhuis

That's fine. I got a sense of déjà vu while reading the code but now I know where it's coming from.

src/debug_utils.cc

Add a static function GetLoadedLibraries under NativeSymbolDebuggingContext abstraction that provides a list of shared objects - either the current process depended on or loaded.

gireeshpunathil · 2018-12-05T15:32:42Z

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

Trott · 2018-12-05T19:58:30Z

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19239/

gireeshpunathil · 2018-12-06T04:49:18Z

Resume Build CI: https://ci.nodejs.org/job/node-test-commit/24016/

danbev · 2018-12-07T04:38:21Z

~~Re-run of failing node-test-commit-windows-fanned~~
Re-run of failing node-test-commit-windows-fanned

bzoz · 2018-12-11T10:28:39Z

This does not compile on Windows:

06:07:23 src\debug_utils.cc(403): error C2664: 'DWORD GetModuleFileNameExA(HANDLE,HMODULE,LPSTR,DWORD)': cannot convert argument 3 from 'WCHAR [260]' to 'LPSTR' [c:\workspace\node-compile-windows\node_lib.vcxproj]
06:07:23   src\debug_utils.cc(406): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast
06:07:23 src\debug_utils.cc(407): error C2664: 'void std::vector<std::string,std::allocator<_Kty>>::push_back(_Ty &&)': cannot convert argument 1 from 'WCHAR [260]' to 'const std::string &' [c:\workspace\node-compile-windows\node_lib.vcxproj]
06:07:23           with
06:07:23           [
06:07:23               _Kty=std::string,
06:07:23               _Ty=std::string
06:07:23           ]
06:07:23   src\debug_utils.cc(407): note: Reason: cannot convert from 'WCHAR [260]' to 'const std::string'
06:07:23   src\debug_utils.cc(407): note: No constructor could take the source type, or constructor overload resolution was ambiguous

bzoz · 2018-12-11T10:30:34Z

src/debug_utils.cc

+           i++) {
+        WCHAR module_name[MAX_PATH];
+        // Obtain and report the full pathname for each module
+        if (GetModuleFileNameEx(process_handle,


Maybe GetModuleFileNameExW to fix the compilation error?

thanks @bzoz . I have 2 options;

use TCHAR instead of wide characters: I am able to build it successfully with that.

use GetModuleFileNameExW and deal with the next error of converting the result into std::string through MultiByteToWideChar.

On an assumption that the module names wont be made with wide characters, do you think the second option is an overkill, and we can just go with TCHAR instead? pls let me know.

an assumption that the module names wont be made with wide characters

I’d definitely prefer not to make such an assumption…

sure, thanks @addaleax for the assertion. Pushed a fix that makes no assumptions on the module name format.

Use WCHAR for module names

bzoz · 2018-12-11T11:29:04Z

Resumed Windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/23184/ ✔️

gireeshpunathil · 2018-12-11T11:32:55Z

@bzoz - just wondering; does CI resume pick-up the latest commit? or build from where it failed?

from https://ci.nodejs.org/job/node-test-commit-windows-fanned/23184/parameters/ it looks like it re-runs from the old commit?

gireeshpunathil · 2018-12-11T12:39:33Z

I was wrong; it picked up the latest commit. thanks @bzoz !

danbev · 2018-12-14T04:43:55Z

Landed in 1c52dcc.

Add a static function GetLoadedLibraries under NativeSymbolDebuggingContext abstraction that provides a list of shared objects - either the current process depended on or loaded. PR-URL: #24825 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

the dl_iterate_phdr and its associated data structure in Linux are fully available in freebsd as well, so opening it up for freebsd means just opening up the platform specific identifiers. Refs: nodejs#24825 PR-URL: nodejs#25106 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]>

the dl_iterate_phdr and its associated data structure in Linux are fully available in freebsd as well, so opening it up for freebsd means just opening up the platform specific identifiers. Refs: #24825 PR-URL: #25106 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Add a static function GetLoadedLibraries under NativeSymbolDebuggingContext abstraction that provides a list of shared objects - either the current process depended on or loaded. PR-URL: nodejs#24825 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

the dl_iterate_phdr and its associated data structure in Linux are fully available in freebsd as well, so opening it up for freebsd means just opening up the platform specific identifiers. Refs: nodejs#24825 PR-URL: nodejs#25106 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]>

Add a static function GetLoadedLibraries under NativeSymbolDebuggingContext abstraction that provides a list of shared objects - either the current process depended on or loaded. PR-URL: #24825 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

the dl_iterate_phdr and its associated data structure in Linux are fully available in freebsd as well, so opening it up for freebsd means just opening up the platform specific identifiers. Refs: #24825 PR-URL: #25106 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 4, 2018

gireeshpunathil requested review from bnoordhuis and addaleax December 4, 2018 09:17

addaleax approved these changes Dec 4, 2018

View reviewed changes

src/debug_utils.h Outdated Show resolved Hide resolved

src/debug_utils.cc Show resolved Hide resolved

src/debug_utils.cc Outdated Show resolved Hide resolved

bnoordhuis reviewed Dec 4, 2018

View reviewed changes

bnoordhuis approved these changes Dec 4, 2018

View reviewed changes

src/debug_utils.cc Outdated Show resolved Hide resolved

src: add GetLoadedLibraries routine

db7154e

Add a static function GetLoadedLibraries under NativeSymbolDebuggingContext abstraction that provides a list of shared objects - either the current process depended on or loaded.

gireeshpunathil force-pushed the native_debug branch from 652355f to db7154e Compare December 4, 2018 13:21

joyeecheung approved these changes Dec 5, 2018

View reviewed changes

bzoz reviewed Dec 11, 2018

View reviewed changes

fixup: fix windows compiler error

d97d06a

Use WCHAR for module names

danbev closed this Dec 14, 2018

BethGriggs mentioned this pull request Dec 18, 2018

V11.5.0 proposal #25102

Merged

gireeshpunathil mentioned this pull request Dec 18, 2018

src: port GetLoadedLibraries for freebsd #25106

Merged

2 tasks

beevelop mentioned this pull request Dec 18, 2018

2018-12-18 Version 11.5.0 (Current) @BethGriggs beevelop/docker-android-nodejs#272

Closed

BethGriggs added the land-on-v10.x label Feb 12, 2019

BethGriggs mentioned this pull request Feb 12, 2019

v10.15.3 proposal #26063

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: GetLoadedLibraries routine under NativeSymbolDebuggingContext #24825

src: GetLoadedLibraries routine under NativeSymbolDebuggingContext #24825

gireeshpunathil commented Dec 4, 2018 •

edited

Loading

nodejs-github-bot commented Dec 4, 2018

bnoordhuis left a comment

gireeshpunathil commented Dec 4, 2018

bnoordhuis left a comment

gireeshpunathil commented Dec 5, 2018

Trott commented Dec 5, 2018

gireeshpunathil commented Dec 6, 2018

danbev commented Dec 7, 2018 •

edited

Loading

bzoz commented Dec 11, 2018

bzoz Dec 11, 2018

gireeshpunathil Dec 11, 2018

addaleax Dec 11, 2018

gireeshpunathil Dec 11, 2018

bzoz commented Dec 11, 2018 •

edited by danbev

Loading

gireeshpunathil commented Dec 11, 2018 •

edited

Loading

gireeshpunathil commented Dec 11, 2018

danbev commented Dec 14, 2018

src: GetLoadedLibraries routine under NativeSymbolDebuggingContext #24825

src: GetLoadedLibraries routine under NativeSymbolDebuggingContext #24825

Conversation

gireeshpunathil commented Dec 4, 2018 • edited Loading

Checklist

nodejs-github-bot commented Dec 4, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

gireeshpunathil commented Dec 4, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

gireeshpunathil commented Dec 5, 2018

Trott commented Dec 5, 2018

gireeshpunathil commented Dec 6, 2018

danbev commented Dec 7, 2018 • edited Loading

bzoz commented Dec 11, 2018

bzoz Dec 11, 2018

Choose a reason for hiding this comment

gireeshpunathil Dec 11, 2018

Choose a reason for hiding this comment

addaleax Dec 11, 2018

Choose a reason for hiding this comment

gireeshpunathil Dec 11, 2018

Choose a reason for hiding this comment

bzoz commented Dec 11, 2018 • edited by danbev Loading

gireeshpunathil commented Dec 11, 2018 • edited Loading

gireeshpunathil commented Dec 11, 2018

danbev commented Dec 14, 2018

gireeshpunathil commented Dec 4, 2018 •

edited

Loading

danbev commented Dec 7, 2018 •

edited

Loading

bzoz commented Dec 11, 2018 •

edited by danbev

Loading

gireeshpunathil commented Dec 11, 2018 •

edited

Loading