Bug 91690 - JSC: bug fixes and enhancements for OfflineASM annotation system
Summary: JSC: bug fixes and enhancements for OfflineASM annotation system
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-18 16:17 PDT by Mark Lam
Modified: 2012-07-19 13:53 PDT (History)
3 users (show)

See Also:


Attachments
Fix. (18.56 KB, patch)
2012-07-18 16:29 PDT, Mark Lam
fpizlo: review-
fpizlo: commit-queue-
Details | Formatted Diff | Diff
rev 1: addressed Filip's concern. No longer play tricks with string encoding. (19.04 KB, patch)
2012-07-18 18:16 PDT, Mark Lam
mark.lam: review-
mark.lam: commit-queue-
Details | Formatted Diff | Diff
rev 2: removed an unneeded OOPS comment in the last patch. (deleted)
2012-07-19 01:00 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-18 16:17:04 PDT
The previously added OfflineASM annotation infrastructure does not support annotations for macros or at lines which do not have instructions at all.  Under some circumstances, adding such annotations will actually break the build.  This change set fills out the implementation of the annotation infrastructure, and it now allows annotations to be added anywhere.  It also adds more pretty printing to make the annotations more readable in the output comment.
Comment 1 Mark Lam 2012-07-18 16:29:15 PDT
Created attachment 153132 [details]
Fix.
Comment 2 Filip Pizlo 2012-07-18 17:28:14 PDT
Comment on attachment 153132 [details]
Fix.

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

> Source/JavaScriptCore/offlineasm/parser.rb:94
> +            annotationType = whitespaceFound ? "@L" : "@G" # local or global annotation

It is customary to use symbols (:thing) rather than strings ("thing") for enumerating in ruby.

> Source/JavaScriptCore/offlineasm/parser.rb:99
> -                result << Token.new(CodeOrigin.new(fileName, lineNumber), "@" + annotation)
> +                result << Token.new(CodeOrigin.new(fileName, lineNumber),
> +                                    "#{annotationType}#{annotation}")

I don't like the idea of using strings with magic as the contents of a token.  Tokens are supposed to contain the string that was parsed.  We already have a slightly ugly hack for numbers (we convert the string to a number then back to a decimal string) but I don't want that to spread.
Comment 3 Mark Lam 2012-07-18 18:16:09 PDT
Created attachment 153151 [details]
rev 1: addressed Filip's concern. No longer play tricks with string encoding.
Comment 4 Mark Lam 2012-07-19 01:00:56 PDT
Created attachment 153203 [details]
rev 2: removed an unneeded OOPS comment in the last patch.
Comment 5 WebKit Review Bot 2012-07-19 13:53:33 PDT
Comment on attachment 153203 [details]
rev 2: removed an unneeded OOPS comment in the last patch. 

Clearing flags on attachment: 153203

Committed r123147: <http://trac.webkit.org/changeset/123147>
Comment 6 WebKit Review Bot 2012-07-19 13:53:38 PDT
All reviewed patches have been landed.  Closing bug.