src: find .text section using dl_iterate_phdr #32244

gabrielschulhof · 2020-03-13T06:48:32Z

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

nodejs-github-bot · 2020-03-13T06:49:51Z

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

nodejs-github-bot · 2020-03-13T07:04:22Z

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

bnoordhuis

LGTM % some minor (mostly style) issues.

src/large_pages/node_large_page.cc

bnoordhuis · 2020-03-13T09:01:01Z

src/large_pages/node_large_page.cc

+          uintptr_t end = start + phdr->p_memsz;
+
+          if (dl_params->reference_sym >= start &&
+              dl_params->reference_sym <= end) {


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.

bnoordhuis · 2020-03-13T09:03:05Z

src/large_pages/node_large_page.cc

+      }
+    }
+    return 0;
+  }, &dl_params) == 1) {


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)) { // ... }

src/large_pages/node_large_page.cc

gabrielschulhof · 2020-03-13T18:20:57Z

src/large_pages/node_large_page.cc

+    if (dl_params.start < dl_params.end) {
+      char* from = reinterpret_cast<char*>(hugepage_align_up(dl_params.start));
+      char* to = reinterpret_cast<char*>(hugepage_align_down(dl_params.end));
+      if (from < to) {
+        size_t pagecount = (to - from) / hps;
+        if (pagecount > 0) {
+          nregion.found_text_region = true;
+          nregion.from = from;
+          nregion.to = to;
+          nregion.total_hugepages = pagecount;
+        }
+      }
+    }


@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.

gabrielschulhof · 2020-03-13T18:22:25Z

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

nodejs-github-bot · 2020-03-13T18:23:36Z

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

nodejs-github-bot · 2020-03-13T20:17:05Z

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

nodejs-github-bot · 2020-03-13T20:25:24Z

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>

gabrielschulhof · 2020-03-13T21:47:21Z

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

nodejs-github-bot · 2020-03-13T21:48:15Z

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

nodejs-github-bot · 2020-03-14T07:06:27Z

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

gabrielschulhof · 2020-03-15T18:42:25Z

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

gabrielschulhof · 2020-03-15T18:43:29Z

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

addaleax · 2020-03-15T18:45:45Z

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

Yes. 👍

gengjiawen · 2020-03-16T00:28:15Z

@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>

gabrielschulhof · 2020-03-16T05:42:51Z

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>

targos · 2020-04-22T16:01:28Z

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 pending requests for changes, 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 pending requests for changes, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: find .text section using dl_iterate_phdr #32244

src: find .text section using dl_iterate_phdr #32244

gabrielschulhof commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

bnoordhuis left a comment

bnoordhuis Mar 13, 2020

gabrielschulhof Mar 13, 2020

bnoordhuis Mar 13, 2020

gabrielschulhof Mar 13, 2020

gabrielschulhof Mar 13, 2020

gabrielschulhof commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

gabrielschulhof commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 14, 2020

gabrielschulhof commented Mar 15, 2020

gabrielschulhof commented Mar 15, 2020

addaleax commented Mar 15, 2020

gengjiawen commented Mar 16, 2020

gabrielschulhof commented Mar 16, 2020

targos commented Apr 22, 2020

src: find .text section using dl_iterate_phdr #32244

src: find .text section using dl_iterate_phdr #32244

Conversation

gabrielschulhof commented Mar 13, 2020

Checklist

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Mar 13, 2020

Choose a reason for hiding this comment

gabrielschulhof Mar 13, 2020

Choose a reason for hiding this comment

bnoordhuis Mar 13, 2020

Choose a reason for hiding this comment

gabrielschulhof Mar 13, 2020

Choose a reason for hiding this comment

gabrielschulhof Mar 13, 2020

Choose a reason for hiding this comment

gabrielschulhof commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

gabrielschulhof commented Mar 13, 2020

nodejs-github-bot commented Mar 13, 2020

nodejs-github-bot commented Mar 14, 2020

gabrielschulhof commented Mar 15, 2020

gabrielschulhof commented Mar 15, 2020

addaleax commented Mar 15, 2020

gengjiawen commented Mar 16, 2020

gabrielschulhof commented Mar 16, 2020

targos commented Apr 22, 2020