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.

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

src: start the .text section with an asm symbol #31981

Closed

Conversation

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Feb 27, 2020

We create an object file in assembly which introduces the symbol
__node_text_start into the .text section and place the resulting
object file as the first file the linker encounters. We do this to
ensure that we can recognize the boundaries of the .text section when
attempting to establish the address range to map to large pages.

Additionally, we rename the section containing the remapping code from
.lpstub to lpstub so as to take advantage of the linker's feature
whereby it inserts the symbol __start_lpstub when the section's name
can be rendered as a valid C variable. We need this symbol in order to
avoid self-mapping the remapping code to large pages, because doing so
would cause the process to crash.

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

This is an alternative to #31947.

We create an object file in assembly which introduces the symbol
`__node_text_start` into the .text section and place the resulting
object file as the first file the linker encounters. We do this to
ensure that we can recognize the boundaries of the .text section when
attempting to establish the address range to map to large pages.

Additionally, we rename the section containing the remapping code from
`.lpstub` to `lpstub` so as to take advantage of the linker's feature
whereby it inserts the symbol `__start_lpstub` when the section's name
can be rendered as a valid C variable. We need this symbol in order to
avoid self-mapping the remapping code to large pages, because doing so
would cause the process to crash.
@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:text-start-via-asm branch to 00787b5 Feb 27, 2020
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Feb 27, 2020

Removed now-unused extern char __executable_start;.

@nodejs-github-bot

This comment has been minimized.

@suresh-srinivas

This comment has been minimized.

Copy link
Contributor

suresh-srinivas commented Feb 27, 2020

This looks good to me. I think __executable_start will also work but the problem was in the surrounding code that had been rewritten from before and not tested, it looks like you have fixed it with this check so we are looking at the right line in the maps file.
if (permission != "r-xp")

@suresh-srinivas

This comment has been minimized.

Copy link
Contributor

suresh-srinivas commented Feb 27, 2020

In the original code that I wrote I also had a check
if (pathname == exename)
to make sure that even if the line was "r-xp", it was pointing to the nodejs executable. This check was an additional precaution to make sure that we were looking at the right line in the maps file.

I havent been following all the code modifications to this file but it has been broken at least twice due to improper restructuring. I dont know what Node.js policies are, but I think it would be good to have a list of owners for files or subsystems.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Feb 27, 2020

@suresh-srinivas IINM the fact that a known Node.js symbol (like __executable_start or __node_text_start with this modification) resides within the mapping under scrutiny establishes that the mapping in fact contains executable code from the Node.js binary rather than a shared library.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Feb 27, 2020

@suresh-srinivas it's a good sanity check though ...

Copy link
Member

bnoordhuis left a comment

