-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubber-stamp LGTM
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/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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_); |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
LGTM. |
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 <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
landed in 2677b9b |
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 <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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), orvcbuild test
(Windows) passesAffected core subsystem(s)
deps: V8