Bug 88315 – [GTK] Enable the LLInt

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Andy Wingo]

The patch to be attached will enable the LLInt for the GTK+ port.

[Andy Wingo]

Created attachment 145735 [details]
Patch

[Andy Wingo]

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!

[Andy Wingo]

Created attachment 145739 [details]
Patch

[Andy Wingo]

Created attachment 145742 [details]
again with the Makefile foo

[Andy Wingo]

Created attachment 145745 [details]
fourth time's the charm

[Andy Wingo]

Created attachment 145749 [details]
fifth time's the... (more makefile foo)

[Filip Pizlo]

Comment on attachment 145749 [details]
fifth time's the... (more makefile foo)

Looks right!

[Filip Pizlo]

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.

[Andy Wingo]

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.

[Andy Wingo]

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.

[Andy Wingo]

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>

[Andy Wingo]

All reviewed patches have been landed.  Closing bug.