Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: refine maps parsing for large pages #29973

Closed
wants to merge 1 commit into from
Closed

src: refine maps parsing for large pages #29973

wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
Contributor

Multiple sections may be marked as "r-xp" and with the executable's
path. We use the location of the __nodetext symbol added by the linker
script to ensure that the range we retrieve from the maps file does
indeed contain the Node.js text section.

Thanks to @suresh-srinivas!

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 14, 2019
@devnexen
Copy link
Contributor

nit: I would make clearer it is Linux even though map implies means /proc reading but no forcing here :-)

bnoordhuis
bnoordhuis reviewed Oct 14, 2019
View reviewed changes
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
src: refine maps parsing for large pages
ecea44c 
Multiple sections may be marked as "r-xp" and with the executable's
path. We use the location of the `__nodetext` symbol added by the linker
script to ensure that the range we retrieve from the maps file does
indeed contain the Node.js text section.

Thanks to Suresh Srinivas <[email protected]>!
@gabrielschulhof gabrielschulhof force-pushed the fix-largepages-section-misread branch from 4937566 to ecea44c Compare October 15, 2019 02:18
@gabrielschulhof
Copy link
Contributor Author

@bnoordhuis I fixed the comparison and moved the whole thing to uintptr_t.

@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 16, 2019
gabrielschulhof pushed a commit that referenced this pull request Oct 16, 2019
src: refine maps parsing for large pages
1599aeb 
Multiple sections may be marked as "r-xp" and with the executable's
path. We use the location of the `__nodetext` symbol added by the linker
script to ensure that the range we retrieve from the maps file does
indeed contain the Node.js text section.

Thanks to Suresh Srinivas <[email protected]>!

PR-URL: #29973
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@gabrielschulhof
Copy link
Contributor Author

Landed in 1599aeb.

@gabrielschulhof gabrielschulhof deleted the fix-largepages-section-misread branch October 16, 2019 19:37
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
@MylesBorins
src: refine maps parsing for large pages
7ed00fd 
Multiple sections may be marked as "r-xp" and with the executable's
path. We use the location of the `__nodetext` symbol added by the linker
script to ensure that the range we retrieve from the maps file does
indeed contain the Node.js text section.

Thanks to Suresh Srinivas <[email protected]>!

PR-URL: #29973
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 23, 2019
@MylesBorins
src: refine maps parsing for large pages
3eae670 
Multiple sections may be marked as "r-xp" and with the executable's
path. We use the location of the `__nodetext` symbol added by the linker
script to ensure that the range we retrieve from the maps file does
indeed contain the Node.js text section.

Thanks to Suresh Srinivas <[email protected]>!

PR-URL: #29973
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
Merged
@gabrielschulhof
Copy link
Contributor Author

@BethGriggs currently this patch applies cleanly to v12.x-staging after the previous patch (#29914). May it please be backported?

targos pushed a commit that referenced this pull request Nov 8, 2019
@targos
src: refine maps parsing for large pages
5096b7d 
Multiple sections may be marked as "r-xp" and with the executable's
path. We use the location of the `__nodetext` symbol added by the linker
script to ensure that the range we retrieve from the maps file does
indeed contain the Node.js text section.

Thanks to Suresh Srinivas <[email protected]>!

PR-URL: #29973
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
@targos
src: refine maps parsing for large pages
45c5ad7 
Multiple sections may be marked as "r-xp" and with the executable's
path. We use the location of the `__nodetext` symbol added by the linker
script to ensure that the range we retrieve from the maps file does
indeed contain the Node.js text section.

Thanks to Suresh Srinivas <[email protected]>!

PR-URL: #29973
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@targos targos mentioned this pull request Nov 10, 2019
Merged
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Mar 6, 2020
src: refine maps parsing for large pages
d5b5b22 
Multiple sections may be marked as "r-xp" and with the executable's
path. We use the location of the `__nodetext` symbol added by the linker
script to ensure that the range we retrieve from the maps file does
indeed contain the Node.js text section.

Thanks to Suresh Srinivas <[email protected]>!

PR-URL: nodejs#29973
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devnexen devnexen devnexen approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gabrielschulhof @devnexen @nodejs-github-bot @bnoordhuis @jasnell @Trott @targos