Bug 90095 – JSC: add infrastructure for appending comments to generated bytecode

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Mark Lam]

Created attachment 149839 [details]
Fix.

[WebKit Review Bot]

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.

[Mark Lam]

Created attachment 149842 [details]
Rev 1: removed a tab that snuck in.

[Geoffrey Garen]

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.

[Mark Lam]

Created attachment 149986 [details]
Rev 2: applied changes based on Geoff's feedback.

[Filip Pizlo]

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.

[Geoffrey Garen]

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.

[Mark Lam]

Created attachment 149992 [details]
rev3: Copyright header updated for the new file.

[Geoffrey Garen]

Comment on attachment 149992 [details]
rev3: Copyright header updated for the new file.

r=me

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.