Bug 91281 - JSC: OfflineASM Pretty printing and commenting enhancements
Summary: JSC: OfflineASM Pretty printing and commenting enhancements
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-07-13 14:30 PDT by Mark Lam
Modified: 2012-07-13 17:45 PDT (History)
3 users (show)

See Also:


Attachments
Fix. (33.30 KB, patch)
2012-07-13 15:29 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-07-13 14:30:28 PDT
It would be nice to be have the output of the OfflineASM be more prettily formatted for human consumption.  Also, it would be nice to be able to more easily add annotations in the LLInt that will show up in the OfflineASM output for debugging and analysis purposes.
Comment 1 Mark Lam 2012-07-13 15:29:53 PDT
Created attachment 152353 [details]
Fix.
Comment 2 WebKit Review Bot 2012-07-13 15:31:30 PDT
Attachment 152353 [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/llint/LLIntOfflineAsmConfig.h:96:  Extra space before )  [whitespace/parens] [2]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2012-07-13 15:35:30 PDT
The 1 style error is because of I have macros that looks like this:

#define OFFLINE_ASM_BEGIN   asm (
#define OFFLINE_ASM_END     );

The style checker is complaining about the spaces before the ");" because it mistook it for an argument list.  I this case, I think the style violation can be allowed as an exception.
Comment 4 Geoffrey Garen 2012-07-13 17:33:05 PDT
Comment on attachment 152353 [details]
Fix.

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

Looks good to me. I'd like Phil to take a look too, since he's more familiar with the Ruby driver. I've CC'd him.

One comment below.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:305
> +    loadp CodeBlock[cfr], t2                // t2<CodeBlock> = cfr.CodeBlock
> +    loadi CodeBlock::m_numVars[t2], t2      // t2<size_t> = t2<CodeBlock>.m_numVars

It looks like these comments parrot the LL assembly language in a slightly modified form. Perhaps comment dumping mode should just dump the LL assembly. I could see myself using that when I debugged something in the interpreter.
Comment 5 WebKit Review Bot 2012-07-13 17:45:00 PDT
Comment on attachment 152353 [details]
Fix.

Clearing flags on attachment: 152353

Committed r122650: <http://trac.webkit.org/changeset/122650>
Comment 6 WebKit Review Bot 2012-07-13 17:45:05 PDT
All reviewed patches have been landed.  Closing bug.