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

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

Merged
merged 1 commit into from
Apr 19, 2017

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Apr 13, 2017

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
Copy link
Member

benjamingr commented Apr 13, 2017

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
Copy link
Member Author

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.

@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Apr 13, 2017
@danbev
Copy link
Contributor

danbev commented Apr 14, 2017

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
Copy link
Member Author

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
Copy link
Contributor

danbev commented Apr 14, 2017

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>
@bnoordhuis bnoordhuis merged commit 2bbee49 into nodejs:v7.x-staging Apr 19, 2017
@bnoordhuis bnoordhuis deleted the fix-v8-gcc-7 branch April 19, 2017 09:03
@kasicka
Copy link

kasicka commented Apr 26, 2017

Can you merge this into v8 branch too?

@bnoordhuis
Copy link
Member Author

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
Copy link

kasicka commented Apr 26, 2017

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

@kasicka kasicka mentioned this pull request Apr 26, 2017
4 tasks
addaleax pushed a commit that referenced this pull request Apr 29, 2017
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 evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
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>
evanlucas added a commit that referenced this pull request May 2, 2017
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
addaleax pushed a commit that referenced this pull request Jun 24, 2017
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
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 pushed a commit that referenced this pull request Jul 11, 2017
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>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
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
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 MylesBorins mentioned this pull request Jul 18, 2017
targos added a commit to targos/node that referenced this pull request Jul 21, 2017
addaleax pushed a commit that referenced this pull request Jul 24, 2017
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 pushed a commit to MylesBorins/node that referenced this pull request Aug 1, 2017
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>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Aug 1, 2017
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>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
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>
addaleax pushed a commit that referenced this pull request Aug 2, 2017
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>
@Qantas94Heavy Qantas94Heavy mentioned this pull request Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants