Bug 88315 - [GTK] Enable the LLInt
Summary: [GTK] Enable the LLInt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Wingo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-05 01:50 PDT by Andy Wingo
Modified: 2012-06-06 09:01 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.84 KB, patch)
2012-06-05 02:14 PDT, Andy Wingo
no flags Details | Formatted Diff | Diff
Patch (11.95 KB, patch)
2012-06-05 02:48 PDT, Andy Wingo
no flags Details | Formatted Diff | Diff
again with the Makefile foo (11.94 KB, patch)
2012-06-05 03:07 PDT, Andy Wingo
no flags Details | Formatted Diff | Diff
fourth time's the charm (12.08 KB, patch)
2012-06-05 03:28 PDT, Andy Wingo
no flags Details | Formatted Diff | Diff
fifth time's the... (more makefile foo) (12.14 KB, patch)
2012-06-05 03:40 PDT, Andy Wingo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Wingo 2012-06-05 01:50:07 PDT
The patch to be attached will enable the LLInt for the GTK+ port.
Comment 1 Andy Wingo 2012-06-05 02:14:08 PDT
Created attachment 145735 [details]
Patch
Comment 2 Andy Wingo 2012-06-05 02:16:43 PDT
The patch includes changes to Platform.h that I hope don't affect anyone, but need some checking from Apple folks for their ports.

I would also appreciate it if Filip could verify that the changes to LowLevelInterpreter.asm and asm.rb are sane.  Thanks!
Comment 3 Andy Wingo 2012-06-05 02:48:04 PDT
Created attachment 145739 [details]
Patch
Comment 4 Andy Wingo 2012-06-05 03:07:47 PDT
Created attachment 145742 [details]
again with the Makefile foo
Comment 5 Andy Wingo 2012-06-05 03:28:10 PDT
Created attachment 145745 [details]
fourth time's the charm
Comment 6 Andy Wingo 2012-06-05 03:40:52 PDT
Created attachment 145749 [details]
fifth time's the... (more makefile foo)
Comment 7 Filip Pizlo 2012-06-05 21:20:23 PDT
Comment on attachment 145749 [details]
fifth time's the... (more makefile foo)

Looks right!
Comment 8 Filip Pizlo 2012-06-05 21:27:13 PDT
Comment on attachment 145749 [details]
fifth time's the... (more makefile foo)

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

My additional comments should not be viewed as commit blockers.  Just giving you stuff to think about, possibly for future patches.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:106
> -    storei 0, 0xbbadbeef[]
> +    storei t0, 0xbbadbeef[]

Actually, why?

To clarify, this looks like it's fine, but unnecessary.  I'm curious why you needed to do this.  On all of the backends supported by offlineasm, 'storei 0, 0xbbadbeef[]' should be supported.  On x86 it's a single mov instruction.  Same for x86_64.  On ARMv7, it'll create some temporary registers for the immediate 0 and the address 0xbbadbeef.  But that should all work.  If it doesn't, then it's a pretty fundamental bug!

> Source/JavaScriptCore/offlineasm/asm.rb:100
> -        "\" SYMBOL_STRING(#{labelName}) \""
> +        "\" SYMBOL_STRING_RELOCATION(#{labelName}) \""

Looks right, sort of.  This makes me worry that on Linux you'll end up taking indirections whenever LLInt jumps at one of its own labels.  But maybe I don't do that enough for it to matter?

> Source/WTF/wtf/InlineASM.h:79
> +#elif OS(LINUX)
> +#define LOCAL_LABEL_STRING(name) ".L" #name

LGTM.
Comment 9 Andy Wingo 2012-06-06 08:10:53 PDT
Thanks for the review and comments.

(In reply to comment #8)
> > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:106
> > -    storei 0, 0xbbadbeef[]
> > +    storei t0, 0xbbadbeef[]
> 
> Actually, why?

The immediate symptom is that the generated assembly causes a compile error:

The assembly:  (Line 6 is the movl)

OFFLINE_ASM_GLOBAL_LABEL(llint_begin) 
"\tmovl $0, 3148725999\n" // ../../Source/JavaScriptCore/llint/LowLevelInterpreter.asm:106
"\txorq %rax, %rax\n" // ../../Source/JavaScriptCore/llint/LowLevelInterpreter.asm:107
"\tcall *%rax\n" // ../../Source/JavaScriptCore/llint/LowLevelInterpreter.asm:108

The error:

/tmp/cczM4iOQ.s: Assembler messages:
/tmp/cczM4iOQ.s:6: Error: unsupported for `mov'
/tmp/cczM4iOQ.s:4386: Error: unsupported for `mov'

Pretty strange stuff; I've thought over it a while but I don't have any coherent explanation.  I assume you do make x86-64 builds, yes?

> > Source/JavaScriptCore/offlineasm/asm.rb:100
> > -        "\" SYMBOL_STRING(#{labelName}) \""
> > +        "\" SYMBOL_STRING_RELOCATION(#{labelName}) \""
> 
> Looks right, sort of.  This makes me worry that on Linux you'll end up taking indirections whenever LLInt jumps at one of its own labels.  But maybe I don't do that enough for it to matter?

Yeah it's a bit irritating.  I think that although we were compiling with -fvisibility=hidden for other parts of webkit, somehow we dropped that for JSC.  I'm looking into that.
Comment 10 Andy Wingo 2012-06-06 08:40:51 PDT
Replying to myself.

(In reply to comment #9)
> I think that although we were compiling with -fvisibility=hidden for other parts of webkit, somehow we dropped that for JSC.  I'm looking into that.

Yes, the GTK+ port did some bone-headed things there (bug 88421).  It will take some time to extricate, though, so it seems we will have to do the PLT indirection dance for some intermediate amount of time.
Comment 11 Andy Wingo 2012-06-06 09:00:50 PDT
Comment on attachment 145749 [details]
fifth time's the... (more makefile foo)

Clearing flags on attachment: 145749

Committed r119593: <http://trac.webkit.org/changeset/119593>
Comment 12 Andy Wingo 2012-06-06 09:01:01 PDT
All reviewed patches have been landed.  Closing bug.