Head's up: Frame marking changes in V8 ToT

[ofrobots]

Please see this V8 commit that changes how frames are marked: v8/v8@9dcd085#diff-a6fe1dc6ffc293ad02ba0491647dfef5

More concretely, off_fp_marker is no longer available as a constant in the post-mortem data. If your post-mortem tools are dependent on this constant, you might need to look at the change above.

I've submitted this CL to fix the post-mortem data generation as a result of the above change.

/cc @jgi, @indutny.

[Is there a better way to provide such head's up other than opening like issues like this here?]

[indutny]

Of course they depend on this. :) Will take a deeper look tomorrow.

[ofrobots]

/cc @misterdjules. Apologies for the spam @jgi.

[misterdjules]

@ofrobots Thank you for the heads up and the pointers to the relevant info, very much appreciated!

[Is there a better way to provide such head's up other than opening like issues like this here?]

I think opening an issue here is great. /ccing @nodejs/post-mortem instead of specific individuals may help to reach out to more people involved in building post-mortem debugging tools. I just added @indutny to that team.

[indutny]

It looks like it could be fixed by just using kContextOffset in place of kMarkerOffset. Fixed in llnode: indutny/llnode@573744e

[indutny]

Thank you for the heads up, @ofrobots !

Btw, I wonder if v8 may be interested in using llnode tests to make sure that postmortem metadata is up-to-date and working? Right now tests depend on node, but I can rewrite them to work with d8 too. Please let me know if you are interested ;)

[indutny]

Looks like I was wrong and it is not working properly... Going to take a deeper look.

[indutny]

Hm... nvm, it was unrelated issue.

[davepacheco]

Has this been pulled into any version of Node yet (even master)? I will be interested to see if the DTrace ustack helper is affected by this change.

[ofrobots]

@davepacheco Not yet. This is going to be in V8 5.1. master is at V8 4.9 at the moment.

If you want to play with this, I have an experimental branch with a recent V8 lkgr + Node.js master here: https://github.com/ofrobots/node/tree/vee-eight-lkgr

[indutny]

@davepacheco I believe it may be broken, ustack helper should use context_offset for marker_offset for that new V8 version, as I did in indutny/llnode@573744e

[davepacheco]

I can confirm that using the branch at https://github.com/ofrobots/node/tree/vee-eight-lkgr, the V8 ustack helper doesn't build because of the missing frame metadata. After fixing that as @indutny suggests, it doesn't build because the SharedFunctionInfo inferred_name field is missing. Clearly some additional work will need to be done here for both the ustack helper and mdb_v8.

[davepacheco]

These changes worked for the ustack helper for me:

$ git diff
diff --git a/src/v8abbr.h b/src/v8abbr.h
index 95a04b4..655aae4 100644
--- a/src/v8abbr.h
+++ b/src/v8abbr.h
@@ -86,8 +86,8 @@
     V8_OFF_HEAP(V8DBG_CLASS_JSFUNCTION__SHARED__SHAREDFUNCTIONINFO)
 #define V8_OFF_SHARED_NAME  \
     V8_OFF_HEAP(V8DBG_CLASS_SHAREDFUNCTIONINFO__NAME__OBJECT)
-#define V8_OFF_SHARED_INFERRED  \
-    V8_OFF_HEAP(V8DBG_CLASS_SHAREDFUNCTIONINFO__INFERRED_NAME__STRING)
+#define V8_OFF_SHARED_IDENT  \
+    V8_OFF_HEAP(V8DBG_CLASS_SHAREDFUNCTIONINFO__FUNCTION_IDENTIFIER__OBJECT)
 #define V8_OFF_SHARED_SCRIPT  \
     V8_OFF_HEAP(V8DBG_CLASS_SHAREDFUNCTIONINFO__SCRIPT__OBJECT)
 #define V8_OFF_SHARED_FUNTOK  \
diff --git a/src/v8ustack.d b/src/v8ustack.d
index 08d06e2..8faaf46 100644
--- a/src/v8ustack.d
+++ b/src/v8ustack.d
@@ -414,7 +414,7 @@ dtrace:helper:ustack:
 dtrace:helper:ustack:
 /!this->done/
 {
-   this->marker = COPYIN_PTR(this->fp + V8_OFF_FP_MARKER);
+ this->marker = COPYIN_PTR(this->fp + V8_OFF_FP_CONTEXT);
 }

 dtrace:helper:ustack:
@@ -532,7 +532,7 @@ dtrace:helper:ustack:
        APPEND_CHR('s');
        APPEND_CHR(' ');

-   this->funcnamestr = COPYIN_PTR(this->shared + V8_OFF_SHARED_INFERRED);
+ this->funcnamestr = COPYIN_PTR(this->shared + V8_OFF_SHARED_IDENT);
        LOAD_STRFIELDS(this->funcnamestr, this->funcnamelen,
            this->funcnameattrs);
 }

@indutny, do you think "context" is the right name for the property that we're using, or does it just happen to be the offset with the correct value? i.e., Should we update the postmortem metadata in V8 instead of changing which one we use?

[indutny]

@davepacheco this field actually hold both marker and context now (from what I read in that v8 commit). Marker is SMI, and Context is a heap object, so it is easy to figure out which is which.