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

build: fix building with ninja #32071

Merged
merged 1 commit into from Mar 4, 2020
Merged

build: fix building with ninja #32071

merged 1 commit into from Mar 4, 2020

Conversation

@richardlau
Copy link
Member

richardlau commented Mar 3, 2020

The ninja build places objects in a different directory.

Refs: #31981 (comment)

cc @gabrielschulhof

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@richardlau richardlau mentioned this pull request Mar 3, 2020
2 of 2 tasks complete
@nodejs-github-bot

This comment has been minimized.

@mmarchini

This comment has been minimized.

Copy link
Contributor

gabrielschulhof left a comment

This breaks on Ubuntu 18.04.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 3, 2020

The file given needs to be the object file, not the static library.

@richardlau richardlau removed the author ready label Mar 3, 2020
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 3, 2020

... and it needs to be the first file given to the linker.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 3, 2020

I mean, if you build with this change, it fails to find the mapping for large pages – probably because the value of __node_text_start is 0x0000000000000000, because the symbol is not linked in. That's why the object file needs to be passed to the linker directly, not via the static library.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 3, 2020

Is there a gyp variable holding the directory where object files are placed?

@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Mar 3, 2020

Is there a gyp variable holding the directory where object files are placed?

<obj_dir>, but the location of the object files under that appear different when using ninja vs. make.

node/common.gypi

Lines 88 to 94 in 3d894d0

['GENERATOR == "ninja"', {
'obj_dir': '<(PRODUCT_DIR)/obj',
'v8_base': '<(PRODUCT_DIR)/obj/tools/v8_gypfiles/libv8_snapshot.a',
}, {
'obj_dir%': '<(PRODUCT_DIR)/obj.target',
'v8_base': '<(PRODUCT_DIR)/obj.target/tools/v8_gypfiles/libv8_snapshot.a',
}],

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 3, 2020

@richardlau oh, so ninja doesn't do src/large_pages/node_text_start.o, meaning that it's the same as the path to the source file, but with .o?

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

@richardlau we should then be able to use a condition like the one above to switch between the two paths, right?

@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Mar 4, 2020

@richardlau we should then be able to use a condition like the one above to switch between the two paths, right?

I'm testing that right now.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

@richardlau can you also manually run node --use-largepages=on? It should not output failed to find text region.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

The test that runs node --use-largepages=on cannot be made to expect quiet on stderr, because mapping is fairly platform-dependent, and failure to map must not be fatal to the execution. Thus, the test only tests that Node.js successfully runs with the flag, not that it successfully maps with the flag.

@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Mar 4, 2020

@richardlau can you also manually run node --use-largepages=on? It should not output failed to find text region.

I don't think my Linux environment has support for large pages (it prints Large pages are not enabled.).

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

@richardlau

echo madvise > /sys/kernel/mm/transparent_hugepage/enabled
@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Mar 4, 2020

@richardlau

echo madvise > /sys/kernel/mm/transparent_hugepage/enabled

@gabrielschulhof It's a shared development server so I almost certainly don't have permissions to do that.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

@richardlau OK, no problem. I can test on my machine when you push to this PR.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

@richardlau another good check is to examine the

g++ -o /home/nix/node/node/out/Release/node -pthread -rdynamic -m64 -Wl,--whole-archive...

line at the end of the build to make sure that node_text_start.o is the first file listed.

@richardlau richardlau force-pushed the richardlau:largepages branch Mar 4, 2020
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

@richardlau testing ...

@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Mar 4, 2020

FTR with Ninja it looks like the object file is written to <(obj_dir)/src/large_pages/node_text_start.node_text_start.o.

with make:

  g++ -o /home/users/riclau/sandbox/github/nodejs/out/Release/node -pthread -rdynamic -m64 -Wl,--whole-archive /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/libnode.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/v8_gypfiles/libv8_base_without_compiler.a -Wl,--no-whole-archive /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/node_text_start/src/large_pages/node_text_start.o -Wl,--whole-archive /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/zlib/libzlib.a -Wl,--no-whole-archive -Wl,--whole-archive /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/uv/libuv.a -Wl,--no-whole-archive -Wl,-z,noexecstack -Wl,--whole-archive /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/v8_gypfiles/libv8_snapshot.a -Wl,--no-whole-archive -Wl,-z,relro -Wl,-z,now -Wl,--whole-archive,/home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/openssl/libopenssl.a -Wl,--no-whole-archive -pthread  -Wl,--start-group /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/node/src/node_main.o /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/node/gen/node_code_cache.o /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/node/gen/node_snapshot.o /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/histogram/libhistogram.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/uvwasi/libuvwasi.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/libnode.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/libnode_text_start.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/v8_gypfiles/libv8_libplatform.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/icu/libicui18n.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/zlib/libzlib.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/llhttp/libllhttp.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/cares/libcares.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/uv/libuv.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/nghttp2/libnghttp2.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/brotli/libbrotli.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/deps/openssl/libopenssl.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/v8_gypfiles/libv8_base_without_compiler.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/icu/libicuucx.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/icu/libicudata.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/v8_gypfiles/libv8_libbase.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/v8_gypfiles/libv8_libsampler.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/v8_gypfiles/libv8_compiler.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/v8_gypfiles/libv8_snapshot.a /home/users/riclau/sandbox/github/nodejs/out/Release/obj.target/tools/v8_gypfiles/libv8_initializers.a -lm -ldl -Wl,--end-group

