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

inspect objects with "Symbol" attributes #156

Closed
mmarchini opened this issue Dec 8, 2017 · 3 comments
Closed

inspect objects with "Symbol" attributes #156

mmarchini opened this issue Dec 8, 2017 · 3 comments

Comments

@mmarchini
Copy link
Contributor

Inspect commands can't handle Symbol attributes (e.g. @@toStringTag and @@unscopable), resulting in <non-string> properties.

For example, the following code uses the @@toStringTag symbol:

class Bar {
  constructor() {
    this[Symbol.toStringTag] = "Bar";
  }
}

let bar = new Bar();

If we use llnode to find out the properties for bar, it will give the following output:

(lldb) v8 findjsinstances -v Bar
0x0000015078184729:<Object: Bar properties {
    .constructor=0x00000150781846e1:<function: Bar at index.js:17:14>}>
0x0000015078184949:<Object: Bar properties {
    .<non-string>=0x00002d2064acb1e1:<String: "Bar">}>

As you can see, there's a <non-string> property instead of our symbol.

I was looking at llnode and V8 code to try to find a solution, but I'm not sure how to implement it. Did anyone try this before?

@bnoordhuis
Copy link
Member

Can you try this patch with node.js master? That should add the metadata for symbols although llnode still needs to be taught to use it.

diff --git a/deps/v8/src/v8.gyp b/deps/v8/src/v8.gyp
index 89eb271f61..38670c193d 100644
--- a/deps/v8/src/v8.gyp
+++ b/deps/v8/src/v8.gyp
@@ -2452,6 +2452,8 @@
             'objects-inl.h',
             'objects/map.h',
             'objects/map-inl.h',
+            'objects/name.h',
+            'objects/name-inl.h',
             'objects/script.h',
             'objects/script-inl.h',
             'objects/shared-function-info.h',
diff --git a/deps/v8/tools/gen-postmortem-metadata.py b/deps/v8/tools/gen-postmortem-metadata.py
index 22f0afbef3..5f33fdebe9 100644
--- a/deps/v8/tools/gen-postmortem-metadata.py
+++ b/deps/v8/tools/gen-postmortem-metadata.py
@@ -417,15 +417,10 @@ def load_objects_from_file(objfilename, checktypes):
         # way around.
         #
         for type in types:
-                #
-                # Symbols and Strings are implemented using the same classes.
-                #
-                usetype = re.sub('SYMBOL_', 'STRING_', type);
-
                 #
                 # REGEXP behaves like REG_EXP, as in JS_REGEXP_TYPE => JSRegExp.
                 #
-                usetype = re.sub('_REGEXP_', '_REG_EXP_', usetype);
+                usetype = re.sub('_REGEXP_', '_REG_EXP_', type);
 
                 #
                 # Remove the "_TYPE" suffix and then convert to camel case,

That gives you these:

$ nm out/Release/node | grep -i v8dbg.*symbol
0000000101596d58 D _v8dbg_class_Symbol__flags__SMI
0000000101596d54 D _v8dbg_class_Symbol__name__Object
000000010159ece4 S _v8dbg_parent_Symbol__Name
0000000101596a50 D _v8dbg_type_Symbol__SYMBOL_TYPE

@mmarchini
Copy link
Contributor Author

@bnoordhuis yes, this patch is enough on the V8 side. Will you submit a CL with these changes?

@mmarchini
Copy link
Contributor Author

@bnoordhuis would you mind if I open a CL with these changes, or do you want to do it?

hubot pushed a commit to v8/v8 that referenced this issue Sep 5, 2018
As discussed in nodejs/llnode#156, we need
postmortem metadata for Symbols to properly print Symbol property names
in postmortem debugging tools. Patch suggested by Ben Noordhuis
(nodejs/llnode#156 (comment)).

R=bmeurer@google.com, yangguo@google.com

Change-Id: Ied6d3c079e8b23a9c796bc632c37785ed7dbc118
Reviewed-on: https://chromium-review.googlesource.com/1205052
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55632}
mmarchini added a commit to mmarchini/node that referenced this issue Sep 24, 2018
Original commit message:

    [postmortem] add postmortem metadata for symbols

    As discussed in nodejs/llnode#156, we need
    postmortem metadata for Symbols to properly print Symbol property names
    in postmortem debugging tools. Patch suggested by Ben Noordhuis
    (nodejs/llnode#156 (comment)).

    R=bmeurer@google.com, yangguo@google.com

    Change-Id: Ied6d3c079e8b23a9c796bc632c37785ed7dbc118
    Reviewed-on: https://chromium-review.googlesource.com/1205052
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#55632}

Refs: v8/v8@958b761
mmarchini added a commit to nodejs/node that referenced this issue Oct 6, 2018
Original commit message:

    [postmortem] add postmortem metadata for symbols

    As discussed in nodejs/llnode#156, we need
    postmortem metadata for Symbols to properly print Symbol property
    names in postmortem debugging tools. Patch suggested by Ben
    Noordhuis
   (nodejs/llnode#156 (comment)).

    R=bmeurer@google.com, yangguo@google.com

    Change-Id: Ied6d3c079e8b23a9c796bc632c37785ed7dbc118
    Reviewed-on: https://chromium-review.googlesource.com/1205052
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#55632}

Refs: v8/v8@958b761

PR-URL: #22914
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Oct 6, 2018
Original commit message:

    [postmortem] add postmortem metadata for symbols

    As discussed in nodejs/llnode#156, we need
    postmortem metadata for Symbols to properly print Symbol property
    names in postmortem debugging tools. Patch suggested by Ben
    Noordhuis
   (nodejs/llnode#156 (comment)).

    R=bmeurer@google.com, yangguo@google.com

    Change-Id: Ied6d3c079e8b23a9c796bc632c37785ed7dbc118
    Reviewed-on: https://chromium-review.googlesource.com/1205052
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#55632}

Refs: v8/v8@958b761

PR-URL: #22914
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to nodejs/node that referenced this issue Oct 7, 2018
Original commit message:

    [postmortem] add postmortem metadata for symbols

    As discussed in nodejs/llnode#156, we need
    postmortem metadata for Symbols to properly print Symbol property
    names in postmortem debugging tools. Patch suggested by Ben
    Noordhuis
   (nodejs/llnode#156 (comment)).

    R=bmeurer@google.com, yangguo@google.com

    Change-Id: Ied6d3c079e8b23a9c796bc632c37785ed7dbc118
    Reviewed-on: https://chromium-review.googlesource.com/1205052
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#55632}

Refs: v8/v8@958b761

PR-URL: #22914
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit to nodejs/node that referenced this issue Oct 17, 2018
Original commit message:

    [postmortem] add postmortem metadata for symbols

    As discussed in nodejs/llnode#156, we need
    postmortem metadata for Symbols to properly print Symbol property
    names in postmortem debugging tools. Patch suggested by Ben
    Noordhuis
   (nodejs/llnode#156 (comment)).

    R=bmeurer@google.com, yangguo@google.com

    Change-Id: Ied6d3c079e8b23a9c796bc632c37785ed7dbc118
    Reviewed-on: https://chromium-review.googlesource.com/1205052
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#55632}

Refs: v8/v8@958b761

PR-URL: #22914
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
mmarchini pushed a commit to mmarchini/llnode that referenced this issue Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants