v8: fix build errors with g++ 7 by bnoordhuis · Pull Request #12392 · nodejs/node

bnoordhuis · 2017-04-13T11:56:39Z

This is a local patch because upstream fixed it differently by moving
large chunks of code out of objects.h. We cannot easily back-port
those changes due to their size and invasiveness.

Node CI: https://ci.nodejs.org/job/node-test-pull-request/7376/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/645/

benjamingr · 2017-04-13T14:34:59Z

test failure is weird and does not happen on my box with this commit (although, with newer clang).

Found this catchorg/Catch2#334

Related issue #12388 , I'm fine with landing as is.

bnoordhuis · 2017-04-13T16:14:55Z

The test failure:

In file included from ../deps/v8/src/base/functional.cc:11:
In file included from ../deps/v8/src/base/functional.h:11:
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.9/../../../../include/c++/4.9/cstddef:51:11: error: no member named 'max_align_t' in the global namespace
  using ::max_align_t;
        ~~^
1 error generated.

It's not related to this pull request, it happens intermittently on the clang 3.4.1 buildbot for reasons beyond my ken.

danbev · 2017-04-14T08:46:27Z

Sorry if this is the wrong place and might be a stupid question...
When reviewing this I wanted to run it locally with g++7, so I installed it (on Mac):

$ g++ -v
gcc version 7.0.1 20170409 (experimental) (GCC)
$ gcc -v
gcc version 7.0.1 20170409 (experimental) (GCC)

Running make I get the following error:

make -C out BUILDTYPE=Release V=1
  c++ '-D_DARWIN_USE_64_BIT_INODE=1' -I../deps/gtest -I../deps/gtest/include  -Os -gdwarf-2 -mmacosx-version-min=10.7 -arch x86_64 -Wall -Wendif-labels -W -Wno-unused-parameter -std=gnu++0x -stdlib=libc++ -fno-rtti -fno-exceptions -fno-threadsafe-statics -fno-strict-aliasing -MMD -MF /Users/danielbevenius/work/nodejs/node/out/Release/.deps//Users/danielbevenius/work/nodejs/node/out/Release/obj.target/gtest/deps/gtest/src/gtest-death-test.o.d.raw   -c -o /Users/danielbevenius/work/nodejs/node/out/Release/obj.target/gtest/deps/gtest/src/gtest-death-test.o ../deps/gtest/src/gtest-death-test.cc
c++: error: unrecognized command line option ‘-stdlib=libc++’
make[1]: *** [/Users/danielbevenius/work/nodejs/node/out/Release/obj.target/gtest/deps/gtest/src/gtest-death-test.o] Error 1
make: *** [node] Error 2

I'm able to force this compile and run the test by setting clang to 0 in common.gypi:

      ['OS=="mac"', {
        'clang%': 0,
      }, {
        'clang%': 0,
      }],

I get the feeling that I'm missing something. If you have any advice I'd appreciated it. I'd like to be able to switch between gcc/clang for future reviews/issues/troubleshooting. Thanks

bnoordhuis · 2017-04-14T09:08:28Z

I believe -stdlib=... is an apple-clang extension. We assume clang on OS X and set CLANG_CXX_LIBRARY accordingly. You should be able to override it with export CXX=g++ CXX.host=g++ && ./configure -- -Dclang=0.

danbev · 2017-04-14T11:39:30Z

You should be able to override it with export CXX=g++ CXX.host=g++ && ./configure -- -Dclang=0.

Worked like a charm, thanks!

This is a local patch because upstream fixed it differently by moving large chunks of code out of objects.h. We cannot easily back-port those changes due to their size and invasiveness. Fixes: nodejs#10388 PR-URL: nodejs#12392 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

kasicka · 2017-04-26T11:09:39Z

Can you merge this into v8 branch too?

bnoordhuis · 2017-04-26T11:47:07Z

Ambiguity alert: do you mean the node.js v8.0 branch or the upstream V8 project?

