build: fix building with ninja by richardlau · Pull Request #32071 · nodejs/node · GitHub

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

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

CI with --ninja: https://ci.nodejs.org/job/node-test-commit-linux/33212/nodes=centos7-64-gcc6/

This breaks on Ubuntu 18.04.

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

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

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.

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

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', 
        
           }],

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

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

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

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

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

@richardlau

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

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

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

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

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

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

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

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

LGTM

should this be fast-tracked?

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

Landed in de6cbd0.

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>

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>

Depends on #31981 to land on v12.x

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> Backport-PR-URL: nodejs#32092 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>

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> Backport-PR-URL: #32092 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>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 3, 2020

richardlau mentioned this pull request Mar 3, 2020

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

Closed

2 tasks

mmarchini approved these changes Mar 3, 2020

View reviewed changes

richardlau added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 3, 2020

gabrielschulhof approved these changes Mar 3, 2020

View reviewed changes

sam-github approved these changes Mar 3, 2020

View reviewed changes

codebytere approved these changes Mar 3, 2020

View reviewed changes

gabrielschulhof requested changes Mar 3, 2020

View reviewed changes

richardlau removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 3, 2020

richardlau force-pushed the largepages branch from 4a65610 to 42f1d92 Compare March 4, 2020 00:17

gabrielschulhof approved these changes Mar 4, 2020

View reviewed changes

richardlau added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 4, 2020

MylesBorins approved these changes Mar 4, 2020

View reviewed changes

MylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 4, 2020

gengjiawen approved these changes Mar 4, 2020

View reviewed changes

richardlau self-assigned this Mar 4, 2020

richardlau force-pushed the largepages branch from 3d59b9c to de6cbd0 Compare March 4, 2020 04:51

richardlau merged commit de6cbd0 into nodejs:master Mar 4, 2020

richardlau deleted the largepages branch March 4, 2020 05:00

gabrielschulhof mentioned this pull request Mar 4, 2020

[v10.x] Backport large pages to v10.x #31105

Closed

4 tasks

MylesBorins mentioned this pull request Mar 4, 2020

v13.10.1 proposal #32099

Merged

targos added the backport-blocked-v12.x label Apr 20, 2020

targos added backported-to-v12.x and removed author ready PRs that have at least one approval, no outstanding review comments, 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

build: fix building with ninja by richardlau · Pull Request #32071 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: fix building with ninja - #32071

build: fix building with ninja#32071richardlau merged 1 commit intonodejs:masternodejs/node:masterfrom richardlau:largepagesrichardlau/node-1:largepagesCopy head branch name to clipboard

Conversation

richardlau commented Mar 3, 2020

Checklist

Uh oh!

nodejs-github-bot commented Mar 3, 2020

Uh oh!

mmarchini commented Mar 3, 2020

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Mar 3, 2020

Uh oh!

gabrielschulhof commented Mar 3, 2020

Uh oh!

gabrielschulhof commented Mar 3, 2020

Uh oh!

gabrielschulhof commented Mar 3, 2020

Uh oh!

richardlau commented Mar 3, 2020

Uh oh!

gabrielschulhof commented Mar 3, 2020

Uh oh!

gabrielschulhof commented Mar 4, 2020

Uh oh!

richardlau commented Mar 4, 2020

Uh oh!

gabrielschulhof commented Mar 4, 2020

Uh oh!

gabrielschulhof commented Mar 4, 2020

Uh oh!

richardlau commented Mar 4, 2020

Uh oh!

gabrielschulhof commented Mar 4, 2020

Uh oh!

richardlau commented Mar 4, 2020

Uh oh!

gabrielschulhof commented Mar 4, 2020

Uh oh!

gabrielschulhof commented Mar 4, 2020

Uh oh!

gabrielschulhof commented Mar 4, 2020

Uh oh!

richardlau commented Mar 4, 2020

Uh oh!

gabrielschulhof commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielschulhof commented Mar 4, 2020

Uh oh!

nodejs-github-bot commented Mar 4, 2020

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Mar 4, 2020

Uh oh!

gabrielschulhof commented Mar 4, 2020

Uh oh!

richardlau commented Mar 4, 2020

Uh oh!

targos commented Apr 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

build: fix building with ninja#32071
richardlau merged 1 commit into
nodejs:masterfrom
richardlau:largepages

gabrielschulhof commented Mar 4, 2020 •
edited
Loading