Bug 90095 - JSC: add infrastructure for appending comments to generated bytecode
Summary: JSC: add infrastructure for appending comments to generated bytecode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-27 12:49 PDT by Mark Lam
Modified: 2012-06-28 16:03 PDT (History)
1 user (show)

See Also:


Attachments
Fix. (66.12 KB, patch)
2012-06-27 18:11 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
Rev 1: removed a tab that snuck in. (66.12 KB, patch)
2012-06-27 18:22 PDT, Mark Lam
ggaren: review-
Details | Formatted Diff | Diff
Rev 2: applied changes based on Geoff's feedback. (66.51 KB, patch)
2012-06-28 12:44 PDT, Mark Lam
ggaren: review-
Details | Formatted Diff | Diff
rev3: Copyright header updated for the new file. (66.30 KB, patch)
2012-06-28 13:16 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2012-06-27 12:49:36 PDT
It would be nice to be able to append comments to bytecode opcodes that are emitted by the BytecodeGenerator.  This aids in providing some context when debugging the correctness of BytecodeGenerator code generation and any other work that involves bytecode inspection.
Comment 1 Mark Lam 2012-06-27 18:11:28 PDT
Created attachment 149839 [details]
Fix.
Comment 2 WebKit Review Bot 2012-06-27 18:19:26 PDT
Attachment 149839 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2012-06-27 18:22:03 PDT
Created attachment 149842 [details]
Rev 1: removed a tab that snuck in.
Comment 4 Geoffrey Garen 2012-06-28 11:58:20 PDT
Comment on attachment 149842 [details]
Rev 1: removed a tab that snuck in.

View in context: https://bugs.webkit.org/attachment.cgi?id=149842&action=review

Design looks fine, but this patch could use some small tweaks.

> Source/JavaScriptCore/ChangeLog:10
> +        development purposes.  It should not be enable for product builds.

Typo: "enable" should be "enabled".

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:99
> +#ifdef USE_BYTECODE_COMMENTS

The WebKit style for this kind of #ifdef is the "ENABLE()" macro.

To turn the feature on: #define ENABLE_BYTECODE_COMMENTS 1

To test the feature: #if ENABLE(BYTECODE_COMMENTS)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:691
> +// Record a comment in the CodeBlock's comments list for the current opcode
> +// that is about to be emitted.

Better to put a comment like this in the header, by the function declaration.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:702
> +// Register a comment to be associated with the next opcode that will be emitted.

Ditto.
Comment 5 Mark Lam 2012-06-28 12:44:22 PDT
Created attachment 149986 [details]
Rev 2: applied changes based on Geoff's feedback.
Comment 6 Filip Pizlo 2012-06-28 12:50:35 PDT
Comment on attachment 149842 [details]
Rev 1: removed a tab that snuck in.

View in context: https://bugs.webkit.org/attachment.cgi?id=149842&action=review

> Source/JavaScriptCore/bytecode/Comment.h:13
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of

We use the 2-clause license these days.
Comment 7 Geoffrey Garen 2012-06-28 12:56:13 PDT
Comment on attachment 149986 [details]
Rev 2: applied changes based on Geoff's feedback.

Phil's right: please use the license @ http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE.
Comment 8 Mark Lam 2012-06-28 13:16:38 PDT
Created attachment 149992 [details]
rev3: Copyright header updated for the new file.
Comment 9 Geoffrey Garen 2012-06-28 14:22:47 PDT
Comment on attachment 149992 [details]
rev3: Copyright header updated for the new file.

r=me
Comment 10 WebKit Review Bot 2012-06-28 16:03:28 PDT
Comment on attachment 149992 [details]
rev3: Copyright header updated for the new file.

Clearing flags on attachment: 149992

Committed r121480: <http://trac.webkit.org/changeset/121480>
Comment 11 WebKit Review Bot 2012-06-28 16:03:33 PDT
All reviewed patches have been landed.  Closing bug.