If it's the former, I don't remember build issues with g++ 7 but if you have them, file a pull request against the master branch.

If it's V8, you'll have to send the changes to them - although here too I don't remember issues last time I tried.

kasicka · 2017-04-26T12:50:16Z

I meant node.js v8 branch, latest nightly fails to build on rawhide.

Porting #12392 to master Ref: #12392 Fixes: #10388 PR-URL: #12676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This is a local patch because upstream fixed it differently by moving large chunks of code out of objects.h. We cannot easily back-port those changes due to their size and invasiveness. Fixes: #10388 PR-URL: #12392 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * **crypto**: - add randomFill and randomFillSync (Evan Lucas) #10209 * **meta**: Added new collaborators - add lucamaraschi to collaborators (Luca Maraschi) #12538 - add DavidCai1993 to collaborators (David Cai) #12435 - add jkrems to collaborators (Jan Krems) #12427 - add AnnaMag to collaborators (AnnaMag) #12414 * **process**: - fix crash when Promise rejection is a Symbol (Cameron Little) #11640 * **url**: - make WHATWG URL more spec compliant (Timothy Gu) #12507 * **v8**: - fix stack overflow in recursive method (Ben Noordhuis) #12460 - fix build errors with g++ 7 (Ben Noordhuis) #12392 PR-URL: #12775

Porting #12392 to V8 5.9 Ref: #12392 Fixes: #10388 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This is a local patch because upstream fixed it differently by moving large chunks of code out of objects.h. We cannot easily back-port those changes due to their size and invasiveness. Fixes: #10388 PR-URL: #12392 Backport-PR-URL: #13574 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Porting #12392 to V8 5.9 Ref: #12392 Fixes: #10388 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Porting nodejs#12392 to V8 5.9 Ref: nodejs#12392 Fixes: nodejs#10388

Porting #12392 to V8 5.9 Ref: #12392 Fixes: #10388 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Original commit message: Fix GCC 7 build errors BUG=chromium:691681 R=franzih@chromium.org Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9 Reviewed-on: https://chromium-review.googlesource.com/530227 Commit-Queue: Toon Verwaest <verwaest@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#46045} PR-URL: nodejs#13517 Fixes: nodejs#10388 Refs: nodejs#12392 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Original commit message: Fix GCC 7 build errors BUG=chromium:691681 R=franzih@chromium.org Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9 Reviewed-on: https://chromium-review.googlesource.com/530227 Commit-Queue: Toon Verwaest <verwaest@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#46045} Refs: nodejs#13517 Fixes: nodejs#10388 Refs: nodejs#12392 PR-URL: nodejs#14004 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Original commit message: Fix GCC 7 build errors BUG=chromium:691681 R=franzih@chromium.org Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9 Reviewed-on: https://chromium-review.googlesource.com/530227 Commit-Queue: Toon Verwaest <verwaest@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#46045} Refs: #13517 Fixes: #10388 Refs: #12392 PR-URL: #14004 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Original commit message: Fix GCC 7 build errors BUG=chromium:691681 R=franzih@chromium.org Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9 Reviewed-on: https://chromium-review.googlesource.com/530227 Commit-Queue: Toon Verwaest <verwaest@chromium.org> Reviewed-by: Toon Verwaest <verwaest@chromium.org> Cr-Commit-Position: refs/heads/master@{#46045} Refs: #13517 Fixes: #10388 Refs: #12392 Backport-PR-URL: #14574 Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net> Backport-Reviewed-By: Refael Ackermann <refack@gmail.com> PR-URL: #14004 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added v7.x v8 engine Issues and PRs related to the V8 dependency. labels Apr 13, 2017

bnoordhuis mentioned this pull request Apr 13, 2017

deps/v8/src/objects.h:3263:46: error: invalid use of incomplete type ‘class v8::internal::Heap #10388

Closed

benjamingr approved these changes Apr 13, 2017

View reviewed changes

addaleax approved these changes Apr 13, 2017

View reviewed changes