LGTM in general but I suspect it's likely that:

  1. ./configure --enable-lto breaks it
  2. as would turning on -Wl,--gc-sections (which we currently don't but could)
src/large_pages/node_large_page.cc Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
.text
.align 0x2000

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Feb 28, 2020

Member

Why 8k?

This comment has been minimized.

Copy link
@gabrielschulhof

gabrielschulhof Feb 28, 2020

Author Contributor

Good question, actually. I'm not sure we need it, because we end up snipping the front and the back off the .text if it doesn't align at 2MiB anyway, leaving the snipped-off bits on 4KiB pages.

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

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Feb 28, 2020

@bnoordhuis AFAICT this works with --enable-lto. I used the flag and the resulting binary was able to find 9 large pages worth of text. I tested this with

  • GNU ld version 2.31.1-37.fc30 on Fedora 30
  • GNU ld (GNU Binutils for Ubuntu) 2.30 on Ubuntu 18.04

I'm hoping the linker will not drop the symbol because there is a reference to it, even if it's a weak reference.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Mar 3, 2020

gabrielschulhof added a commit that referenced this pull request Mar 3, 2020
We create an object file in assembly which introduces the symbol
`__node_text_start` into the .text section and place the resulting
object file as the first file the linker encounters. We do this to
ensure that we can recognize the boundaries of the .text section when
attempting to establish the address range to map to large pages.

Additionally, we rename the section containing the remapping code from
`.lpstub` to `lpstub` so as to take advantage of the linker's feature
whereby it inserts the symbol `__start_lpstub` when the section's name
can be rendered as a valid C variable. We need this symbol in order to
avoid self-mapping the remapping code to large pages, because doing so
would cause the process to crash.

PR-URL: #31981
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Mar 3, 2020

Landed in 987a673.

@gabrielschulhof gabrielschulhof deleted the gabrielschulhof:text-start-via-asm branch Mar 3, 2020
@gabrielschulhof gabrielschulhof mentioned this pull request Mar 3, 2020
4 of 4 tasks complete
@codebytere codebytere mentioned this pull request Mar 3, 2020
codebytere added a commit that referenced this pull request Mar 3, 2020
We create an object file in assembly which introduces the symbol
`__node_text_start` into the .text section and place the resulting
object file as the first file the linker encounters. We do this to
ensure that we can recognize the boundaries of the .text section when
attempting to establish the address range to map to large pages.

Additionally, we rename the section containing the remapping code from
`.lpstub` to `lpstub` so as to take advantage of the linker's feature
whereby it inserts the symbol `__start_lpstub` when the section's name
can be rendered as a valid C variable. We need this symbol in order to
avoid self-mapping the remapping code to large pages, because doing so
would cause the process to crash.

PR-URL: #31981
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@lundibundi

This comment has been minimized.

Copy link
Member

lundibundi commented Mar 3, 2020

Looks like this broke build with configure --ninja. It fails for me locally every time (It cannot find the node_text_start file during linking).

Also, there is recent CI failure:
#32046
https://github.com/nodejs/node/pull/32046/checks?check_run_id=482240372

c++ -fsanitize=address -pthread -rdynamic -m64 obj.target/node_text_start/src/large_pages/node_text_start.o -Wl,--whole-archive obj/libnode.a obj/tools/v8_gypfiles/libv8_base_without_compiler.a -Wl,--no-whole-archive -Wl,--whole-archive obj/deps/zlib/libzlib.a -Wl,--no-whole-archive -Wl,--whole-archive obj/deps/uv/libuv.a -Wl,--no-whole-archive -Wl,-z,noexecstack -Wl,--whole-archive obj/tools/v8_gypfiles/libv8_snapshot.a -Wl,--no-whole-archive -Wl,-z,relro -Wl,-z,now -Wl,--whole-archive,obj/deps/openssl/libopenssl.a -Wl,--no-whole-archive -pthread -o node -Wl,--start-group obj/gen/node.node_code_cache.o obj/gen/node.node_snapshot.o obj/src/node.node_main.o obj/deps/histogram/libhistogram.a obj/deps/uvwasi/libuvwasi.a obj/libnode.a obj/libnode_text_start.a obj/tools/v8_gypfiles/libv8_libplatform.a obj/tools/icu/libicui18n.a obj/deps/zlib/libzlib.a obj/deps/llhttp/libllhttp.a obj/deps/cares/libcares.a obj/deps/uv/libuv.a obj/deps/nghttp2/libnghttp2.a obj/deps/brotli/libbrotli.a obj/deps/openssl/libopenssl.a obj/tools/v8_gypfiles/libv8_base_without_compiler.a obj/tools/icu/libicuucx.a obj/tools/icu/libicudata.a obj/tools/v8_gypfiles/libv8_libbase.a obj/tools/v8_gypfiles/libv8_libsampler.a obj/tools/v8_gypfiles/libv8_compiler.a obj/tools/v8_gypfiles/libv8_snapshot.a obj/tools/v8_gypfiles/libv8_initializers.a  -lm -ldl -Wl,--end-group
c++: error: obj.target/node_text_start/src/large_pages/node_text_start.o: No such file or directory
@richardlau richardlau mentioned this pull request Mar 3, 2020
2 of 2 tasks complete
@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Mar 3, 2020

Looks like this broke build with configure --ninja. It fails for me locally every time (It cannot find the node_text_start file during linking).

Also, there is recent CI failure:
#32046
https://github.com/nodejs/node/pull/32046/checks?check_run_id=482240372

c++ -fsanitize=address -pthread -rdynamic -m64 obj.target/node_text_start/src/large_pages/node_text_start.o -Wl,--whole-archive obj/libnode.a obj/tools/v8_gypfiles/libv8_base_without_compiler.a -Wl,--no-whole-archive -Wl,--whole-archive obj/deps/zlib/libzlib.a -Wl,--no-whole-archive -Wl,--whole-archive obj/deps/uv/libuv.a -Wl,--no-whole-archive -Wl,-z,noexecstack -Wl,--whole-archive obj/tools/v8_gypfiles/libv8_snapshot.a -Wl,--no-whole-archive -Wl,-z,relro -Wl,-z,now -Wl,--whole-archive,obj/deps/openssl/libopenssl.a -Wl,--no-whole-archive -pthread -o node -Wl,--start-group obj/gen/node.node_code_cache.o obj/gen/node.node_snapshot.o obj/src/node.node_main.o obj/deps/histogram/libhistogram.a obj/deps/uvwasi/libuvwasi.a obj/libnode.a obj/libnode_text_start.a obj/tools/v8_gypfiles/libv8_libplatform.a obj/tools/icu/libicui18n.a obj/deps/zlib/libzlib.a obj/deps/llhttp/libllhttp.a obj/deps/cares/libcares.a obj/deps/uv/libuv.a obj/deps/nghttp2/libnghttp2.a obj/deps/brotli/libbrotli.a obj/deps/openssl/libopenssl.a obj/tools/v8_gypfiles/libv8_base_without_compiler.a obj/tools/icu/libicuucx.a obj/tools/icu/libicudata.a obj/tools/v8_gypfiles/libv8_libbase.a obj/tools/v8_gypfiles/libv8_libsampler.a obj/tools/v8_gypfiles/libv8_compiler.a obj/tools/v8_gypfiles/libv8_snapshot.a obj/tools/v8_gypfiles/libv8_initializers.a  -lm -ldl -Wl,--end-group
c++: error: obj.target/node_text_start/src/large_pages/node_text_start.o: No such file or directory

I think #32071 should fix it.

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 4, 2020
We create an object file in assembly which introduces the symbol
`__node_text_start` into the .text section and place the resulting
object file as the first file the linker encounters. We do this to
ensure that we can recognize the boundaries of the .text section when
attempting to establish the address range to map to large pages.

Additionally, we rename the section containing the remapping code from
`.lpstub` to `lpstub` so as to take advantage of the linker's feature
whereby it inserts the symbol `__start_lpstub` when the section's name
can be rendered as a valid C variable. We need this symbol in order to
avoid self-mapping the remapping code to large pages, because doing so
would cause the process to crash.

PR-URL: nodejs#31981
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
MylesBorins added a commit that referenced this pull request Mar 4, 2020
We create an object file in assembly which introduces the symbol
`__node_text_start` into the .text section and place the resulting
object file as the first file the linker encounters. We do this to
ensure that we can recognize the boundaries of the .text section when
attempting to establish the address range to map to large pages.

Additionally, we rename the section containing the remapping code from
`.lpstub` to `lpstub` so as to take advantage of the linker's feature
whereby it inserts the symbol `__start_lpstub` when the section's name
can be rendered as a valid C variable. We need this symbol in order to
avoid self-mapping the remapping code to large pages, because doing so
would cause the process to crash.

PR-URL: #31981
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@MylesBorins MylesBorins mentioned this pull request Mar 4, 2020
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 6, 2020
We create an object file in assembly which introduces the symbol
`__node_text_start` into the .text section and place the resulting
object file as the first file the linker encounters. We do this to
ensure that we can recognize the boundaries of the .text section when
attempting to establish the address range to map to large pages.

Additionally, we rename the section containing the remapping code from
`.lpstub` to `lpstub` so as to take advantage of the linker's feature
whereby it inserts the symbol `__start_lpstub` when the section's name
can be rendered as a valid C variable. We need this symbol in order to
avoid self-mapping the remapping code to large pages, because doing so
would cause the process to crash.

PR-URL: nodejs#31981
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.