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: backport ec1ffe3 from upstream V8 #12061

Closed
wants to merge 1 commit into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Mar 27, 2017

This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when make install is run.

Original commit message:

[tools] add lldbinit

The goal of this commit is to add the equivalent to gdbinit but
for lldb. I've tried to replicate the commands as close as possible
but I'm unsure about the jss command and hoping to get some feedback
on it in addition to the bta command which I'm not sure how/when
this could be used. This is probably just inexperience on my part.

The lldbinit file can be placed into a directory prefixed with dot
(.lldbinit) and the python script is currently expected to be in the
same directory. The path to the script can be changed manually if
needed as well.

NOTRY=true

Review-Url: https://codereview.chromium.org/2758373002
Cr-Commit-Position: refs/heads/master@{#44136}
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

    Review-Url: https://codereview.chromium.org/2758373002
    Cr-Commit-Position: refs/heads/master@{nodejs#44136}
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Mar 27, 2017
@mscdex mscdex removed the build Issues and PRs related to build files or the CI. label Mar 27, 2017
@richardlau
Copy link
Member

cc @nodejs/post-mortem

@danbev
Copy link
Contributor Author

danbev commented Mar 30, 2017

@richardlau Should I wait with merging this to allow more responses, with regards to the cc I mean?

@@ -133,6 +133,8 @@ def files(action):
action(['src/node.stp'], 'share/systemtap/tapset/')

action(['deps/v8/tools/gdbinit'], 'share/doc/node/')
action(['deps/v8/tools/lldbinit'], 'share/doc/node/')
action(['deps/v8/tools/lldb_commands.py'], 'share/doc/node/')

Choose a reason for hiding this comment

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

Why do we install programs in shared/doc? #2123 did the same for gdbinit, and it seems it made sense for @ofrobots and @bnoordhuis, so I'm probably just missing something.

Choose a reason for hiding this comment

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

OK, I understand now that these files need to be copied somewhere else as configuration files.

@misterdjules
Copy link

FWIW, doing a quick test , the jss and bta commands didn't output what I expected:

➜  node git:(master) lldb node_g test/parallel/test-vm-context.js
(lldb) target create "node_g"
Current executable set to 'node_g' (x86_64).
(lldb) settings set -- target.run-args  "test/parallel/test-vm-context.js"
(lldb) b node::ContextifyContext::GlobalPropertyQueryCallback(v8::Local<v8::Name>, v8::PropertyCallbackInfo<v8::Integer> const&)                                                                            Breakpoint 1: where = node_g`node::ContextifyContext::GlobalPropertyQueryCallback(v8::Local<v8::Name>, v8::PropertyCallbackInfo<v8::Integer> const&) + 19 at node_contextify.cc:446, address = 0x00000001017802d3
(lldb) r
Process 62506 launched: '/Users/JulienGilli/dev/node/node/node_g' (x86_64)
run in a new empty context
create a new pre-populated context
test updating context
test runInContext signature
test RegExp as argument to assert.throws
Process 62506 stopped
* thread #1: tid = 0xd84c3, 0x00000001017802d3 node_g`node::ContextifyContext::GlobalPropertyQueryCallback(property=(val_ = v8::Name * = 0x00007fff5fbfcd00), args=0x00007fff5fbfc420) + 19 at node_contextify.cc:446, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x00000001017802d3 node_g`node::ContextifyContext::GlobalPropertyQueryCallback(property=(val_ = v8::Name * = 0x00007fff5fbfcd00), args=0x00007fff5fbfc420) + 19 at node_contextify.cc:446
   443 	      Local<Name> property,
   444 	      const PropertyCallbackInfo<Integer>& args) {
   445 	    ContextifyContext* ctx;
-> 446 	    ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
   447 	
   448 	    // Still initializing
   449 	    if (ctx->context_.IsEmpty())
(lldb) bta
Traceback (most recent call last):
  File "/Users/JulienGilli/lldb_commands.py", line 44, in bta
    functionSignature = frame.GetDisplayFunctionName()
  File "/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Resources/Python/lldb/__init__.py", line 3912, in <lambda>
    __getattr__ = lambda self, name: _swig_getattr(self, SBFrame, name)
  File "/Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/Resources/Python/lldb/__init__.py", line 78, in _swig_getattr
    raise AttributeError(name)
AttributeError: GetDisplayFunctionName
(lldb) jss
(lldb) 

I would have expected jss to output at least some JS stack frames, and bta to not error.

I have no objection to back porting these tools to node, but it would be nice to have some kind of documentation so that users know what to expect when using it. It would also be nice to have some tests to determine whether or not these tools work as expected from a maintainers perspective.

@richardlau
Copy link
Member

Should I wait with merging this to allow more responses, with regards to the cc I mean?

The cc was mainly to bring it to the attention of the group responsible for https://github.com/nodejs/llnode

@danbev
Copy link
Contributor Author

danbev commented Mar 31, 2017

The cc was mainly to bring it to the attention of the group responsible for https://github.com/nodejs/llnode

Thanks for that, I was not aware of this project. I must take a closer look.

@danbev
Copy link
Contributor Author

danbev commented Mar 31, 2017

I would have expected jss to output at least some JS stack frames, and bta to not error.

Yeah, something is not right here. Could you tell me what version of lldb you are using (I'm using lldb-360.1.70)?

Trying to follow your exact steps I get:

$ lldb node_g test/parallel/test-vm-context.js
(lldb) target create "node_g"
Current executable set to 'node_g' (x86_64).
(lldb) settings set -- target.run-args  "test/parallel/test-vm-context.js"
(lldb) b node::ContextifyContext::GlobalPropertyQueryCallback(v8::Local<v8::Name>, v8::PropertyCallbackInfo<v8::Integer> const&)
Breakpoint 1: where = node_g`node::ContextifyContext::GlobalPropertyQueryCallback(v8::Local<v8::Name>, v8::PropertyCallbackInfo<v8::Integer> const&) + 19 at node_contextify.cc:446, address = 0x00000001016f80e3
(lldb) r
Process 40999 launched: '/Users/danielbevenius/work/nodejs/node/node_g' (x86_64)
run in a new empty context
create a new pre-populated context
test updating context
test runInContext signature
test RegExp as argument to assert.throws
Process 40999 stopped
* thread #1: tid = 0x4f432, 0x00000001016f80e3 node_g`node::ContextifyContext::GlobalPropertyQueryCallback(property=(val_ = 0x00007fff5fbfbf40), args=0x00007fff5fbfb670) + 19 at node_contextify.cc:446, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x00000001016f80e3 node_g`node::ContextifyContext::GlobalPropertyQueryCallback(property=(val_ = 0x00007fff5fbfbf40), args=0x00007fff5fbfb670) + 19 at node_contextify.cc:446
   443 	      Local<Name> property,
   444 	      const PropertyCallbackInfo<Integer>& args) {
   445 	    ContextifyContext* ctx;
-> 446 	    ASSIGN_OR_RETURN_UNWRAP(&ctx, args.Data().As<Object>());
   447
   448 	    // Still initializing
   449 	    if (ctx->context_.IsEmpty())
(lldb) bta
[0 ] node::ContextifyContext::GlobalPropertyQueryCallback         node_contextify.cc:446
[1 ] v8::internal::PropertyCallbackArguments::Call                api-arguments-inl.h:41
[2 ] v8::internal::                                               objects.cc:1695
[3 ] v8::internal::JSObject::GetPropertyAttributesWithInterceptor objects.cc:5570
[4 ] v8::internal::JSReceiver::GetPropertyAttributes              objects.cc:5584
[5 ] v8::internal::JSReceiver::GetOwnPropertyDescriptor           objects.cc:7088
[6 ] v8::internal::JSReceiver::OrdinaryDefineOwnProperty          objects.cc:6301
[7 ] v8::internal::JSReceiver::OrdinaryDefineOwnProperty          objects.cc:6288
[8 ] v8::internal::JSReceiver::DefineOwnProperty                  objects.cc:6250
[9 ] v8::internal::JSReceiver::DefineProperty                     objects.cc:6139
[10] v8::internal::Builtin_Impl_ObjectDefineProperty              builtins-object.cc:398
[11] v8::internal::Builtin_ObjectDefineProperty                   builtins-object.cc:391
[16] v8::internal::                                               execution.cc:144
[17] v8::internal::                                               execution.cc:180
[18] v8::internal::Execution::Call                                execution.cc:190
[19] v8::Script::Run                                              api.cc:2038
[20] v8::Script::Run                                              api.cc:2051
[21] node::ContextifyScript::EvalMachine                          node_contextify.cc:896
[22] node::ContextifyScript::RunInContext                         node_contextify.cc:671
[23] v8::internal::FunctionCallbackArguments::Call                api-arguments.cc:25
[24] v8::internal::MaybeHandle                                    builtins-api.cc:107
[25] v8::internal::Builtin_Impl_HandleApiCall                     builtins-api.cc:136
[26] v8::internal::Builtin_HandleApiCall                          builtins-api.cc:124
[45] v8::internal::                                               execution.cc:144
[46] v8::internal::                                               execution.cc:180
[47] v8::internal::Execution::Call                                execution.cc:190
[48] v8::Function::Call                                           api.cc:5101
[49] v8::Function::Call                                           api.cc:5110
[50] node::LoadEnvironment                                        node.cc:3512
[51] node::Start                                                  node.cc:4469
[52] node::Start                                                  node.cc:4546
[53] node::Start                                                  node.cc:4601
[54] main                                                         node_main.cc:79
[55] start
(lldb)

I also get output from the jst command but it is a little too lengthy to list here.

process = target.GetProcess()
thread = process.GetSelectedThread()
frame = thread.GetSelectedFrame()
frame.EvaluateExpression("_v8_internal_Print_StackTrace();")
Copy link
Contributor

Choose a reason for hiding this comment

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

On my system this needs to be frame.EvaluateExpression("(void)_v8_internal_Print_StackTrace();")
If you run it manually lldb says the expression needs a return type:

(lldb) p _v8_internal_Print_StackTrace()
error: '_v8_internal_Print_StackTrace' has unknown return type; cast the call to its declared return type

@hhellyer
Copy link
Contributor

Unfortunately I don't think this script works post-mortem. The jss and jst commands rely on being able to evaluate expressions (and run C/C++ functions in the process) which you can do with a live process but not from a core dump. (I'll leave open the possibility that there's some magic you can do to make it work but a quick test locally suggests not!) The bta command does work with a core but it doesn't evaluate any expressions.

This is definitely a useful tool to have it just fits a slightly different use case to llnode, live debugging rather than debugging from a core file.

@richardlau - Sorry should have taken a look and responded sooner.

@bnoordhuis
Copy link
Member

FWIW, my LGTM still stands. As Howard says, it's still useful for live debugging. Besides, it has been accepted upstream.

@misterdjules
Copy link

@danbev

Could you tell me what version of lldb you are using (I'm using lldb-360.1.70)?

➜  node git:(master) lldb --version
lldb-320.4.156
➜  node git:(master)

@danbev
Copy link
Contributor Author

danbev commented Apr 3, 2017

@hhellyer Would you be alright with me merging this? If there are improvements/fixes to allow this to work on a core file I think it might be better to add these upstream in that case.

@hhellyer
Copy link
Contributor

hhellyer commented Apr 3, 2017

@danbev - I don't think I have the power to approve or deny a merge. I'm not that important. :-)

I think it only works on debug builds (otherwise v8::internal::Isolate::Current() doesn't seem to evaluate for me) but the gdbinit file is installed in the same way so I'm assuming that's fine - it doesn't add any overhead unless you are debugging.

@jasnell
Copy link
Member

jasnell commented Apr 3, 2017

@hhellyer ... opinions are still valuable and important!

/cc @nodejs/v8 ... thoughts?

@misterdjules
Copy link

@danbev

@hhellyer Would you be alright with me merging this? If there are improvements/fixes to allow this to work on a core file I think it might be better to add these upstream in that case.

I assumed that the use case for these tools was exclusively live debugging, not post-mortem debugging. Is it what you had in mind too?

I don't think any improvement or fix will ever make this work with core files and/or post-mortem, since it would need to use a fundamentally different approach.

That's not a problem since live debugging is a valid use case, and the post-mortem debugging use case is covered by nodejs/llnode on the platforms on which the tools added by this PR run.

Having some documentation that clarifies the use case (live debugging), requirements (lldb version, how node needs to be built, etc.) and example of commands output would go a long way toward avoiding confusion.

@danbev
Copy link
Contributor Author

danbev commented Apr 4, 2017

I assumed that the use case for these tools was exclusively live debugging, not post-mortem debugging. Is it what you had in mind too?

Yeah, that was the only use case I had in mind

@danbev
Copy link
Contributor Author

danbev commented Apr 5, 2017

Having some documentation that clarifies the use case (live debugging), requirements (lldb version, how node needs to be built, etc.) and example of commands output would go a long way toward avoiding confusion.

I've tried to find some docs about gdbinit which already exists but I've not found any yet. If there is none perhaps there should be a separate issue/pull request for both gdbinit and lldbinit?
If no one objects I'll create an issue for this and merge this pull request later today.

danbev added a commit to danbev/node that referenced this pull request Apr 6, 2017
This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

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

PR-URL: nodejs#12061
Reviewed-By: Ben Noordhuis <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented Apr 6, 2017

Landed in 23498f2

@danbev danbev closed this Apr 6, 2017
@danbev danbev deleted the v8-lldbinit branch April 6, 2017 05:51
italoacasas pushed a commit to italoacasas/node that referenced this pull request Apr 10, 2017
This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

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

PR-URL: nodejs#12061
Reviewed-By: Ben Noordhuis <[email protected]>
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
2 tasks
@MylesBorins
Copy link
Contributor

is this useful to backport? /cc @nodejs/post-mortem @nodejs/v8

@ofrobots
Copy link
Contributor

+1 on backporting from me.

MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

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

PR-URL: #12061
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

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

PR-URL: #12061
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

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

PR-URL: #12061
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 24, 2017
This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

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

PR-URL: #12061
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit to targos/node that referenced this pull request May 6, 2017
This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

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

PR-URL: nodejs#12061
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request May 6, 2017
This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

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

PR-URL: #12061
Reviewed-By: Ben Noordhuis <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

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

PR-URL: nodejs#12061
Reviewed-By: Ben Noordhuis <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
This commit adds lldbinit files from upstream V8 and also adds these so
that they get installed when `make install` is run.

Original commit message:

[tools] add lldbinit

    The goal of this commit is to add the equivalent to gdbinit but
    for lldb. I've tried to replicate the commands as close as possible
    but I'm unsure about the jss command and hoping to get some feedback
    on it in addition to the bta command which I'm not sure how/when
    this could be used. This is probably just inexperience on my part.

    The lldbinit file can be placed into a directory prefixed with dot
    (.lldbinit) and the python script is currently expected to be in the
    same directory. The path to the script can be changed manually if
    needed as well.

    NOTRY=true

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

PR-URL: nodejs/node#12061
Reviewed-By: Ben Noordhuis <[email protected]>
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.

10 participants