Bug 91690 – JSC: bug fixes and enhancements for OfflineASM annotation system

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

[Mark Lam]

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.

[Mark Lam]

Created attachment 153132 [details]
Fix.

[Filip Pizlo]

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.

[Mark Lam]

Created attachment 153151 [details]
rev 1: addressed Filip's concern. No longer play tricks with string encoding.

[Mark Lam]

Created attachment 153203 [details]
rev 2: removed an unneeded OOPS comment in the last patch.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.