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: V8: fix backtrace for symbols #10732

Closed
wants to merge 2 commits into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Jan 11, 2017

The cherry-pick of #7612 to v4.x (4369055) added in #9298 wasn't quite correct as it depends on a runtime function %SymbolDescriptiveString that doesn't exist on v4.x (V8 4.5). We can use %SymbolDescription instead.

It seems that the V8-CI was missed when when the #7612 was backported to v4.x so we didn't catch it at that point. At this point however, the V8-CI is no longer passing because of the V8 test failures caused by the above.

R=@hashseed, @nodejs/v8
/cc @MylesBorins

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

deps: V8

The cherry-pick of nodejs#7612 to v4.x (4369055) added in nodejs#9298 wasn't quite
correct as it depends on a runtime function %SymbolDescriptiveString
that doesn't exist on v4.x. We can use %SymbolDescription instead.

Ref: nodejs#7612
Ref: nodejs#9298
@nodejs-github-bot nodejs-github-bot added v4.x v8 engine Issues and PRs related to the V8 dependency. labels Jan 11, 2017
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM

@MylesBorins MylesBorins self-assigned this Jan 11, 2017
@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 11, 2017

Thanks so much for digging into this! going to run ci and land this if it is green

ci: https://ci.nodejs.org/job/node-test-pull-request/5806/
V8-ci: https://ci.nodejs.org/job/node-test-commit-v8-linux/518/

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a question.

@@ -1515,7 +1515,7 @@ PropertyMirror.prototype.name = function() {


PropertyMirror.prototype.toText = function() {
if (IS_SYMBOL(this.name_)) return %SymbolDescriptiveString(this.name_);
if (IS_SYMBOL(this.name_)) return %SymbolDescription(this.name_);
Copy link
Member

Choose a reason for hiding this comment

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

Should the argument be couched in a %_ValueOf(...)? I see other calls to %SymbolDescription() do that but I presume that's for the case when IS_SYMBOL(x) === false && IS_SYMBOL_WRAPPER(x) === true.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. %_ValueOf is only needed for object wrappers around primitives. Being a property key, it cannot be a wrapper here.

@hashseed
Copy link
Member

LGTM.

MylesBorins pushed a commit that referenced this pull request Jan 12, 2017
The cherry-pick of #7612 to v4.x (4369055) added in #9298 wasn't quite
correct as it depends on a runtime function %SymbolDescriptiveString
that doesn't exist on v4.x. We can use %SymbolDescription instead.

Ref: #7612
Ref: #9298

PR-URL: #10732
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 2677b9b

MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
The cherry-pick of #7612 to v4.x (4369055) added in #9298 wasn't quite
correct as it depends on a runtime function %SymbolDescriptiveString
that doesn't exist on v4.x. We can use %SymbolDescription instead.

Ref: #7612
Ref: #9298

PR-URL: #10732
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
The cherry-pick of #7612 to v4.x (4369055) added in #9298 wasn't quite
correct as it depends on a runtime function %SymbolDescriptiveString
that doesn't exist on v4.x. We can use %SymbolDescription instead.

Ref: #7612
Ref: #9298

PR-URL: #10732
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Feb 22, 2017
The cherry-pick of nodejs#7612 to v4.x (4369055) added in nodejs#9298 wasn't quite
correct as it depends on a runtime function %SymbolDescriptiveString
that doesn't exist on v4.x. We can use %SymbolDescription instead.

Ref: nodejs/node#7612
Ref: nodejs/node#9298

PR-URL: nodejs/node#10732
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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.

9 participants