Uh oh!
There was an error while loading. Please reload this page.
src: find .text section using dl_iterate_phdr - #32244
Conversation
nodejs-github-bot
commented
Mar 13, 2020
efd3932 to
b53d3cdComparenodejs-github-bot
commented
Mar 13, 2020
bnoordhuis
left a comment
There was a problem hiding this comment.
LGTM % some minor (mostly style) issues.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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) {
// ...
}There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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)) {
// ...
}Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
@bnoordhuis I added some more sanity checks to ensure that we create a region that has proper offsets and > 0 page count.
There was a problem hiding this comment.
... or rather, that we consider the region to have been found only if it has proper offsets and >0 page count.
gabrielschulhof
commented
Mar 13, 2020
@bnoordhuis I addressed your review comments and added some sanity checks wrt. your comment regarding inclusive/exclusive comparison. |
nodejs-github-bot
commented
Mar 13, 2020
nodejs-github-bot
commented
Mar 13, 2020
nodejs-github-bot
commented
Mar 13, 2020
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>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
af31134 to
2d6be5eComparegabrielschulhof
commented
Mar 13, 2020
Rebased in the hope that if fixes the "node ASAN" test. |
nodejs-github-bot
commented
Mar 13, 2020
nodejs-github-bot
commented
Mar 14, 2020
gabrielschulhof
commented
Mar 15, 2020
@nodejs/collaborators sorry about the spam! Can this land without the "node ASAN" test? AFAICT it's failing for a lot of PRs. |
gabrielschulhof
commented
Mar 15, 2020
addaleax
commented
Mar 15, 2020
Yes. 👍 |
gengjiawen
commented
Mar 16, 2020
We dealt with Also |
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
commented
Mar 16, 2020
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>
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
commented
Apr 22, 2020
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>
Use
dl_iterate_phdr(3)to find the mapping containing__node_text_startinstead of parsing/proc/self/maps.Signed-off-by: @gabrielschulhof
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes