src: support thin strings and stub frames by joyeecheung · Pull Request #121 · nodejs/llnode

joyeecheung · 2017-08-15T15:28:17Z

Fixes: #117

In Node.js v8.3.0 Turbofan has been enabled by default, so thin strings have also been enabled and now Turbofan would put stub frames on the stack (to store register states for bytecode handlers, I think).

Before this change, when viewing the backtrace of test/fixtures/inspect-scenario.js with Node.js v8.3.0, frames containing functions with thin string arguments or in thin string paths would be broken:

See backtrace of inspect-scenario.js

 * thread #1: tid = 0x81d450, 0x0000000100bbe942 node`v8::base::OS::Abort() at platform-posix.cc:261, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
  * frame #0: 0x0000000100bbe942 node`v8::base::OS::Abort() at platform-posix.cc:261 [opt]
    frame #1: 0x00000001008a27cb node`v8::internal::Runtime_Throw(int, v8::internal::Object**, v8::internal::Isolate*) [inlined] v8::internal::__RT_impl_Runtime_Throw(v8::internal::Arguments, v8::internal::Isolate*) at runtime-internal.cc:74 [opt]
    frame #2: 0x00000001008a27aa node`v8::internal::Runtime_Throw(args_length=<unavailable>, args_object=<unavailable>, isolate=0x00000001030492e0) at runtime-internal.cc:71 [opt]
    frame #3: 0x00001a1235b840dd <exit>
    frame #4: 0x00001a1235c8523a
    frame #5: 0x00001a1235b92f12 method(this=0x00002976af95c089:<Object: Class>) at (no script):20:43 fn=0x000026033eb8f931
    frame #6: 0x00001a1235c735e9
    frame #7: 0x00001a1235b92f12 closure(this=0x000022d8c1402241:<undefined>) at (no script):11:17 fn=0x00002976af905ad9
    frame #8: 0x00001a1235c73fa1
    frame #9: 0x00001a1235b92f12
    frame #10: 0x00001a1235c72cc8
    frame #11: 0x00001a1235b92f12
    frame #12: 0x00001a1235c738aa
    frame #13: 0x00001a1235b92f12
    frame #14: 0x00001a1235c738aa
    frame #15: 0x00001a1235b92f12
    frame #16: 0x00001a1235c7373c
    frame #17: 0x00001a1235b92f12
    frame #18: 0x00001a1235c74242
    frame #19: 0x00001a1235b92f12 Module._load(this=0x0000135cdc0bb3b1:<function: Module at module.js:55:16>, 0x0000135cdc0c7119:<String: "/Users/joyee/pro...">, 0x000022d8c1402211:<null>, 0x000022d8c14022c1:<true>) at module.js:434:24 fn=0x000012d14eec2e29
    frame #20: 0x00001a1235c72cc8
    frame #21: 0x00001a1235b92f12 Module.runMain(this=0x0000135cdc0bb3b1:<function: Module at module.js:55:16>) at module.js:607:26 fn=0x000012d14eec3099
    frame #22: 0x00001a1235c93c29
    frame #23: 0x00001a1235b92f12 startup(this=0x000022d8c1402241:<undefined>) at bootstrap_node.js:1:10 fn=0x0000304c372feb79
    frame #24: 0x00001a1235c73fa1
    frame #25: 0x00001a1235b92f12 (anonymous)(this=0x000022d8c1402211:<null>, 0x0000304c372febf9:<Object: process>) at bootstrap_node.js:1:10 fn=0x0000304c372fec49
    frame #26: 0x00001a1235b90650 <internal>
    frame #27: 0x00001a1235c64ead <entry>
    frame #28: 0x00000001005daed1 node`v8::internal::(anonymous namespace)::Invoke(isolate=0x0000000000000049, is_construct=<unavailable>, target=<unavailable>, receiver=<unavailable>, argc=1, args=<unavailable>, new_target=<unavailable>, message_handling=<unavailable>) at execution.cc:145 [opt]
    frame #29: 0x00000001005dabb9 node`v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [inlined] v8::internal::(anonymous namespace)::CallInternal(message_handling=kReport) at execution.cc:181 [opt]
    frame #30: 0x00000001005dab30 node`v8::internal::Execution::Call(isolate=0x0000000103000c00, callable=Handle<v8::internal::Object> @ r12, receiver=<unavailable>, argc=1, argv=<unavailable>) at execution.cc:191 [opt]
    frame #31: 0x0000000100204b98 node`v8::Function::Call(this=0x0000000103049238, context=<unavailable>, recv=<unavailable>, argc=1, argv=0x00007fff5fbfe1f0) at api.cc:5279 [opt]
    frame #32: 0x0000000100a4d083 node`node::LoadEnvironment(env=0x00007fff5fbfe290) at node.cc:3613 [opt]
    frame #33: 0x0000000100a53de8 node`node::Start(isolate=0x0000000103000c00, isolate_data=<unavailable>, argc=2, argv=<unavailable>, exec_argc=1, exec_argv=0x0000000102503c70) at node.cc:4528 [opt]
    frame #34: 0x0000000100a4e9a0 node`node::Start(event_loop=0x000000010154ef90, argc=2, argv=0x00000001025043a0, exec_argc=<unavailable>, exec_argv=<unavailable>) at node.cc:4607 [opt]
    frame #35: 0x0000000100a4e62d node`node::Start(argc=<unavailable>, argv=0x00000001025043a0) at node.cc:4662 [opt]
    frame #36: 0x0000000100001434 node`start + 52

