Bug 125168

Created attachment 218311 [details] proposed patch.

Comment on attachment 218311 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=218311&action=review > Source/JavaScriptCore/offlineasm/mips.rb:-535 > - if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) > + if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) || /_llint_op_catch/.match(node.name) > # Functions called from trampoline/JIT codes. > myList << Instruction.new(node.codeOrigin, "pichdr", []) > - elsif /_llint_op_catch/.match(node.name) > - # Exception cactcher entry point function. > - myList << Instruction.new(node.codeOrigin, "pichdrra", []) This is atrocious. Are you seriously matching label names?

(In reply to comment #2) > (From update of attachment 218311 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=218311&action=review > > > Source/JavaScriptCore/offlineasm/mips.rb:-535 > > - if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) > > + if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) || /_llint_op_catch/.match(node.name) > > # Functions called from trampoline/JIT codes. > > myList << Instruction.new(node.codeOrigin, "pichdr", []) > > - elsif /_llint_op_catch/.match(node.name) > > - # Exception cactcher entry point function. > > - myList << Instruction.new(node.codeOrigin, "pichdrra", []) > > This is atrocious. Are you seriously matching label names? Yes. On MIPS for PIC compatibility a special header (pichdr) needed. I didn't find a better way to add PIC header only to the "entry point" functions/ops.

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 218311 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=218311&action=review > > > > > Source/JavaScriptCore/offlineasm/mips.rb:-535 > > > - if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) > > > + if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) || /_llint_op_catch/.match(node.name) > > > # Functions called from trampoline/JIT codes. > > > myList << Instruction.new(node.codeOrigin, "pichdr", []) > > > - elsif /_llint_op_catch/.match(node.name) > > > - # Exception cactcher entry point function. > > > - myList << Instruction.new(node.codeOrigin, "pichdrra", []) > > > > This is atrocious. Are you seriously matching label names? > > Yes. On MIPS for PIC compatibility a special header (pichdr) needed. I didn't find a better way to add PIC header only to the "entry point" functions/ops. This is really bad. Please find a better way to do this before making the problem worse.

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 218311 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=218311&action=review > > > > > > > Source/JavaScriptCore/offlineasm/mips.rb:-535 > > > > - if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) > > > > + if /_prologue$/.match(node.name) || /^_llint_function_/.match(node.name) || /_llint_op_catch/.match(node.name) > > > > # Functions called from trampoline/JIT codes. > > > > myList << Instruction.new(node.codeOrigin, "pichdr", []) > > > > - elsif /_llint_op_catch/.match(node.name) > > > > - # Exception cactcher entry point function. > > > > - myList << Instruction.new(node.codeOrigin, "pichdrra", []) > > > > > > This is atrocious. Are you seriously matching label names? > > > > Yes. On MIPS for PIC compatibility a special header (pichdr) needed. I didn't find a better way to add PIC header only to the "entry point" functions/ops. > > This is really bad. Please find a better way to do this before making the problem worse. Thank you for the review. I dislike this solution also but PIC header is 3 instructions long so I didn't want to add it to all the LLInt ops when it is not necessary. I think there is no other sign of entry point code than the node.name from offlineasm/mips.rb. I could add PIC header to all the ops (like gcc does for every function). I will check the speed penalty of that solution.

Created attachment 218585 [details] Fixed patch. Added PIC header (.cpload($t9)) to all the LLInt ops. With this we can avoid filtering rutins by label name. The speed penalty is not serious by the SunSpider and v8 tests (although they aren't ideal for LLInt measuring, I think). SunSpider: 5272.0ms -> 5295.0ms v8: 18751.0ms -> 18893.0ms With this patch the MIPS implementation has 0 regression in mozilla tests.

Patch looks good to me: it solves all remaining crashes in layout jsc tests on my mips board. Moreover, it removes the following warnings when compiling the LLINT: [100%] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o /tmp/cc9i3yBA.s: Assembler messages: /tmp/cc9i3yBA.s:270: Warning: No .cprestore pseudo-op used in PIC code /tmp/cc9i3yBA.s:300: Warning: No .cprestore pseudo-op used in PIC code /tmp/cc9i3yBA.s:332: Warning: No .cprestore pseudo-op used in PIC code /tmp/cc9i3yBA.s:375: Warning: No .cprestore pseudo-op used in PIC code [...] Although there's a small speed penalty, I think your 2nd patch is better (and more clean).

(In reply to comment #7) > Patch looks good to me: it solves all remaining crashes in layout jsc tests on my mips board. > > Moreover, it removes the following warnings when compiling the LLINT: > > [100%] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/llint/LowLevelInterpreter.cpp.o > /tmp/cc9i3yBA.s: Assembler messages: > /tmp/cc9i3yBA.s:270: Warning: No .cprestore pseudo-op used in PIC code > /tmp/cc9i3yBA.s:300: Warning: No .cprestore pseudo-op used in PIC code > /tmp/cc9i3yBA.s:332: Warning: No .cprestore pseudo-op used in PIC code > /tmp/cc9i3yBA.s:375: Warning: No .cprestore pseudo-op used in PIC code > [...] > > > Although there's a small speed penalty, I think your 2nd patch is better (and more clean). Thanks for reviewing and testing. The disappearing of the "No .cprestore" warning is a nice side effect as .cprestore is still not used in the LLInt ops. :) But anyway now the generated code seems to be more comfortable to the compiler.

r160244 introduced a conditional branch from an LLInt op to an other. This breaks the "PIC header everywhere" patch. In mozilla test causes a regression: ecma_2/FunctionObjects/apply-001-n.js I am working on fixing this problem.

Created attachment 218975 [details] Fixed and rebased patch. Branch to other LLInt operations (to Labels, not to LocalLabels) fixed also. Rebased on r160432.

Comment on attachment 218975 [details] Fixed and rebased patch. I don't really know this platform. Since all the code is isolated to it, I don't think it can harm the rest of the project, so I'll say r+.

(In reply to comment #11) > I don't really know this platform. Since all the code is isolated to it, I don't think it can harm the rest of the project, so I'll say r+. Thanks. As discussed, I'll probably take a look at the mips port during next days, and this patch will help me to start from a clean state.

Comment on attachment 218975 [details] Fixed and rebased patch. Clearing flags on attachment: 218975 Committed r173232: <http://trac.webkit.org/changeset/173232>

All reviewed patches have been landed. Closing bug.