Skip to content

Conversation

@mmarchini
Copy link
Contributor

Error objects have a .stack property, which is actually an accessor. The
stack is stored in a unnamed symbol property and converted to a string
when .stack is accessed in JavaScript. This patch implements the
accessor behavior to print the error stack when inspecting an Error
object.

Fixes: #218

@mmarchini
Copy link
Contributor Author

Jenkins CI: https://ci.nodejs.org/view/post-mortem/job/llnode-pipeline/31/

(first try, let's see how it goes)

Error objects have a .stack property, which is actually an accessor. The
stack is stored in a unnamed symbol property and converted to a string
when .stack is accessed in JavaScript. This patch implements the
accessor behavior to print the error stack when inspecting an Error
object.

Fixes: nodejs#218
cjihrig

This comment was marked as off-topic.

@mmarchini
Copy link
Contributor Author

@cjihrig thank you for your review! I addressed your comments, PTAL.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Looks like the test is happy enough, so LGTM. Personally I'd prefer we use constant names instead of magic numbers, but that can be refactored later


int64_t stack_len = Smi(maybe_stack_len).GetValue();

int multiplier = 5;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be more maintainable if FrameArray::kElementsPerFrame is exposed in the post mortem metadata and gets loaded here..(not sure if that's on 6.x but it's still in TOT)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have this on my TODO list and will probably get to that at some point.

// TODO (mmarchini): Refactor: create an StackIterator which returns
// StackFrame objects
for (int64_t i = 0; i < stack_len; i++) {
Value maybe_fn = arr.GetArrayElement(2 + (i * multiplier), err);
Copy link
Member

Choose a reason for hiding this comment

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

So this is actually kFirstIndex + i * kElementsPerFrame + kFunctionOffset right?

mmarchini pushed a commit that referenced this pull request Nov 9, 2018
Error objects have a .stack property, which is actually an accessor. The
stack is stored in a unnamed symbol property and converted to a string
when .stack is accessed in JavaScript. This patch implements the
accessor behavior to print the error stack when inspecting an Error
object.

Fixes: #218

PR-URL: #233
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@mmarchini
Copy link
Contributor Author

Landed in bb0d400

@mmarchini mmarchini closed this Nov 9, 2018
@misterdjules
Copy link

@mmarchini Did this make it into a release? If not would that be possible to release a version of llnode that includes this?

@mmarchini
Copy link
Contributor Author

Yes, I'll prepare a release today, which should be released in the next few days :)

(Want to make sure we have a new release before #246 lands)

@misterdjules
Copy link

misterdjules commented Nov 29, 2018

@mmarchini I was inspecting a core dump generated from node 6.x and I was trying to get the actual stack trace for the error, and when trying to get the full stack trace with `v8 inspect -l length-of-stack-array' I constantly get a segmentation fault:

$ lldb ~/Downloads/node-v6.10.3-linux-x64/bin/node -c /path/to/core/dump
(lldb) target create "/Users/jgilli/Downloads/node-v6.10.3-linux-x64/bin/node" --core "/path/to/core/dump"
Core file '/path/to/core/dump' (x86_64) was loaded.
(lldb) plugin load /Users/jgilli/dev/llnode/llnode.dylib
(lldb) v8 bt
 * thread #1: tid = 1024, 0x0000000001334fa9 node`v8::base::OS::Abort() + 9, name = 'node', stop reason = signal SIGILL
  * frame #0: 0x0000000001334fa9 node`v8::base::OS::Abort() + 9
    frame #1: 0x0000000000d5241a node`v8::internal::Isolate::Throw(v8::internal::Object*, v8::internal::MessageLocation*) + 490
    frame #2: 0x0000000000eb4e33 node`v8::internal::Runtime_Throw(int, v8::internal::Object**, v8::internal::Isolate*) + 307
    frame #3: 0x000035e3c31092a7 <exit>
    frame #4: 0x000035e3c58ec96b throwError(this=0x2ad297b2fc41:<Object: Immediate>, 0x2ad297b2f569:<Object: TypeError>) 
[...redacted to hide private info...]
(lldb) v8 inspect 0x2ad297b2f569
0x2ad297b2f569:<Object: TypeError properties {
    .stack=0x2ad297b2f981:<Array: length=45>,
    .<non-string>=0x2ad297b2f981:<Array: length=45>,
    .message=0x2ad297b2f541:<String: "Cannot match aga...">,
    .<non-string>=0x2725531043c1:<true>}>
(lldb) v8 inspect 0x2ad297b2f981
0x2ad297b2f981:<Array: length=45 {
[...redacted to hide private info...]
}>
(lldb) v8 inspect -l 44 0x2ad297b2f981
Segmentation fault: 11
$

Am I missing something?

EDIT: I'm running llnode from latest master:

$ git rev-parse --short HEAD
bb0d400
$ 

@mmarchini
Copy link
Contributor Author

Seems like a separate issue. v8 inspect -l 44 0x2ad297b2f981 is inspecting an Array unaware it might be a stack trace, so it's not - or at least shouldn't be - related to changes on this PR.

This is not the first segfault I see in llnode (#196), we probably have some fragile code paths somewhere. Do you mind opening a new issue for this? Also, it would be nice to get llnode's stack trace right before it segfaulted. I haven't tried it, but it seems to be possible with dtrace: https://stackoverflow.com/a/17115382/2956796.

@misterdjules
Copy link

@mmarchini How are users supposed to inspect the stack trace? v8 inspect address-of-stack-property doesn't seem to output the full stack.

@mmarchini
Copy link
Contributor Author

v8 inspect 0x2ad297b2f569 (the Error object) should give you the stack trace.

Ohhh I see, this is a TypeError. Ok, we'll need to handle this differently in Node.js v6.x.
Will do a follow-up PR for that.

@mmarchini
Copy link
Contributor Author

mmarchini commented Nov 30, 2018

FWIW, it's working as expected on Node.js v10.13.0

(llnode) v8 findjsinstances -d TypeError
0x17febe244e11:<Object: TypeError properties {
    .stack=0x17feba59f0d9:<unknown>,
    .<non-string>=0x17febe244fd1:<Array: length=41>}
  error stack {
    0x17fe6794dd81:<function: (anonymous) at [eval]:1:0>
    0x17febe210d61:<function: runInThisContext at vm.js:91:19>
    0x17febe210c71:<function: runInThisContext at vm.js:299:26>
    0x17febe244791:<function: (anonymous) at [eval]-wrapper:1:10>
    0x17fe27675eb9:<function: Module._compile at (external).js:649:37>
    0x17fe6791fae9:<function: evalScript at internal/bootstrap/node.js:1:10>
    0x17fec0b985a9:<function: startup at internal/bootstrap/node.js:1:10>
    0x17fec0b987e1:<function: bootstrapNodeJSCore at internal/bootstrap/node.js:1:10>
  }>
(Showing 1 to 1 of 1 instances)

(edit: and I'm also getting segfault with v6.x)

@joyeecheung
Copy link
Member

Tips: you can turn on the environment variable LLNODE_DEBUG=true to see some debug output from llnode. Also running log enable lldb api in the lldb console would tell lldb to log all the API calls which may help as well (if you don't want to debug into the plugin, that'll be debugging the debugger)

@mmarchini
Copy link
Contributor Author

if you don't want to debug into the plugin, that'll be debugging the debugger

I just did it 😂

Tip: to attach a debugger to the debugger, run the debugger you'll use to attach as root.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants