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

deps: workaround clang-3.4 ICE #8343

Merged
merged 1 commit into from
Aug 31, 2016

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Aug 30, 2016

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

deps: v8

Description of change

Work around internal compiler error with clang-3.4.

R=@nodejs/v8
/cc @chrislea
Fixes: #8323

EDIT: CI: https://ci.nodejs.org/job/node-test-pull-request/3893/
EDIT: V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/293/

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Aug 30, 2016
@mscdex
Copy link
Contributor

mscdex commented Aug 30, 2016

Is there already an upstream bug fix or at least an upstream issue for this that might be worth linking to also?

@ofrobots
Copy link
Contributor Author

I don't have an upstream bug fix link, but the issue seems to be fixed in clang-3.4.2. As per #8323 (comment), it is non-trivial to upgrade to clang-3.4.2 or newer on the wheezy & precise configurations.

@rvagg
Copy link
Member

rvagg commented Aug 31, 2016

Aside from objecting to the ugliness of C++, this LGTM

Green CI run @ https://ci.nodejs.org/job/node-test-commit/4862/

Most notably, clang 3.4.1 run @ https://ci.nodejs.org/job/node-test-commit-linux/4917/nodes=ubuntu1204-clang341-64/

@ofrobots ofrobots changed the base branch from v6.x to master August 31, 2016 21:03
@ofrobots ofrobots force-pushed the workaround-for-clang-3.4 branch from e926bbd to dc33d15 Compare August 31, 2016 21:06
@ofrobots
Copy link
Contributor Author

I have retargeted this PR to master (from v6.x) now that we have a (broken) CI builder for clang-3.4.

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

@rvagg
Copy link
Member

rvagg commented Aug 31, 2016

CI is green (enough). Debian8 failure is Jenkins, I've updated and restarted that node. OSX failure is known flaky (#4629). clang 4.3.1 is 👍

lgtm still

PR-URL: nodejs#8343
Fixes: nodejs#8323
Reviewed-By: rvagg - Rod Vagg <[email protected]>
@ofrobots ofrobots force-pushed the workaround-for-clang-3.4 branch from dc33d15 to 870faee Compare August 31, 2016 22:17
@ofrobots ofrobots merged commit 870faee into nodejs:master Aug 31, 2016
@ofrobots
Copy link
Contributor Author

Thanks landed as 870faee.

@ofrobots ofrobots deleted the workaround-for-clang-3.4 branch August 31, 2016 22:19
@Fishrock123 Fishrock123 mentioned this pull request Sep 6, 2016
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
PR-URL: nodejs#8343
Fixes: nodejs#8323
Reviewed-By: rvagg - Rod Vagg <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
PR-URL: #8343
Fixes: #8323
Reviewed-By: rvagg - Rod Vagg <[email protected]>
@bnoordhuis bnoordhuis mentioned this pull request Sep 12, 2016
2 tasks
ofrobots pushed a commit to ofrobots/node that referenced this pull request Sep 12, 2016
targos added a commit to targos/node that referenced this pull request Sep 22, 2016
Ref: nodejs#8343
Fixes: nodejs#8323

PR-URL: nodejs#8317
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
jasnell pushed a commit that referenced this pull request Sep 29, 2016
Ref: #8343
Fixes: #8323

PR-URL: #8317
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
targos added a commit to targos/node that referenced this pull request Dec 30, 2016
Ref: nodejs#8343
Fixes: nodejs#8323

PR-URL: nodejs#8317
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
targos added a commit to targos/node that referenced this pull request Feb 7, 2017
Fixes: nodejs#8323
Refs: nodejs#8343
Refs: nodejs#8317

PR-URL: nodejs#11029
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
Fixes: nodejs#8323
Refs: nodejs#8343
Refs: nodejs#8317

PR-URL: nodejs#11029
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@targos targos mentioned this pull request Mar 29, 2017
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v6.5.0 fails to build using clang-3.4.
4 participants