Skip to content

deps: backport 224d376 from V8 upstream - #10526

Closed
jBarz wants to merge 1 commit into
nodejs:masterfrom
jBarz:v8_new_delete
Closed

deps: backport 224d376 from V8 upstream#10526
jBarz wants to merge 1 commit into
nodejs:masterfrom
jBarz:v8_new_delete

Conversation

@jBarz

@jBarzjBarz commented Dec 29, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

Orignial commit message:
Abort in delete operators that shouldn't be called.

Section 3.2 of the C++ standard states that destructor
definitions implicitly "use" operator delete functions.
Therefore, these operator delete functions must be
defined even if they are never called by user code
explicitly.
http://www.open-std.org/JTC1/SC22/WG21/docs/
cwg_defects.html#261

gcc allows them to remain as empty definitions. However,
not all compilers allow this. (e.g. xlc on zOS). This pull
request creates definitions which if ever called, result
in an abort.

R=danno@chromium.org,jochen@chromium.org
BUG=
LOG=N

Review-Url: https://codereview.chromium.org/2588433002
Cr-Commit-Position: refs/heads/master@{#41981}

Checklist
  • make -j4 test (UNIX)
  • commit message follows commit guidelines
Affected core subsystem(s)

v8

@nodejs-github-botnodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Dec 29, 2016
@jBarz

jBarz commented Dec 29, 2016

Copy link
Copy Markdown
ContributorAuthor

cc @gibfahn

@sam-github

sam-github commented Dec 29, 2016

Copy link
Copy Markdown
Contributor

LGTM

Did you run into this as a problem on a non-gcc system?

@jBarz

jBarz commented Dec 29, 2016

Copy link
Copy Markdown
ContributorAuthor

I ran into this on z/OS which uses the xlc compiler.

@gibfahn

gibfahn commented Dec 30, 2016

Copy link
Copy Markdown
Member

I think the V8 patch level needs to be updated. The process should be documented in the updating v8 guide.

mhdawson
mhdawson approved these changes Jan 4, 2017
View reviewed changes

@mhdawsonmhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson

mhdawson commented Jan 4, 2017

Copy link
Copy Markdown
Member

CI runs: https://ci.nodejs.org/job/node-test-pull-request/5703/
https://ci.nodejs.org/job/node-test-commit-v8-linux/493/

@jasnell

jasnell commented Jan 6, 2017

Copy link
Copy Markdown
Member

Weird failure in the arm CI job. Appears unrelated but just in case: https://ci.nodejs.org/job/node-test-pull-request/5730/

@jBarz

jBarz commented Jan 6, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

It is weird because when I click on the link for details, the test appears to have passed. :-)
Unless I am missing something

@jasnell

jasnell commented Jan 6, 2017

Copy link
Copy Markdown
Member

The results showing now are for the new run I just kicked off... which does appear to have passed

@gibfahn

gibfahn commented Jan 6, 2017

Copy link
Copy Markdown
Member

The Github check sometimes wrongly reports that test/arm failed (nodejs/build#572), it's nothing to worry about.

@gibfahn

gibfahn commented Jan 6, 2017

Copy link
Copy Markdown
Member

@jBarz do the changes in include/v8.h from v8/v8@224d37 not need to be included?

@jBarz

jBarz commented Jan 7, 2017

Copy link
Copy Markdown
ContributorAuthor

@gib: v8.h in the master branch in chromium underwent some changes that needed to be reversed.
But node has not absorbed those changes yet so this PR does not need them.

gibfahn
gibfahn approved these changes Jan 7, 2017
View reviewed changes

@gibfahngibfahn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case LGTM (in that it matches the upstream commit). Should be reviewed by @nodejs/v8 and/or @MylesBorins though.

ofrobots
ofrobots approved these changes Jan 10, 2017
View reviewed changes

@ofrobotsofrobots left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w/ nit: the commit abstract should say 'backport' as this is not a clean cherry-pick.

@ofrobots

ofrobots commented Jan 18, 2017

Copy link
Copy Markdown
Contributor

Another nit: prefer 7-character commit ids.

@jBarz
deps: backport 224d376 from V8 upstream
99c7f8c
Orignial commit message:
Abort in delete operators that shouldn't be called.
Section 3.2 of the C++ standard states that destructor
definitions implicitly "use" operator delete functions.
Therefore, these operator delete functions must be
defined even if they are never called by user code
explicitly.
http://www.open-std.org/JTC1/SC22/WG21/docs/
cwg_defects.html#261
gcc allows them to remain as empty definitions. However,
not all compilers allow this. (e.g. xlc on zOS). This pull
request creates definitions which if ever called, result
in an abort.
R=danno@chromium.org,jochen@chromium.org
BUG=
LOG=N
Review-Url: https://codereview.chromium.org/2588433002
Cr-Commit-Position: refs/heads/master@{nodejs#41981}
@jBarz

jBarz commented Jan 18, 2017

Copy link
Copy Markdown
ContributorAuthor

addressed nits

@jBarzjBarz changed the title deps: cherry-pick 224d37 from V8 upstreamdeps: cherry-pick 224d376 from V8 upstreamJan 18, 2017
@jBarzjBarz changed the title deps: cherry-pick 224d376 from V8 upstreamdeps: backport 224d376 from V8 upstreamJan 18, 2017
@ofrobots

ofrobots commented Jan 19, 2017

Copy link
Copy Markdown
Contributor

Thanks. LGTM.

bnoordhuis
bnoordhuis approved these changes Jan 19, 2017
View reviewed changes

@bnoordhuisbnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @jBarz Did you request backports to 5.5 and 5.6?

@jBarz

jBarz commented Jan 19, 2017

Copy link
Copy Markdown
ContributorAuthor

oh no :-(
I will do that

@jBarz

jBarz commented Jan 19, 2017

Copy link
Copy Markdown
ContributorAuthor

I have requested backports to 5.5, 5.6.

@targos

targos commented Jan 28, 2017•
edited
Loading

Copy link
Copy Markdown
Member

Does it need to be applied to v7, v6 or v4?
What is the status on the backport request?

@jBarz

jBarz commented Jan 28, 2017

Copy link
Copy Markdown
ContributorAuthor

This fix is required on v6.x and above.
The backport to node v6.x was successfull (#10546)
Backport to v8 5.5 was rejected (too late). Doesn't matter I think because of #10546
Backport to v8 5.6 was accepted.

@jasnell

jasnell commented Mar 24, 2017

Copy link
Copy Markdown
Member

Ping. any updates on this one?

@jasnelljasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017
@gibfahn

gibfahn commented Mar 25, 2017

Copy link
Copy Markdown
Member

@jBarz if you could rebase and confirm this is still needed, we should get it landed, especially if it's already gone into v6.x

@jBarz

jBarz commented Mar 25, 2017

Copy link
Copy Markdown
ContributorAuthor

This backport is no longer needed because it is already backported via #11752

@jBarzjBarz closed this Mar 25, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes
+3 more reviewers
@ofrobotsofrobotsofrobots approved these changes
@mhdawsonmhdawsonmhdawson approved these changes
@gibfahngibfahngibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

stalledIssues and PRs that are stalled.v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jBarz@sam-github@gibfahn@mhdawson@jasnell@ofrobots@targos@bnoordhuis@nodejs-github-bot