[Webkit-unassigned] [Bug 128115] [Win] LLINT is not working.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=128115





--- Comment #23 from Mark Lam <mark.lam at apple.com>  2014-02-06 10:23:25 PST ---
(In reply to comment #22)
> Thanks for looking further into this :) I have updated the patch after your latest review.
> 
> >> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:-212
> >> -        storei t0, 0xbbadbeef[]
> 
> >Why remove this line?
> 
> I removed this line because it generates the instruction "mov 3148725999, eax" on Windows, which doesn't compile (immediate operand not allowed).

Yeah, I remembered seeing your comment in the ChangeLog afterwards, but forgot to update this feedback comment.  The instruction is intended to trigger a crash by writing eax to some invalid address (and 0xbadbeef tends to be a bad address ... which isn’t always true).  Anyway, why don’t we try this instead: implement the crash() macro in terms of a call to a slow path function, and let that slow path function take care of doing CRASH() in the platform appropriate way:

Index: Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
===================================================================
--- Source/JavaScriptCore/llint/LLIntSlowPaths.cpp    (revision 163476)
+++ Source/JavaScriptCore/llint/LLIntSlowPaths.cpp    (working copy)
@@ -1441,6 +1441,11 @@
     Heap::writeBarrier(cell);
 }

+extern "C" SlowPathReturnType llint_crash()
+{
+    CRASH();
+}
+
 } } // namespace JSC::LLInt

 #endif // ENABLE(LLINT)
Index: Source/JavaScriptCore/llint/LLIntSlowPaths.h
===================================================================
--- Source/JavaScriptCore/llint/LLIntSlowPaths.h    (revision 163476)
+++ Source/JavaScriptCore/llint/LLIntSlowPaths.h    (working copy)
@@ -128,6 +128,7 @@
 #if ENABLE(LLINT_C_LOOP)
 extern "C" SlowPathReturnType llint_stack_check_at_vm_entry(VM*, Register*) WTF_INTERNAL;
 #endif
+extern "C" SlowPathReturnType llint_crash() WTF_INTERNAL;

 } } // namespace JSC::LLInt

Index: Source/JavaScriptCore/llint/LowLevelInterpreter.asm
===================================================================
--- Source/JavaScriptCore/llint/LowLevelInterpreter.asm    (revision 163476)
+++ Source/JavaScriptCore/llint/LowLevelInterpreter.asm    (working copy)
@@ -209,9 +209,7 @@
     if C_LOOP
         cloopCrash
     else
-        storei t0, 0xbbadbeef[]
-        move 0, t0
-        call t0
+        call _llint_crash
     end
 end


> I will upload a new patch with autogeneration of the forward declared symbols soon.

Thanks.

> The bots are green now, but I did nothing more than update the patch with your suggestions. Are the bots not running the stress tests?
> 
> I have also tested that the generated GCC code is identical to the code generated without the patch (ignoring the changes in LowLevelInterpreter(32_64).asm).

The EWS bots do run the layout tests.  So, the mac bots being green is a good sign.  For previous patches, the mac bots were showing signs of trouble (e.g. being yellow could mean that it was taking a long time, ... which suggests flakiness or a lot crashes).

I didn’t debug it too much yestserday (especially since I didn’t have your updated patch ... and didn’t want to do duplicate work).  But I’ve just run the jsc tests on your updated patch with Mac x86_64 and it appears to be fine.

However, I’m seeing crashes on a 32-bit x86 run.  Currently running the test on the baseline 32-bit x86 on Mac.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.