Hi Dave, > If the V8 team would monitor such output periodically to make sure it's correct, > that might be a good idea.
I am considering to switch some of our postmortem debugging scripts (e.g. grokdump) to use information extracted by your script so we would definitely monitor errors. > 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? Both gcl and git-cl understand uploading another patch set to the same issue. Just say gcl upload <patch-set> / git cl upload again. -- Vyacheslav Egorov On Wed, Jan 18, 2012 at 1:05 AM, <[email protected]> wrote: > Reviewers: Vyacheslav Egorov, > > Message: > 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<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<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_Values_for_**Variables_%28%25<http://code.google.com/p/gyp/wiki/InputFormatReference#Providing_Default_Values_for_Variables_%28%25> > ) > > > > 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-metadata.py<http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py> > >> File tools/gen-postmortem-metadata.**py (right): >> > > > http://codereview.chromium.**org/8803024/diff/1001/tools/** > gen-postmortem-metadata.py#**newcode4<http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode4> > >> 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-metadata.py#**newcode56<http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode56> > >> 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-metadata.py#**newcode208<http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode208> > >> 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-metadata.py#**newcode244<http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode244> > >> 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-metadata.py#**newcode247<http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode247> > >> 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-metadata.py#**newcode291<http://codereview.chromium.org/8803024/diff/1001/tools/gen-postmortem-metadata.py#newcode291> > >> 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 > > Description: > Optionally export metadata with libv8 to enable debuggers to inspect V8 > state. > > Contributed by [email protected] > > Please review this at > http://codereview.chromium.**org/8803024/<http://codereview.chromium.org/8803024/> > > SVN Base: > http://v8.googlecode.com/svn/**branches/bleeding_edge/<http://v8.googlecode.com/svn/branches/bleeding_edge/> > > Affected files: > M build/common.gypi > A tools/gen-postmortem-metadata.**py > M tools/gyp/v8.gyp > > > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