mscdex added the build Issues and PRs related to build files or the CI. label Apr 13, 2017

jasnell approved these changes Apr 13, 2017

View reviewed changes

danbev approved these changes Apr 14, 2017

View reviewed changes

thefourtheye mentioned this pull request Apr 18, 2017

deps: cherry-pick 79aee39 from upstream v8 #12412

Merged

bnoordhuis force-pushed the fix-v8-gcc-7 branch from 26dfbd6 to 2bbee49 Compare April 19, 2017 09:03

bnoordhuis merged commit 2bbee49 into nodejs:v7.x-staging Apr 19, 2017

bnoordhuis deleted the fix-v8-gcc-7 branch April 19, 2017 09:03

gibfahn mentioned this pull request Apr 24, 2017

ubuntu1204-clang341 has been disabled in CI nodejs/build#685

Closed

kasicka mentioned this pull request Apr 26, 2017

v8: fix gcc 7 build errors #12676

Closed

4 tasks

addaleax pushed a commit that referenced this pull request Apr 29, 2017

v8: fix gcc 7 build errors

b07e1a8

Porting #12392 to master Ref: #12392 Fixes: #10388 PR-URL: #12676 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

addaleax pushed a commit that referenced this pull request Jun 24, 2017

v8: fix gcc 7 build errors

547ca9a

Porting #12392 to V8 5.9 Ref: #12392 Fixes: #10388 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

rvagg pushed a commit that referenced this pull request Jun 29, 2017

v8: fix gcc 7 build errors

950b97c

Porting #12392 to V8 5.9 Ref: #12392 Fixes: #10388 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax pushed a commit that referenced this pull request Jul 11, 2017

v8: fix gcc 7 build errors

dd48f5d

Porting #12392 to V8 5.9 Ref: #12392 Fixes: #10388 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax pushed a commit that referenced this pull request Jul 18, 2017

v8: fix gcc 7 build errors

9235125

Porting #12392 to V8 5.9 Ref: #12392 Fixes: #10388 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

targos added a commit to targos/node that referenced this pull request Jul 21, 2017

v8: fix gcc 7 build errors

b47e758

Porting nodejs#12392 to V8 5.9 Ref: nodejs#12392 Fixes: nodejs#10388

addaleax pushed a commit that referenced this pull request Jul 24, 2017

v8: fix gcc 7 build errors

0be4d17

Porting #12392 to V8 5.9 Ref: #12392 Fixes: #10388 PR-URL: #13515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Qantas94Heavy mentioned this pull request Oct 11, 2017

v4.8.5 proposal #15507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

v8: fix build errors with g++ 7#12392

v8: fix build errors with g++ 7#12392
bnoordhuis merged 1 commit intonodejs:v7.x-stagingfrom
bnoordhuis:fix-v8-gcc-7

bnoordhuis commented Apr 13, 2017 •

edited

Loading

Uh oh!

benjamingr commented Apr 13, 2017 •

edited

Loading

Uh oh!

bnoordhuis commented Apr 13, 2017

Uh oh!

danbev commented Apr 14, 2017

Uh oh!

bnoordhuis commented Apr 14, 2017

Uh oh!

danbev commented Apr 14, 2017

Uh oh!

kasicka commented Apr 26, 2017

Uh oh!

bnoordhuis commented Apr 26, 2017

Uh oh!

kasicka commented Apr 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Uh oh!

Conversation

bnoordhuis commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamingr commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Apr 13, 2017

Uh oh!

danbev commented Apr 14, 2017

Uh oh!

bnoordhuis commented Apr 14, 2017

Uh oh!

danbev commented Apr 14, 2017

Uh oh!

kasicka commented Apr 26, 2017

Uh oh!

bnoordhuis commented Apr 26, 2017

Uh oh!

kasicka commented Apr 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

bnoordhuis commented Apr 13, 2017 •

edited

Loading

benjamingr commented Apr 13, 2017 •

edited

Loading