and with ninja:

[60/60] c++ -pthread -rdynamic -m64 -Wl,--whole-archive obj/libnode.a obj/tools/v8_gypfiles/libv8_base_without_compiler.a -Wl,--no-whole-archive obj/src/large_pages/node_text_start.node_text_start.o -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
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

@richardlau argh! This works, but the command line changes:

g++
-o
/home/gschulho/node/out/Release/node
-pthread
-rdynamic
-m64
-Wl,--whole-archive
/home/gschulho/node/out/Release/obj.target/libnode.a
/home/gschulho/node/out/Release/obj.target/tools/v8_gypfiles/libv8_base_without_compiler.a
-Wl,--no-whole-archive
/home/gschulho/node/out/Release/obj.target/node_text_start/src/large_pages/node_text_start.o

so the textobject file gets placed after libnode.a and libv8_base_without_compiler.a and so the usefulness of the symbol is greatly diminished because it comes a lot later in the game. I don't understand why the switch to a condition has such a terrible effect.

I would like to test putting the condition in the variables section so that we can add the ldflags+ unconditionally. Maybe that'll please The GYP King™ 🤷

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

Great success! – hopefully:

g++
-o
/home/gschulho/node/out/Release/node
-pthread
-rdynamic
-m64
/home/gschulho/node/out/Release/obj.target/node_text_start/src/large_pages/node_text_start.o
-Wl,--whole-archive
/home/gschulho/node/out/Release/obj.target/libnode.a
...
@gabrielschulhof gabrielschulhof force-pushed the richardlau:largepages branch Mar 4, 2020
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

@richardlau I pushed a commit to your PR branch. Can you please test it?

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Mar 4, 2020

@richardlau I pushed a commit to your PR branch. Can you please test it?

with ninja:

[60/60] c++ -pthread -rdynamic -m64 obj/src/large_pages/node_text_start.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

or (for ease of reading):

[60/60] c++ 
-pthread 
-rdynamic 
-m64 
obj/src/large_pages/node_text_start.node_text_start.o 
-Wl,--whole-archive 
obj/libnode.a 
obj/tools/v8_gypfiles/libv8_base_without_compiler.a 
-Wl,--no-whole-archive
...
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

@richardlau so far so good. Not sure if I submitted that CI job correctly because it's saying "ninja: command not found" on a bunch of the platforms.

@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Mar 4, 2020

@richardlau so far so good. Not sure if I submitted that CI job correctly because it's saying "ninja: command not found" on a bunch of the platforms.

We generally don't have ninja installed on our CI. I noticed this failing because the recently added ASAN CI using GitHub Actions (#31902) uses it (I myself haven't used ninja before today).

@gabrielschulhof gabrielschulhof force-pushed the richardlau:largepages branch Mar 4, 2020
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

Pushed one more minor change – to remove the trailing comma, thereby removing a superfluous diff.

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

MylesBorins left a comment

LGTM

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Mar 4, 2020

should this be fast-tracked?

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

gabrielschulhof commented Mar 4, 2020

@MylesBorins I guess it does break the GitHub action, so it's probably a good idea.

The ninja build places objects in a different directory.

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>

PR-URL: #32071
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@richardlau richardlau self-assigned this Mar 4, 2020
@richardlau richardlau force-pushed the richardlau:largepages branch to de6cbd0 Mar 4, 2020
@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Mar 4, 2020

Landed in de6cbd0.

@richardlau richardlau merged commit de6cbd0 into nodejs:master Mar 4, 2020
11 checks passed
11 checks passed
build-docs
Details
build (3.8)
Details
ubuntu-build ubuntu-build
Details
build-linux
Details
build-windows
Details
build-macOS
Details
lint-addon-docs
Details
lint-cpp
Details
lint-md
Details
lint-js
Details
Travis CI - Pull Request Build Passed
Details
@richardlau richardlau deleted the richardlau:largepages branch Mar 4, 2020
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 4, 2020
The ninja build places objects in a different directory.

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>

PR-URL: nodejs#32071
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@gabrielschulhof gabrielschulhof mentioned this pull request Mar 4, 2020
4 of 4 tasks complete
MylesBorins added a commit that referenced this pull request Mar 4, 2020
The ninja build places objects in a different directory.

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>

PR-URL: #32071
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.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
The ninja build places objects in a different directory.

Co-authored-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Signed-off-by: Richard Lau <riclau@uk.ibm.com>

PR-URL: nodejs#32071
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.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

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