After (with patch in #117 (comment)):

See backtrace of inspect-scenario.js

 * thread #1: tid = 0x81cad8, 0x0000000100bbe942 node`v8::base::OS::Abort() at platform-posix.cc:261, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
  * frame #0: 0x0000000100bbe942 node`v8::base::OS::Abort() at platform-posix.cc:261 [opt]
    frame #1: 0x00000001008a27cb node`v8::internal::Runtime_Throw(int, v8::internal::Object**, v8::internal::Isolate*) [inlined] v8::internal::__RT_impl_Runtime_Throw(v8::internal::Arguments, v8::internal::Isolate*) at runtime-internal.cc:74 [opt]
    frame #2: 0x00000001008a27aa node`v8::internal::Runtime_Throw(args_length=<unavailable>, args_object=<unavailable>, isolate=0x00000001030492e0) at runtime-internal.cc:71 [opt]
    frame #3: 0x000016c7041040dd <exit>
    frame #4: 0x000016c70420523a <stub>
    frame #5: 0x000016c704112f12 method(this=0x00000a7bd42dc201:<Object: Class>) at /Users/joyee/projects/llnode/test/fixtures/inspect-scenario.js:27:43 fn=0x00003aeebdc8faa9
    frame #6: 0x000016c7041f35e9 <stub>
    frame #7: 0x000016c704112f12 closure(this=0x0000156976502241:<undefined>) at /Users/joyee/projects/llnode/test/fixtures/inspect-scenario.js:18:17 fn=0x00000a7bd4285c81
    frame #8: 0x000016c7041f3fa1 <stub>
    frame #9: 0x000016c704112f12 (anonymous)(this=0x00000a7bd4283a61:<Object: Object>, 0x00000a7bd4283a61:<Object: Object>, 0x00000a7bd42857e9:<function: require at (external).js:8:19>, 0x00000a7bd42839a1:<Object: Module>, 0x00000a7bd4282dd9:<String: "/Users/joyee/pro...">, 0x00000a7bd4285781:<String: "/Users/joyee/pro...">) at /Users/joyee/projects/llnode/test/fixtures/inspect-scenario.js:1:10 fn=0x00000a7bd4285739
    frame #10: 0x000016c7041f2cc8 <stub>
    frame #11: 0x000016c704112f12 Module._compile(this=0x00000a7bd42839a1:<Object: Module>, 0x00000a7bd42847b1:<String: "'use strict';

c...">, 0x00000a7bd4282dd9:<String: "/Users/joyee/pro...">) at module.js:530:37 fn=0x0000170c90843011
    frame #12: 0x000016c7041f38aa <stub>
    frame #13: 0x000016c704112f12 Module._extensions..js(this=0x000037f6cb67e271:<Object: Object>, 0x00000a7bd42839a1:<Object: Module>, 0x00000a7bd4282dd9:<String: "/Users/joyee/pro...">) at module.js:582:37 fn=0x0000170c90843059
    frame #14: 0x000016c7041f38aa <stub>
    frame #15: 0x000016c704112f12 Module.load(this=0x00000a7bd42839a1:<Object: Module>, 0x00000a7bd4282dd9:<String: "/Users/joyee/pro...">) at module.js:498:33 fn=0x0000170c90842f81
    frame #16: 0x000016c7041f373c <stub>
    frame #17: 0x000016c704112f12 tryModuleLoad(this=0x0000156976502241:<undefined>, 0x00000a7bd42839a1:<Object: Module>, 0x00000a7bd4282dd9:<String: "/Users/joyee/pro...">) at module.js:467:23 fn=0x000037f6cb63b561
    frame #18: 0x000016c7041f4242 <stub>
    frame #19: 0x000016c704112f12 Module._load(this=0x000037f6cb63b3b1:<function: Module at module.js:55:16>, 0x000037f6cb647081:<String: "/Users/joyee/pro...">, 0x0000156976502211:<null>, 0x00001569765022c1:<true>) at module.js:434:24 fn=0x0000170c90842ec1
    frame #20: 0x000016c7041f2cc8 <stub>
    frame #21: 0x000016c704112f12 Module.runMain(this=0x000037f6cb63b3b1:<function: Module at module.js:55:16>) at module.js:607:26 fn=0x0000170c90843131
    frame #22: 0x000016c704213c29 <stub>
    frame #23: 0x000016c704112f12 startup(this=0x0000156976502241:<undefined>) at bootstrap_node.js:1:10 fn=0x000027f86ea7eb79
    frame #24: 0x000016c7041f3fa1 <stub>
    frame #25: 0x000016c704112f12 (anonymous)(this=0x0000156976502211:<null>, 0x000027f86ea7ebf9:<Object: process>) at bootstrap_node.js:1:10 fn=0x000027f86ea7ec49
    frame #26: 0x000016c704110650 <internal>
    frame #27: 0x000016c7041e4ead <entry>
    frame #28: 0x00000001005daed1 node`v8::internal::(anonymous namespace)::Invoke(isolate=0x0000000000000049, is_construct=<unavailable>, target=<unavailable>, receiver=<unavailable>, argc=1, args=<unavailable>, new_target=<unavailable>, message_handling=<unavailable>) at execution.cc:145 [opt]
    frame #29: 0x00000001005dabb9 node`v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) [inlined] v8::internal::(anonymous namespace)::CallInternal(message_handling=kReport) at execution.cc:181 [opt]
    frame #30: 0x00000001005dab30 node`v8::internal::Execution::Call(isolate=0x0000000103000c00, callable=Handle<v8::internal::Object> @ r12, receiver=<unavailable>, argc=1, argv=<unavailable>) at execution.cc:191 [opt]
    frame #31: 0x0000000100204b98 node`v8::Function::Call(this=0x0000000103049238, context=<unavailable>, recv=<unavailable>, argc=1, argv=0x00007fff5fbfe1f0) at api.cc:5279 [opt]
    frame #32: 0x0000000100a4d083 node`node::LoadEnvironment(env=0x00007fff5fbfe290) at node.cc:3613 [opt]
    frame #33: 0x0000000100a53de8 node`node::Start(isolate=0x0000000103000c00, isolate_data=<unavailable>, argc=2, argv=<unavailable>, exec_argc=1, exec_argv=0x0000000102503c70) at node.cc:4528 [opt]
    frame #34: 0x0000000100a4e9a0 node`node::Start(event_loop=0x000000010154ef90, argc=2, argv=0x00000001025043a0, exec_argc=<unavailable>, exec_argv=<unavailable>) at node.cc:4607 [opt]
    frame #35: 0x0000000100a4e62d node`node::Start(argc=<unavailable>, argv=0x00000001025043a0) at node.cc:4662 [opt]
    frame #36: 0x0000000100001434 node`start + 52

Also display stub frames as <stub> to fix frame-test.js.

This would need a patch in v8 to get the kThinStringTag constant: #117 (comment)

Tested with v4.8.4, v6.11.1, v8.2.1 and v8.3.0(with patch from #117 (comment))

joyeecheung · 2017-08-16T12:03:16Z

@indutny Doing make format on my mac results in:

clang-format -i src/*
Problem while looking for clang-format in Chromium source tree:
Could not find checkout in any parent of the current path.
Set CHROMIUM_BUILDTOOLS_PATH to use outside of a chromium checkout.
make: *** [format] Error 1

I have a (very old) chromium checkout on my machine but setting CHROMIUM_BUILDTOOLS_PATH to chromium/src/buildtools then run clang-format -i src/* all I got was silence :/...I will try to update my chromium checkout (not sure if that's the right way to fix this though).

Fixes: nodejs#117

joyeecheung · 2017-08-16T12:11:18Z

OK, turns out silence was what it was supposed to be, the file would be formatted directly by the script. o.0 I've fixed the style, PTAL @indutny @bnoordhuis

bnoordhuis · 2017-08-17T13:18:01Z

Still LGTM. I'm trying to sort out https://ci.nodejs.org/view/post-mortem/job/llnode-pipeline/ but you can land this if you want, @joyeecheung.

bnoordhuis · 2017-08-17T14:15:02Z

For good measure: https://ci.nodejs.org/view/post-mortem/job/llnode-pipeline/13/

Original commit message: Add postmortem metadata for thin strings. See: nodejs/llnode#117 Change-Id: Icc2830c8e9096610df33ffdc2f89e74cb1b35662 Reviewed-on: https://chromium-review.googlesource.com/618986 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl> Cr-Commit-Position: refs/heads/master@{nodejs#47778} Refs: nodejs/llnode#117 Refs: nodejs/llnode#121

Original commit message: Add postmortem metadata for thin strings. See: nodejs/llnode#117 Change-Id: Icc2830c8e9096610df33ffdc2f89e74cb1b35662 Reviewed-on: https://chromium-review.googlesource.com/618986 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl> Cr-Commit-Position: refs/heads/master@{#47778} PR-URL: #15184 Ref: nodejs/llnode#117 Ref: nodejs/llnode#121 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: Add postmortem metadata for thin strings. See: nodejs/llnode#117 Change-Id: Icc2830c8e9096610df33ffdc2f89e74cb1b35662 Reviewed-on: https://chromium-review.googlesource.com/618986 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl> Cr-Commit-Position: refs/heads/master@{nodejs#47778} PR-URL: nodejs#15184 Ref: nodejs/llnode#117 Ref: nodejs/llnode#121 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: Add postmortem metadata for thin strings. See: nodejs/llnode#117 Change-Id: Icc2830c8e9096610df33ffdc2f89e74cb1b35662 Reviewed-on: https://chromium-review.googlesource.com/618986 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl> Cr-Commit-Position: refs/heads/master@{#47778} PR-URL: #15184 Ref: nodejs/llnode#117 Ref: nodejs/llnode#121 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: Add postmortem metadata for thin strings. See: nodejs/llnode#117 Change-Id: Icc2830c8e9096610df33ffdc2f89e74cb1b35662 Reviewed-on: https://chromium-review.googlesource.com/618986 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl> Cr-Commit-Position: refs/heads/master@{nodejs#47778} PR-URL: nodejs#15184 Ref: nodejs/llnode#117 Ref: nodejs/llnode#121 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: Add postmortem metadata for thin strings. See: nodejs/llnode#117 Change-Id: Icc2830c8e9096610df33ffdc2f89e74cb1b35662 Reviewed-on: https://chromium-review.googlesource.com/618986 Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Ben Noordhuis <info@bnoordhuis.nl> Cr-Commit-Position: refs/heads/master@{#47778} Backport-PR-URL: #15393 PR-URL: #15184 Ref: nodejs/llnode#117 Ref: nodejs/llnode#121 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

joyeecheung requested a review from bnoordhuis August 15, 2017 15:28

This comment was marked as off-topic.

Sign in to view

joyeecheung force-pushed the thin-stub branch from 16fca63 to 178882b Compare August 16, 2017 12:10

joyeecheung added 2 commits August 16, 2017 20:10

src: support thin strings

1ce2a40

Fixes: nodejs#117

src: support stub frames

178882b

This comment was marked as off-topic.

Sign in to view

hhellyer merged commit 58d2c1a into nodejs:master Aug 22, 2017

mcollina mentioned this pull request Aug 24, 2017

dtrace ustack helper broken with turbofan nodejs/node#15010

Closed

bnoordhuis mentioned this pull request Sep 4, 2017

deps: cherry-pick 1aead19 from upstream V8 nodejs/node#15184

Closed

joyeecheung mentioned this pull request Nov 22, 2017

Meta data for llnode - Node.js version 8 nodejs/post-mortem#50

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: support thin strings and stub frames#121

src: support thin strings and stub frames#121
hhellyer merged 2 commits intonodejs:masterfrom
joyeecheung:thin-stub

joyeecheung commented Aug 15, 2017 •

edited

Loading

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

joyeecheung commented Aug 16, 2017 •

edited

Loading

Uh oh!

joyeecheung commented Aug 16, 2017 •

edited

Loading

Uh oh!

This comment was marked as off-topic.

Uh oh!

bnoordhuis commented Aug 17, 2017

Uh oh!

bnoordhuis commented Aug 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

joyeecheung commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

joyeecheung commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

bnoordhuis commented Aug 17, 2017

Uh oh!

bnoordhuis commented Aug 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joyeecheung commented Aug 15, 2017 •

edited

Loading

joyeecheung commented Aug 16, 2017 •

edited

Loading

joyeecheung commented Aug 16, 2017 •

edited

Loading