Bug 88315

Created attachment 145735 [details] Patch

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!

Created attachment 145739 [details] Patch

Created attachment 145742 [details] again with the Makefile foo

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

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

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

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.

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.

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 on attachment 145749 [details] fifth time's the... (more makefile foo) Clearing flags on attachment: 145749 Committed r119593: <http://trac.webkit.org/changeset/119593>

All reviewed patches have been landed. Closing bug.