Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(441)

Issue 8803024: Optionally export metadata with libv8 to enable debuggers to inspect V8 state. (Closed)

Created:
9 years ago by dave.pacheco
Modified:
8 years, 10 months ago
CC:
v8-dev
Visibility:
Public.

Description

Optionally export metadata with libv8 to enable debuggers to inspect V8 state. Patch from David Pacheco <dap@joyent.com>;. Committed: https://code.google.com/p/v8/source/detail?r=10596

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 7

Patch Set 3 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+516 lines, -0 lines) Patch
M build/common.gypi View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
A tools/gen-postmortem-metadata.py View 1 2 1 chunk +478 lines, -0 lines 0 comments Download
M tools/gyp/v8.gyp View 1 2 2 chunks +33 lines, -0 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
Vyacheslav Egorov (Chromium)
I think it's almost LGTM http://codereview.chromium.org/8803024/diff/1001/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/8803024/diff/1001/build/common.gypi#newcode84 build/common.gypi:84: 'v8_postmortem_support': 'false', should not ...
8 years, 11 months ago (2012-01-17 18:14:58 UTC) #1
dave.pacheco
8 years, 11 months ago (2012-01-18 00:05:36 UTC) #2
Thanks for the review.  My responses are inline below.

On 2012/01/17 18:14:58, Vyacheslav Egorov wrote:
> I think it's almost LGTM
> 
> http://codereview.chromium.org/8803024/diff/1001/build/common.gypi
> File build/common.gypi (right):
> 
> http://codereview.chromium.org/8803024/diff/1001/build/common.gypi#newcode84
> build/common.gypi:84: 'v8_postmortem_support': 'false',
> should not it use % at the end of the name?
> 
>
http://code.google.com/p/gyp/wiki/InputFormatReference#Providing_Default_Valu...)
> 


Yes, that looks good.


> a small comment about the nature of the flag above it would be also helpful.


Okay.


>
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadat...
> File tools/gen-postmortem-metadata.py (right):
> 
>
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadat...
> tools/gen-postmortem-metadata.py:4: # Copyright 2006-2008 the V8 project
> authors. All rights reserved.
> 2012


Thanks.


>
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadat...
> tools/gen-postmortem-metadata.py:56: { 'name': 'FirstNonstringType',    
> 'value': 'FIRST_NONSTRING_TYPE' },
> I seems you can just list what you want to export and come up with some
> meaningful mangling scheme to avoid duplication and  stuff. 
> 
> (just thinking loud)


I did consider this, but I thought the added clarity from listing them that way
was worth the risk of a copy/paste error. Besides that, several of the fields
are exported with names that don't exactly match their names in the source,
which is important, as the corresponding names in the source may change.

 
>
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadat...
> tools/gen-postmortem-metadata.py:208: #
> Did you consider using TYPE_CHECKER macro as a feedback for this loop?


No, I didn't notice TYPE_CHECKER.  It looks promising, but a quick check shows
that it's missing some types that we really need (like Script), so it won't work
by itself.



>
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadat...
> tools/gen-postmortem-metadata.py:244: # Mapping string types is more
> complicated.  Both types and
> I think it might be fine to hard wire some instance type names to class names
> instead of trying to guess them in such a convoluted way.
> 
> (just a thought)


Personally, I'd rather have the list automatically generated, even though it's
somewhat complex.  Although the algorithm is convoluted, it matches the comments
describing type naming in src/objects.h.


>
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadat...
> tools/gen-postmortem-metadata.py:247: # In the simplest case, both of these
are
> explicit in both names,
> long line.


Thanks for catching that.


>
http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadat...
> tools/gen-postmortem-metadata.py:291: typeclasses[type] = cctype;
> Maybe tools should have a whitelist and complain to stdout when it fails to
map
> class into instance type?
> 
> This way we can detect if tool will ever go out of sync with V8 source base.


If the V8 team would monitor such output periodically to make sure it's correct,
that might be a good idea. The script could exit failure if it doesn't find at
least some bare minimum classes. (It won't be exhaustive, though, as we continue
to update the debugger to make use of additional classes.) I had the impression
that the team wasn't going to build with the GYP flag enabled, so it would never
notice such breakage.

What's the next step? I will make several of the above suggested changes and
also see about rebasing onto the latest V8, as there's likely been a lot of
change since I submitted this review. Do I submit another review, or is there a
way to update this one?

Thanks again for the review.

-- Dave

Powered by Google App Engine
This is Rietveld 408576698