src: find .text section using dl_iterate_phdr by gabrielschulhof · Pull Request #32244 · nodejs/node · GitHub

Use dl_iterate_phdr(3) to find the mapping containing
__node_text_start instead of parsing /proc/self/maps.

Signed-off-by: @gabrielschulhof

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/29798/

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

LGTM % some minor (mostly style) issues.

Just checking, is end inclusive or exclusive?

Aside: it looks like there's no real need for the dl_params->reference_sym field. This also works, wouldn't it?

auto reference_sym = reinterpret_cast<uintptr_t>(&__node_text_start);
if (reference_sym >= start && reference_sym <= end) {
  // ...
}

@bnoordhuis the reference_sym field is being used both inside the dl_iterate_phdr callback and outside of it. I thought it best not to assign reinterpret_cast<uintptr_t>(&__node_text_start); to two different variables because, if the symbol changes in the future, they might get out of sync.

For legibility, it'd be better to hoist the lambda out of the function call:

auto callback = [](dl_phdr_info* info, size_t, void* data) -> int {
  // ...
};

if (1 == dl_iterate_phdr(callback, &dl_params)) {
  // ...
}

@bnoordhuis I added some more sanity checks to ensure that we create a region that has proper offsets and > 0 page count.

... or rather, that we consider the region to have been found only if it has proper offsets and >0 page count.

@bnoordhuis I addressed your review comments and added some sanity checks wrt. your comment regarding inclusive/exclusive comparison.

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

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

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

Use `dl_iterate_phdr(3)` to find the mapping containing `__node_text_start` instead of parsing `/proc/self/maps`. Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>

Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>

Rebased in the hope that if fixes the "node ASAN" test.

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

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

@nodejs/collaborators sorry about the spam! Can this land without the "node ASAN" test? AFAICT it's failing for a lot of PRs.

https://github.com/nodejs/node/actions?query=workflow%3A%22node+ASAN%22

sorry about the spam! Can this land without the "node ASAN" test? AFAICT it's failing for a lot of PRs.

Yes. 👍

@nodejs/collaborators sorry about the spam! Can this land without the "node ASAN" test? AFAICT it's failing for a lot of PRs.

We dealt with Node ASAN fail in #32116. Not sure we should cherrypick the commit to master, since the PR is pretty ready to launch.

Also +1 for this PR to launch.

Use `dl_iterate_phdr(3)` to find the mapping containing `__node_text_start` instead of parsing `/proc/self/maps`. Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl> PR-URL: #32244 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: David Carlier <devnexen@gmail.com>

Landed in 25cb855.

Use `dl_iterate_phdr(3)` to find the mapping containing `__node_text_start` instead of parsing `/proc/self/maps`. Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl> PR-URL: #32244 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: David Carlier <devnexen@gmail.com>

Depends on the large pages change to land on v12.x

Use `dl_iterate_phdr(3)` to find the mapping containing `__node_text_start` instead of parsing `/proc/self/maps`. Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl> PR-URL: nodejs#32244 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: David Carlier <devnexen@gmail.com>

Use `dl_iterate_phdr(3)` to find the mapping containing `__node_text_start` instead of parsing `/proc/self/maps`. Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl> PR-URL: #32244 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: David Carlier <devnexen@gmail.com>

gabrielschulhof requested review from bnoordhuis, devnexen and lundibundi March 13, 2020 06:48

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

gabrielschulhof force-pushed the large-pages-use-dl_iterate_phdr branch from efd3932 to b53d3cd Compare March 13, 2020 07:01

bnoordhuis approved these changes Mar 13, 2020

View reviewed changes

gabrielschulhof commented Mar 13, 2020

View reviewed changes

devnexen approved these changes Mar 13, 2020

View reviewed changes

Gabriel Schulhof and others added 6 commits March 13, 2020 14:46

src: find .text section using dl_iterate_phdr

31c03a2

Use `dl_iterate_phdr(3)` to find the mapping containing `__node_text_start` instead of parsing `/proc/self/maps`. Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>

Update src/large_pages/node_large_page.cc

f6531aa

Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>

Update src/large_pages/node_large_page.cc

5932d05

Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>

address review comments

617e601

introduce additional sanity checks on the range

ba539ef

ensure _GNU_SOURCE is defined instead of defining it unconditionally

2d6be5e

gabrielschulhof force-pushed the large-pages-use-dl_iterate_phdr branch from af31134 to 2d6be5e Compare March 13, 2020 21:46

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 15, 2020

gabrielschulhof closed this Mar 16, 2020

gabrielschulhof deleted the large-pages-use-dl_iterate_phdr branch March 16, 2020 05:43

MylesBorins mentioned this pull request Mar 19, 2020

v13.12.0 proposal #32376

Merged

targos added the backport-blocked-v12.x label Apr 22, 2020

targos removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. backport-blocked-v12.x labels Apr 25, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

src: find .text section using dl_iterate_phdr by gabrielschulhof · Pull Request #32244 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: find .text section using dl_iterate_phdr - #32244

src: find .text section using dl_iterate_phdr#32244gabrielschulhof wants to merge 6 commits intonodejs:masternodejs/node:masterfrom gabrielschulhof:large-pages-use-dl_iterate_phdrgabrielschulhof/node:large-pages-use-dl_iterate_phdrCopy head branch name to clipboard

Conversation

gabrielschulhof commented Mar 13, 2020

Checklist

Uh oh!

nodejs-github-bot commented Mar 13, 2020

Uh oh!

nodejs-github-bot commented Mar 13, 2020

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnoordhuis Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gabrielschulhof Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof Mar 13, 2020

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Mar 13, 2020

Uh oh!

nodejs-github-bot commented Mar 13, 2020

Uh oh!

nodejs-github-bot commented Mar 13, 2020

Uh oh!

nodejs-github-bot commented Mar 13, 2020

Uh oh!

gabrielschulhof commented Mar 13, 2020

Uh oh!

nodejs-github-bot commented Mar 13, 2020

Uh oh!

nodejs-github-bot commented Mar 14, 2020

Uh oh!

gabrielschulhof commented Mar 15, 2020

Uh oh!

gabrielschulhof commented Mar 15, 2020

Uh oh!

addaleax commented Mar 15, 2020

Uh oh!

gengjiawen commented Mar 16, 2020

Uh oh!

gabrielschulhof commented Mar 16, 2020

Uh oh!

targos commented Apr 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

src: find .text section using dl_iterate_phdr#32244
gabrielschulhof wants to merge 6 commits into
nodejs:masterfrom
gabrielschulhof:large-pages-use-dl_iterate_phdr