Skip to content

deps: backport c0f1ff2 from upstream V8#13517

Closed
targos wants to merge 1 commit intonodejs:masterfrom
targos:fix-gcc-7-v8-59
Closed

deps: backport c0f1ff2 from upstream V8#13517
targos wants to merge 1 commit intonodejs:masterfrom
targos:fix-gcc-7-v8-59

Conversation

@targos
Copy link
Member

@targos targos commented Jun 7, 2017

Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388

/cc @nodejs/v8 @bnoordhuis @octoploid

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

V8

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jun 7, 2017
@targos targos mentioned this pull request Jun 7, 2017
Closed
@bnoordhuis
Copy link
Member

bnoordhuis commented Jun 7, 2017

If it's still an issue, it should be fixed upstream first.

@octoploid
Copy link

octoploid commented Jun 7, 2017

Well, the standard upstream response is: »Do we care about gcc?«

@gibfahn
Copy link
Member

gibfahn commented Jun 7, 2017 •
edited
Loading

Well, the standard upstream response is: »Do we care about gcc?«

Isn't V8 built with gcc by default on some platforms?

cc/ @nodejs/v8

@bnoordhuis
Copy link
Member

bnoordhuis commented Jun 7, 2017

Yes, there is at least one gcc buildbot, although it probably uses gcc 6, not 7.

@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Jun 7, 2017
@gibfahn gibfahn mentioned this pull request Jun 9, 2017
Closed
@octoploid
Copy link

octoploid commented Jun 10, 2017

Upstream bug: https://bugs.chromium.org/p/chromium/issues/detail?id=691681

@targos
Copy link
Member Author

targos commented Jun 12, 2017

Trying to upstream the change: https://chromium-review.googlesource.com/c/530227/

/cc @fhinkel

jasnell
jasnell previously approved these changes Jun 13, 2017
View reviewed changes
@targos
Copy link
Member Author

targos commented Jun 19, 2017

I'm still waiting for a review upstream.

@bnoordhuis
Copy link
Member

bnoordhuis commented Jun 19, 2017

Maybe pick another reviewer?

@targos
Copy link
Member Author

targos commented Jun 20, 2017

@octoploid @bnoordhuis Could you please take a look at my last patch: https://chromium-review.googlesource.com/c/530227/3
I'd like to make sure I got this hash-table-inl.h thing right and that I did not reintroduce a cycle.

@octoploid
Copy link

octoploid commented Jun 20, 2017 •
edited
Loading

Please post an updated patch for Node.js.

@bnoordhuis
Copy link
Member

bnoordhuis commented Jun 20, 2017

@octoploid It's going upstream first before it goes into node.

@targos Left a comment on the CL.

@octoploid
Copy link

octoploid commented Jun 20, 2017

@bnoordhuis: Sure, it is just that I don't have v8 (or chromium) installed on my test machines.
So it would be much easier for me to test the patch for node.js.

@bnoordhuis
Copy link
Member

bnoordhuis commented Jun 20, 2017

@octoploid Just git apply --directory=deps/v8 it.

@targos
Copy link
Member Author

targos commented Jun 20, 2017

The change landed on V8 master: v8/v8@c0f1ff2

I will update this PR with a backport

@targos targos force-pushed the fix-gcc-7-v8-59 branch from d780f80 to ee63c30 Compare June 20, 2017 15:09
@targos targos changed the title v8: fix gcc 7 build errors deps: backport c0f1ff2 from upstream V8 Jun 20, 2017
@targos
deps: backport c0f1ff2 from upstream V8
522c29e 
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}

Fixes: nodejs#10388
@targos targos force-pushed the fix-gcc-7-v8-59 branch from ee63c30 to 522c29e Compare June 20, 2017 15:16
@targos
Copy link
Member Author

targos commented Jun 20, 2017

Updated to a backport. PTAL.

@octoploid
Copy link

octoploid commented Jun 20, 2017

Works fine. Thanks.

@targos
Copy link
Member Author

targos commented Jun 22, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/8795/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/753/

targos added a commit to targos/node that referenced this pull request Jun 23, 2017
@targos
deps: backport c0f1ff2 from upstream V8
0ba74db 
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>
@targos
Copy link
Member Author

targos commented Jun 23, 2017

Landed in 0ba74db

@targos targos closed this Jun 23, 2017
@targos targos deleted the fix-gcc-7-v8-59 branch June 23, 2017 08:15
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Aug 1, 2017
@targos @MylesBorins
deps: backport c0f1ff2 from upstream V8
e649b00 
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
@targos @MylesBorins
deps: backport c0f1ff2 from upstream V8
5c285c6 
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
@targos @MylesBorins
deps: backport c0f1ff2 from upstream V8
5651a6f 
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
@targos @addaleax
deps: backport c0f1ff2 from upstream V8
2dbf95d 
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>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell left review comments

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@targos @bnoordhuis @octoploid @gibfahn @jasnell @mscdex @nodejs-github-bot