Optionally export metadata with libv8 to enable debuggers to inspect V8 state.

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

[Vyacheslav Egorov (Chromium), 2012-01-17 18:14:58]

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...

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

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

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)

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?

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)

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.

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.

[dave.pacheco, 2012-01-18 00:05:36]

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