Bug 128115 – [Win] LLINT is not working.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

On Windows, LLINT is not working.
It would be nice to have this up and running, as it now seems to be a requirement for JIT.

Created attachment 223007 [details]
Patch

[Brent Fulgham]

Unfortunately, this doesn't seem to work on our build system:

         ml.exe /c /nologo /Zi /Fo"C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\JavaScriptCore\LowLevelInterpreterWin.obj" /W3 /errorReport:prompt  /TaC:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\JavaScriptCore\DerivedSources\LowLevelInterpreterWin.asm
     1>InterpreterWin.asm(11): error A2071: initializer magnitude too large for specified size
     1>: error A2008 : syntax error : .
     1>InterpreterWin.asm(12): error A2071: initializer magnitude too large for specified size
     1>: error A2008 : syntax error : .
     1>InterpreterWin.asm(13): error A2071: initializer magnitude too large for specified size
     1>: error A2008 : syntax error : .
     1>InterpreterWin.asm(14): error A2071: initializer magnitude too large for specified size
     1>: error A2008 : syntax error : CAST
     1>InterpreterWin.asm(15): error A2071: initializer magnitude too large for specified size
     1>: error A2008 : syntax error : CAST
     1>_________________________________________________________ 
COMPILING jsc...                                          

How can we help you move forward? I'd love to see this get done sooner than the JSC team has scheduled it!  :-)

(In reply to comment #2)
> Unfortunately, this doesn't seem to work on our build system:
> 
>          ml.exe /c /nologo /Zi /Fo"C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\JavaScriptCore\LowLevelInterpreterWin.obj" /W3 /errorReport:prompt  /TaC:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\JavaScriptCore\DerivedSources\LowLevelInterpreterWin.asm
>      1>InterpreterWin.asm(11): error A2071: initializer magnitude too large for specified size
>      1>: error A2008 : syntax error : .
>      1>InterpreterWin.asm(12): error A2071: initializer magnitude too large for specified size
>      1>: error A2008 : syntax error : .
>      1>InterpreterWin.asm(13): error A2071: initializer magnitude too large for specified size
>      1>: error A2008 : syntax error : .
>      1>InterpreterWin.asm(14): error A2071: initializer magnitude too large for specified size
>      1>: error A2008 : syntax error : CAST
>      1>InterpreterWin.asm(15): error A2071: initializer magnitude too large for specified size
>      1>: error A2008 : syntax error : CAST
>      1>_________________________________________________________ 
> COMPILING jsc...                                          
> 
> How can we help you move forward? I'd love to see this get done sooner than the JSC team has scheduled it!  :-)

The problem seem to be that LLINT is not enabled in the patch.
Should I enable it in the patch, for testing purposes?

[Brent Fulgham]

I can apply it locally, but it would probably be best to upload a patch that activates LLINT so we can confirm the software builds. You might not get a review if it's red.

We can decide to not activate it before landing if we need to.

Created attachment 223015 [details]
Patch

(In reply to comment #4)
> I can apply it locally, but it would probably be best to upload a patch that activates LLINT so we can confirm the software builds. You might not get a review if it's red.
> 
> We can decide to not activate it before landing if we need to.

OK, enabled LLINT and JIT for 32-bit in latest patch.

[Brent Fulgham]

(In reply to comment #6)
> (In reply to comment #4)
> > I can apply it locally, but it would probably be best to upload a patch that activates LLINT so we can confirm the software builds. You might not get a review if it's red.
> > 
> > We can decide to not activate it before landing if we need to.
> 
> OK, enabled LLINT and JIT for 32-bit in latest patch.

Thanks!

Created attachment 223031 [details]
Patch

[Mark Lam]

Comment on attachment 223031 [details]
Patch

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

Good work.  I tried applying your 2nd patch and doing a Windows build, but the build failed in the generation of LowLevelInterpreterWin.asm.  Can you please address the feedback, rebase to ToT (if you haven't already), and retest to make sure that the patch does build for Windows?  Also make sure the EWS bots are green.  I'll take another look after that.  Thanks.

> Source/JavaScriptCore/ChangeLog:11
> +        The aim of this patch is to get LLINT it up and running on Windows.

typo: remove "it".

> Source/JavaScriptCore/offlineasm/asm.rb:188
> +        putsProcEndIfNeeded

Since this is only needed for isMSVC, please change this line to "putsProcEndIfNeeded if isMSVC".  The notion of ProcEnd has no meaning for non-MSVC code anyway, and making it explicit better mirrors the isMSVC condition call to putsProc() below.

> Source/JavaScriptCore/offlineasm/asm.rb:211
> +        putsProcEndIfNeeded

Is this really needed?  I would think that local labels should not exists out of the bounds of a PROC and ENDP.  If this indeed needed, can you provide details of an example where it's needed and why?

> Source/JavaScriptCore/offlineasm/x86.rb:801
> +                if !isIntelSyntax
> +                    $asm.puts "fucomi #{operands[1].x87Operand(0)}"
> +                else
> +                    $asm.puts "fucomi st(0), #{operands[1].x87Operand(0)}"
> +                end

This pattern seems to be repeated in many places.  Why don't you express this as:

    $asm.puts "fucomi #{@@floatingPointCompareImplicitOperand} #{operands[1].x87Operand(0)}"

where floatingPointCompareImplicitOperand is:
    @@floatingPointCompareImplicitOperand = isIntelSyntax ? "st(0), " : ""

… or something along those lines?

> Source/JavaScriptCore/offlineasm/x86.rb:808
> +                if !isIntelSyntax
> +                    $asm.puts "fucomip #{operands[1].x87Operand(1)}"
> +                else
> +                    $asm.puts "fucomip st(0), #{operands[1].x87Operand(1)}"
> +                end

ditto.

> Source/JavaScriptCore/offlineasm/x86.rb:816
> +                if !isIntelSyntax
> +                    $asm.puts "fucomi #{operands[0].x87Operand(0)}"
> +                else
> +                    $asm.puts "fucomi st(0), #{operands[0].x87Operand(0)}"
> +                end

ditto.

> Source/JavaScriptCore/offlineasm/x86.rb:823
> +                if !isIntelSyntax
> +                    $asm.puts "fucomip #{operands[0].x87Operand(1)}"
> +                else
> +                    $asm.puts "fucomip st(0), #{operands[0].x87Operand(1)}"
> +                end

ditto.

Created attachment 223116 [details]
Patch

(In reply to comment #9)
> (From update of attachment 223031 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223031&action=review
> 
> Good work.  I tried applying your 2nd patch and doing a Windows build, but the build failed in the generation of LowLevelInterpreterWin.asm.  Can you please address the feedback, rebase to ToT (if you haven't already), and retest to make sure that the patch does build for Windows?  Also make sure the EWS bots are green.  I'll take another look after that.  Thanks.

Thanks for looking into this :) I updated the patch according to your comments.

> 
> > Source/JavaScriptCore/offlineasm/asm.rb:211
> > +        putsProcEndIfNeeded
> 
> Is this really needed?  I would think that local labels should not exists out of the bounds of a PROC and ENDP.  If this indeed needed, can you provide details of an example where it's needed and why?

Good you picked up on this. I removed this call, and then had to make a local label global (opPutByIdSlow), in order for others to see it, since it then was visible only inside the PROC.

[Build Bot]

Comment on attachment 223116 [details]
Patch

Attachment 223116 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5070620337373184

New failing tests:
compositing/checkerboard.html
compositing/absolute-inside-out-of-view-fixed.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
animations/added-while-suspended.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
accessibility/alt-tag-on-image-with-nonimage-role.html
accessibility/accessibility-node-reparent.html
compositing/clip-change.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
animations/animation-controller-drt-api.html
http/tests/cache/display-image-unset-allows-cached-image-load.html
animations/3d/change-transform-in-end-event.html
http/tests/cache/content-type-ignored-during-revalidation.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/cancel-during-revalidation-succeeded.html
canvas/philip/tests/2d.canvas.readonly.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
http/tests/cache/cached-main-resource.html
accessibility/adjacent-continuations-cause-assertion-failure.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
compositing/absolute-position-changed-with-composited-parent-layer.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Build Bot]

Created attachment 223129 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 223116 [details]
Patch

Attachment 223116 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5100490358521856

New failing tests:
compositing/checkerboard.html
compositing/absolute-inside-out-of-view-fixed.html
animations/3d/matrix-transform-type-animation.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/added-while-suspended.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
accessibility/alt-tag-on-image-with-nonimage-role.html
accessibility/accessibility-node-reparent.html
compositing/clip-change.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
animations/animation-controller-drt-api.html
animations/3d/change-transform-in-end-event.html
http/tests/cache/content-type-ignored-during-revalidation.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/cancel-during-revalidation-succeeded.html
canvas/philip/tests/2d.canvas.readonly.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.canvas.reference.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
animations/3d/transform-origin-vs-functions.html
accessibility/accessibility-node-memory-management.html
animations/animation-css-rule-types.html
http/tests/cache/cached-main-resource.html
accessibility/adjacent-continuations-cause-assertion-failure.html
compositing/absolute-position-changed-with-composited-parent-layer.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Build Bot]

Created attachment 223132 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

Created attachment 223229 [details]
Patch

[Geoffrey Garen]

Comment on attachment 223229 [details]
Patch

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

> Source/JavaScriptCore/llint/LLIntSymbolsWin.asm:2
> +EXTERN llint_trace_prologue : near

Can we find a way to auto-generate this file? It should be pretty easy: just collect the list of labels during parsing, and then emit them into a file.

[Mark Lam]

Comment on attachment 223229 [details]
Patch

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

Thanks for the update.  Still need some fixes.  My suggestions (nits) and the issues that need fixing are detailed below.

I also like Geoff’s recommendation.  You should be able to enhance asm.rb to generate the symbol asm file if isMSVC.  Either make the symbol filename an extra optional command line arg (at the end) to asm.rb, or synthesize it from the LowLevelInterpreterWin.asm name.  My concern here is to not break other ports.

Also, when you’re done with your new patch, please run the jsc tests to make sure that your patch is working properly:
$ pathToYourOpenSourceDir/Tools/Scripts/run-javascriptcore-test --debug --no-build --no-jsc-stress

For extra credit, also run the layout tests and make sure your patch is not breaking anything new:
$ pathToYourOpenSourceDir/Tools/Scripts/run-webkit-test --debug --no-build

Thanks.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1503
>  macro noAdditionalChecks(oldStructure, scratch)

change to: macro noAdditionalChecks(oldStructure, scratch, slowPath)

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1524
> +
> +    .opPutByIdSlow:
> +        callSlowPath(_llint_slow_path_put_by_id)
> +        dispatch(9)

Remove this blob (it is a replica of the one provided by putByIdTransition().  Instead:
1. Change structureChainChecks() to: macro structureChainChecks(oldStructure, scratch, slowPath)
2. Change putByIdTransition() to call additionalChecks(t1, t3, .opPutByIdSlow)
3. Change structureChainChecks() to use the passed in slowPath arg instead of .opPutByIdSlow.

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

Why remove this line?

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:-699
> -.functionForCallBegin:
> -    functionInitialization(0)
> -    dispatch(0)

This is not functionally equivalent, and hence is a bug.  _llint_function_for_call_prologue expects to fall thru into .functionForCallBegin.  Now it doesn’t.

Fix it by removing the .functionCallBegin label (which isn’t used locally).  I guess will have to replicate this code.  I’ll do some benchmarking after this to make sure that there’s no perf impact here.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:-706
>  _llint_function_for_construct_prologue:
>      prologue(functionForConstructCodeBlockGetter, functionCodeBlockSetter, _llint_entry_osr_function_for_construct, _llint_trace_prologue_function_for_construct)
> -.functionForConstructBegin:
> -    functionInitialization(1)
> -    dispatch(0)

This is not functionally equivalent, and hence is a bug.  _llint_function_for_construct_prologue expects to fall thru into .functionForConstructBegin.  Now it doesn’t.

Fix by removing the .functionForConstructBegin label which isn’t used locally.

> Source/JavaScriptCore/offlineasm/asm.rb:43
> +def commentString
> +    if !isMSVC
> +        "//"
> +    else
> +        ";"
> +    end
> +end

nit: rename commentString to commentPrefix.

def commentPrefix
    isMSVC ? “;” : “//"
end

> Source/JavaScriptCore/offlineasm/asm.rb:64
> -        @outp.puts "OFFLINE_ASM_BEGIN"
> +        if !isMSVC
> +            @outp.puts "OFFLINE_ASM_BEGIN"
> +        end

nit: @outp.puts “OFFLINE_ASM_BEGIN” if !isMSVC

> Source/JavaScriptCore/offlineasm/asm.rb:69
> +        putsProcEndIfNeeded

putsProcEndIfNeeded if isMSVC

> Source/JavaScriptCore/offlineasm/asm.rb:73
> +        if !isMSVC
> +            @outp.puts "OFFLINE_ASM_END"
> +        end

nit: @outp.puts “OFFLINE_ASM_END” if !isMSVC

> Source/JavaScriptCore/offlineasm/asm.rb:101
> -            result = "// " + result
> +            result = commentString + " " + result

nit: rename commentString to commentPrefix.

> Source/JavaScriptCore/offlineasm/asm.rb:173
> +    def putsProc(label, comment)

Add a line after the def here to ensure that this is only called for MSVC:

    raise unless isMSVC

> Source/JavaScriptCore/offlineasm/asm.rb:178
> +    def putsProcEndIfNeeded

Add a line after the def here to ensure that this is only called for MSVC:

    raise unless isMSVC

> Source/JavaScriptCore/offlineasm/asm.rb:256
> +                @outp.puts "    " + commentString + " #{@codeOrigin}"
> +                @outp.puts "    " + commentString + " #{text}"

nit:
@outp.puts "    " + commentPrefix + “ #{@codeOrigin}”
@outp.puts "    " + commentPrefix + “ #{text}"

> Source/JavaScriptCore/offlineasm/asm.rb:289
> +    commentString + " offlineasm input hash: " + parseHash(asmFile) +

nit: commentPrefix + “ offlineasm input hash: " + parseHash(asmFile) +

> Source/JavaScriptCore/offlineasm/x86.rb:70
> +    if isIntelSyntax
> +        name        
> +    else
> +        "%" + name
> +    end

Change to: isIntelSyntax ? name : “%” + name

> Source/JavaScriptCore/offlineasm/x86.rb:78
> +    if isIntelSyntax
> +        "[#{off} + #{register}]"
> +    else
> +        "#{off}(#{register})"
> +    end

Change to: isIntelSyntax ? "[#{off} + #{register}]” : "#{off}(#{register})"

> Source/JavaScriptCore/offlineasm/x86.rb:86
> +    if isIntelSyntax
> +        ""
> +    else
> +        "*"
> +    end

Change to: isIntelSyntax ? “” : “*"

> Source/JavaScriptCore/offlineasm/x86.rb:94
> +    if isIntelSyntax
> +        "#{opB}, #{opA}"
> +    else
> +        "#{opA}, #{opB}"
> +    end

Change to: isIntelSyntax ? "#{opB}, #{opA}” : "#{opA}, #{opB}"

> Source/JavaScriptCore/offlineasm/x86.rb:102
> +    if isIntelSyntax
> +        "#{c}"
> +    else
> +        "$#{c}"
> +    end

Change to: isIntelSyntax ? "#{c}” : "$#{c}"

> Source/JavaScriptCore/offlineasm/x86.rb:794
> +        @@floatingPointCompareImplicitOperand = isIntelSyntax ? "st(0), " : ""

I think you can put this at the top of the Instruction class so that you can use it in the other remaining emission of fucomip below.

> Source/JavaScriptCore/offlineasm/x86.rb:1091
> -                $asm.puts "fildl -4(#{sp.x86Operand(:ptr)})"
> -                $asm.puts "fucomip #{operands[0].x87Operand(1)}"
> +                $asm.puts "fild#{x86Suffix(:int)} #{getSizeString(:int)}#{offsetRegister(-4, sp.x86Operand(:ptr))}"
> +                if !isIntelSyntax
> +                    $asm.puts "fucomip #{operands[0].x87Operand(1)}"
> +                else
> +                    $asm.puts "fucomip st(0), #{operands[0].x87Operand(1)}"
> +                end

Use @@floatingPointCompareImplicitOperand here.

[Mark Lam]

Comment on attachment 223229 [details]
Patch

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

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:-699
>> -    dispatch(0)
> 
> This is not functionally equivalent, and hence is a bug.  _llint_function_for_call_prologue expects to fall thru into .functionForCallBegin.  Now it doesn’t.
> 
> Fix it by removing the .functionCallBegin label (which isn’t used locally).  I guess will have to replicate this code.  I’ll do some benchmarking after this to make sure that there’s no perf impact here.

To clarify: I meant you should remove the .functionForCallBegin label, but leave the other 2 lines in.  Same for _llint_function_for_construct_prologue below.

[Mark Lam]

Comment on attachment 223229 [details]
Patch

I can't run the benchmarks (to confirm that there is no perf regression) because with the patch, we're crashing or getting assertion failures.

The ToT baseline I'm using is r163476.  With this patch (your 5th one @ https://bugs.webkit.org/attachment.cgi?id=223229) without any changes, the jsc tests fails (crashes) in testapi.  With my suggested fix for _llint_function_for_call_prologue and _llint_function_for_construct_prologue, it gets past testapi but fails in the jsc-stress tests (I'm running on a Mac, not Windows).

Running with just the LLINT, I get assertion failures / crashes:
$ JSC_useJIT=false ./Tools/Scripts/run-javascriptcore-test --debug

Running with just the JIT, I get assertion failures / crashes as well:
$ JSC_useLLInt=false ./Tools/Scripts/run-javascriptcore-test --debug
 
Running the baseline (without your patch), I see only 4 timeouts.

Can you please do some debugging, or do another round of code self-review to see what you've changed that is causing these crashes / assertion failures?

Created attachment 223328 [details]
Patch

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).

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

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).

[Mark Lam]

(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.

[Mark Lam]

(In reply to comment #23)
> However, I’m seeing crashes on a 32-bit x86 run.  Currently running the test on the baseline 32-bit x86 on Mac.

The 32-bit crashes appear to be pre-existing in the baseline.  So, it’s not caused by this patch.

Created attachment 223452 [details]
Patch

Created attachment 223455 [details]
Patch

Created attachment 223456 [details]
Patch

Updated patch with auto generation of needed symbols.

[Mark Lam]

Comment on attachment 223456 [details]
Patch

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

Thanks for your work.  I did a benchmark run on Mac x86_64 to make sure that the LLINT changes did not regress performance.  There were no regressions.

r=me with llint_crash updated to not require a return value.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1448
> +extern "C" SlowPathReturnType llint_crash()
> +{
> +    CRASH();
> +    return encodeResult(0, 0);

nit: you can get rid of this return by changing the prototype to: extern "C” NO_RETURN_DUE_TO_CRASH void llint_crash().  Sorry I didn’t catch this yesterday.

> Source/JavaScriptCore/llint/LLIntSlowPaths.h:131
> +extern "C" SlowPathReturnType llint_crash() WTF_INTERNAL;

ditto.  Change to: extern "C” NO_RETURN_DUE_TO_CRASH void llint_crash() WTF_INTERNAL;

Created attachment 223887 [details]
Patch

[Mark Lam]

Comment on attachment 223887 [details]
Patch

r=me

(In reply to comment #31)
> (From update of attachment 223887 [details])
> r=me

Thanks for your help and feedback on this one :)

I fixed the return type of llint_crash.
The latest patch also fixes compilation problems when compiling with the C loop backend.
Now, both the C backend, and the x86 backend should compile cleanly.
LLINT is also not enabled in the latest patch, since there are crashes that needs to be sorted out first.

[Mark Lam]

(In reply to comment #32)
> (In reply to comment #31)
> > (From update of attachment 223887 [details] [details])
> > r=me
> 
> Thanks for your help and feedback on this one :)
> 
> I fixed the return type of llint_crash.
> The latest patch also fixes compilation problems when compiling with the C loop backend.
> Now, both the C backend, and the x86 backend should compile cleanly.
> LLINT is also not enabled in the latest patch, since there are crashes that needs to be sorted out first.

What crashes?  Can you provide details?

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > (From update of attachment 223887 [details] [details] [details])
> > > r=me
> > 
> > Thanks for your help and feedback on this one :)
> > 
> > I fixed the return type of llint_crash.
> > The latest patch also fixes compilation problems when compiling with the C loop backend.
> > Now, both the C backend, and the x86 backend should compile cleanly.
> > LLINT is also not enabled in the latest patch, since there are crashes that needs to be sorted out first.
> 
> What crashes?  Can you provide details?

Yes, there seems to be problems with stack alignment.
I often hit the "int 3" instruction in callToJavaScript, and there are crashes when JIT code calls into C functions, I'm guessing that's stack alignment issues as well. I think this is what we need to figure out next, before we enable it.
Maybe we should try with some __declspec(align(x)) tricks?

[Mark Lam]

Comment on attachment 223887 [details]
Patch

I didn't realize that you've made these additional code changes not in the original review.  I'll need to take a closer look at this again.

Regarding the stack alignment issue, I may be able to make some suggestions if you can post some debugging info on the issue you're seeing e.g. what is the value of the frame pointer and stack pointer when the assertion fails.

Also, to help you debug the issue, I recommend disabling the JIT first and making sure that the assembly LLINT works.  You can do that by setting an environment var JSC_useJIT=false.   Alternatively, you can temporarily change useJIT to false in Options.h.

(In reply to comment #35)
> (From update of attachment 223887 [details])
> I didn't realize that you've made these additional code changes not in the original review.  I'll need to take a closer look at this again.
> 

Ok, thanks, it didn't compile without enabling LLINT, so I had to do some extra modifications.

> Regarding the stack alignment issue, I may be able to make some suggestions if you can post some debugging info on the issue you're seeing e.g. what is the value of the frame pointer and stack pointer when the assertion fails.
> 
> Also, to help you debug the issue, I recommend disabling the JIT first and making sure that the assembly LLINT works.  You can do that by setting an environment var JSC_useJIT=false.   Alternatively, you can temporarily change useJIT to false in Options.h.

Ok, thanks, will try this out, and provide you with info :)

(In reply to comment #35)
> (From update of attachment 223887 [details])
>
> Regarding the stack alignment issue, I may be able to make some suggestions if you can post some debugging info on the issue you're seeing e.g. what is the value of the frame pointer and stack pointer when the assertion fails.
> 

In a WinCairo release build, the first assert I get, is in callToJavaScript:

callToJavaScript PROC PUBLIC
  push ebp
  push esi
  push edi
  push ebx
  sub esp, 12
  mov ebx, dword ptr [36 + esp]
  mov edi, dword ptr [32 + esp]
  mov edx, esp
  and edx, 15
  test edx, edx
  jz _offlineasm_doCallToJavaScript__checkStackPointerAlignment__stackPointerOkay
  mov edx, 3134249985
  int 3 --> Breaks here

When on the "int 3" instruction, esp == 001CE8C8, and ebp == 001CE900

[Michael Saboff]

(In reply to comment #37)
> (In reply to comment #35)
> > (From update of attachment 223887 [details] [details])
> >
> > Regarding the stack alignment issue, I may be able to make some suggestions if you can post some debugging info on the issue you're seeing e.g. what is the value of the frame pointer and stack pointer when the assertion fails.
> > 
> 
> In a WinCairo release build, the first assert I get, is in callToJavaScript:
> 
> callToJavaScript PROC PUBLIC
>   push ebp
>   push esi
>   push edi
>   push ebx
>   sub esp, 12
>   mov ebx, dword ptr [36 + esp]
>   mov edi, dword ptr [32 + esp]
>   mov edx, esp
>   and edx, 15
>   test edx, edx
>   jz _offlineasm_doCallToJavaScript__checkStackPointerAlignment__stackPointerOkay
>   mov edx, 3134249985
>   int 3 --> Breaks here
> 
> When on the "int 3" instruction, esp == 001CE8C8, and ebp == 001CE900

It could be that the compiler doesn't enforce as strict alignment on the stack as we want in JavaScript.  We want a 16 but aligned stack, primarily due to the use of xmm registers for FP operations.  I believe that Windows 32 bit is 8 byte aligned at best.  The combination of the implicit push from the "call" instruction, followed by 4 pushes and then the sub 12 will leave the stack pointer with the same alignment as the caller had at the time of the call.

We had a similar issue with ARMv7 and handled it by aligning the stack in LowLevelInterpreter.asm:callToJavaScriptPrologue  (see the elsif ARM or ARMv7 or ARMv7_TRADITIONAL case).  We effectively pushed the pre aligned stack pointer on the stack post alignment.  We then restore everything back in LowLevelInterpreter.asm:callToJavaScriptEpilogue.  You may have to do the same thing for Windows.

You can verify the need for this by checking the stack pointer value before the first instruction of callToJavaScript (or callToNativeFunction).  The least significant nibble should be 'C'.

(In reply to comment #38)
> It could be that the compiler doesn't enforce as strict alignment on the stack as we want in JavaScript.  We want a 16 but aligned stack, primarily due to the use of xmm registers for FP operations.  I believe that Windows 32 bit is 8 byte aligned at best.  The combination of the implicit push from the "call" instruction, followed by 4 pushes and then the sub 12 will leave the stack pointer with the same alignment as the caller had at the time of the call.
> 
> We had a similar issue with ARMv7 and handled it by aligning the stack in LowLevelInterpreter.asm:callToJavaScriptPrologue  (see the elsif ARM or ARMv7 or ARMv7_TRADITIONAL case).  We effectively pushed the pre aligned stack pointer on the stack post alignment.  We then restore everything back in LowLevelInterpreter.asm:callToJavaScriptEpilogue.  You may have to do the same thing for Windows.
> 
> You can verify the need for this by checking the stack pointer value before the first instruction of callToJavaScript (or callToNativeFunction).  The least significant nibble should be 'C'.

Thanks, will try this out :) How can we differentiate between x86 Mac, and x86 Win in the .asm file?

[Michael Saboff]

(In reply to comment #39)

> Thanks, will try this out :) How can we differentiate between x86 Mac, and x86 Win in the .asm file?

It would be good to have a new X86_WIN backend in the offline assembler.  X86_WIN could also enable X86, but there are enough differences between X86 for Mac, for Windows and for any other platform to warrant they new backend.
.

It seems Win 32-bit has 4 byte stack alignment, as I've seen both 4, 8, and C as the last nibble in esp when entering the callToJavaScript function.
I guess this means we cannot use fixed offsets of the stack pointer to find the function arguments on the stack.
I've tried aligning the stack to 16 byte as suggested.
I also stored the value (esp & 0xf) in a temp register, and used that to compute the offset of the function arguments.

This is the prologue code I've tried:

    ...
    pushCalleeSaves
    if X86
        subp 12, sp
    elsif X86_WIN
        subp 16, sp
        move sp, t4
        move t4, t0
        move t4, t2
        andp 0xf, t2
        andp 0xfffffff0, t0
        move t0, sp
        storep t4, [sp]
    elsif ARM or ARMv7 or ARMv7_TRADITIONAL
    ...

Code to get function arguments from stack:

    if X86
        loadp 36[sp], vm
        loadp 32[sp], entry
    elsif X86_WIN
        loadp 40[sp,temp3], vm
        loadp 36[sp,temp3], entry
    else
        move cfr, previousCFR
    end

    ...

    if X86
        loadp 28[sp], previousPC
        loadp 24[sp], previousCFR
    elsif X86_WIN
        loadp 32[sp,temp3], previousPC
        loadp 28[sp,temp3], previousCFR
    end
    storep previousPC, ReturnPC[cfr]
    storep previousCFR, CallerFrame[cfr]

    if X86
        loadp 40[sp], protoCallFrame
    elsif X86_WIN
        loadp 44[sp,temp3], protoCallFrame
    end

This works fine in both debug and release, no more asserts, and I can see that the value of the parameters picked off the stack are correct.

Is this an OK way to do it?

[Michael Saboff]

(In reply to comment #41)
> It seems Win 32-bit has 4 byte stack alignment, as I've seen both 4, 8, and C as the last nibble in esp when entering the callToJavaScript function.
> I guess this means we cannot use fixed offsets of the stack pointer to find the function arguments on the stack.
> I've tried aligning the stack to 16 byte as suggested.
> I also stored the value (esp & 0xf) in a temp register, and used that to compute the offset of the function arguments.
> 
> This is the prologue code I've tried:
> 
>     ...
>     pushCalleeSaves
>     if X86
>         subp 12, sp
>     elsif X86_WIN
>         subp 16, sp
>         move sp, t4
>         move t4, t0
>         move t4, t2
>         andp 0xf, t2
>         andp 0xfffffff0, t0
>         move t0, sp
>         storep t4, [sp]
>     elsif ARM or ARMv7 or ARMv7_TRADITIONAL
>     ...
> 
> Code to get function arguments from stack:
> 
>     if X86
>         loadp 36[sp], vm
>         loadp 32[sp], entry
>     elsif X86_WIN
>         loadp 40[sp,temp3], vm
>         loadp 36[sp,temp3], entry
>     else
>         move cfr, previousCFR
>     end
> 
>     ...
> 
>     if X86
>         loadp 28[sp], previousPC
>         loadp 24[sp], previousCFR
>     elsif X86_WIN
>         loadp 32[sp,temp3], previousPC
>         loadp 28[sp,temp3], previousCFR
>     end
>     storep previousPC, ReturnPC[cfr]
>     storep previousCFR, CallerFrame[cfr]
> 
>     if X86
>         loadp 40[sp], protoCallFrame
>     elsif X86_WIN
>         loadp 44[sp,temp3], protoCallFrame
>     end
> 
> This works fine in both debug and release, no more asserts, and I can see that the value of the parameters picked off the stack are correct.
> 
> Is this an OK way to do it?

Looks fine to me.

(nit) Add a space after the ',' and before the "temp3" on the argument access instructions.

(In reply to comment #42)
> 
> Looks fine to me.
> 
> (nit) Add a space after the ',' and before the "temp3" on the argument access instructions.

Thanks :) I think I will create a separate bug/patch for this, since it involves adding another backend (X86_WIN), and to avoid adding more to this bug/patch.

Created attachment 224206 [details]
Patch

(In reply to comment #44)
> Created an attachment (id=224206) [details]
> Patch

Merged the patch with trunk, since there were conflicts.
I will file a separate bug/patch for the stack alignment issues.

[Mark Lam]

Comment on attachment 224206 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:488
> +#elif !OS(WINDOWS) // !ENABLE(LLINT_C_LOOP)

Please remove the // !ENABLE(LLINT_C_LOOP).

> Source/JavaScriptCore/offlineasm/asm.rb:162
> +        raise unless isMSVC

This is only needed if we’re emitting WIN asm, right?  For consistency, change to: raise unless $emitWinAsm.

> Source/JavaScriptCore/offlineasm/asm.rb:168
> +        raise unless isMSVC

For consistency, change to: raise unless $emitWinAsm.

> Source/JavaScriptCore/offlineasm/asm.rb:268
> +    winAsmFile = "LowLevelInterpreterWin.asm"

I’m uncomfortable with hardcoding a file name in here.  We didn’t have this limitation before.

> Source/JavaScriptCore/offlineasm/asm.rb:279
> +    else
> +        # Create a dummy asm file.
> +        # LowLevelInterpreterWin.asm is part of the project, and needs to exist to avoid compile errors.
> +        File.open(winAsmFile, "w") {
> +            | outp |
> +            outp.puts "END"
> +        }

This is an MSVC build issue.  Rather than rigging this lower level tool to make MSVC happy, you could have just provided a DefaultLowLevelInterpreterWin.asm file, and have your project copy that to LowLevelInterpreterWin.asm before running the offlineasm.  If the offlineasm generates a real LowLevelInterpreter.asm, it will overwrite the default version that you copied over first.  Can you fix this?  That way, we can also avoid hardcoding the name above, and just use the outputFileName that was passed in. 

Come think of it, this createAsmFile() function isn’t needed.  You can have your project do any necessary copying and renaming as needed.

> Source/JavaScriptCore/offlineasm/asm.rb:287
> +        mask = 1 << 11

This seems extremely fragile to me.  How did you come up with this 1 << 11 mask?

> Source/JavaScriptCore/offlineasm/settings.rb:181
> +        $output.puts "INCLUDE LLIntSymbolsWin.asm"

Instead of hardcoding the name here to be LLIntSymbolsWin.asm, can you just generate it as outputFlnm + “.sym”?  Hence:
    $output.puts "INCLUDE #{outputFlnm}.sym”

Would that work?

> Source/JavaScriptCore/offlineasm/x86.rb:827
> +            File.open("LLIntSymbolsWin.asm", "a") {

Similarly, us "#{outputFlnm}.sym” here instead of “LLIntSymbolsWin.asm”.

Created attachment 224237 [details]
Patch

(In reply to comment #46)
> (From update of attachment 224206 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224206&action=review
> 

Thanks for reviewing :) Updated patch according to your feedback.
The decision to emit asm code is now based on the input filename to asm.rb.
This way it can be controlled by the windows build env, and we can avoid the
unneeded complexity in the ruby script.

[Mark Lam]

Comment on attachment 224237 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 224237 [details]
Patch

Clearing flags on attachment: 224237

Committed r164164: <http://trac.webkit.org/changeset/164164>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Comment on attachment 224237 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:29
> +        * llint/LowLevelInterpreter.asm: Remove instruction which generates incorrect assembly code on Windows (MOV 0xbbadbeef, register), call llint_crash instead.

This seems like really shady engineering.  Why can't you generate that instruction on Windows?  It's just an x86 instruction, and I'm sure that MASM has an encoding for it.

[Mark Lam]

(In reply to comment #52)
> (From update of attachment 224237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224237&action=review
> 
> > Source/JavaScriptCore/ChangeLog:29
> > +        * llint/LowLevelInterpreter.asm: Remove instruction which generates incorrect assembly code on Windows (MOV 0xbbadbeef, register), call llint_crash instead.
> 
> This seems like really shady engineering.  Why can't you generate that instruction on Windows?  It's just an x86 instruction, and I'm sure that MASM has an encoding for it.

I thought of that when I reviewed it, but then thought that calling a helper function had an advantage.  Specifically, this way, we can crash in the platform defined way as specified by the CRASH() macro, and we don’t have a separate interpretation of what we should do to crash in the LLINT.  With that, I didn’t bother pushing for the implementation of the equivalent asm instruction.

(In reply to comment #52)
> (From update of attachment 224237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224237&action=review
> 
> > Source/JavaScriptCore/ChangeLog:29
> > +        * llint/LowLevelInterpreter.asm: Remove instruction which generates incorrect assembly code on Windows (MOV 0xbbadbeef, register), call llint_crash instead.
> 
> This seems like really shady engineering.  Why can't you generate that instruction on Windows?  It's just an x86 instruction, and I'm sure that MASM has an encoding for it.

My mistake,

I should have investigated further why this doesn't seem to work with MASM.

The instruction "storei t0, 0xbbadbeef[]" translates to "mov 3148725999, eax" on Windows.
This fails to compile with the message "error A2001:immediate operand not allowed".
The same goes for the instruction "mov [3148725999], eax"

[Michael Saboff]

(In reply to comment #54)
> (In reply to comment #52)
> > (From update of attachment 224237 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=224237&action=review
> > 
> > > Source/JavaScriptCore/ChangeLog:29
> > > +        * llint/LowLevelInterpreter.asm: Remove instruction which generates incorrect assembly code on Windows (MOV 0xbbadbeef, register), call llint_crash instead.
> > 
> > This seems like really shady engineering.  Why can't you generate that instruction on Windows?  It's just an x86 instruction, and I'm sure that MASM has an encoding for it.
> 
> My mistake,
> 
> I should have investigated further why this doesn't seem to work with MASM.
> 
> The instruction "storei t0, 0xbbadbeef[]" translates to "mov 3148725999, eax" on Windows.
> This fails to compile with the message "error A2001:immediate operand not allowed".
> The same goes for the instruction "mov [3148725999], eax"

A little looking on the web says that masm may have an issue with "mov [123], <reg>".
What about "mov dword ptr [3148725999], eax"?  If that doesn't work, put 0xbbadbeef in one register and use it as the destination address.

(In reply to comment #55)
> 
> A little looking on the web says that masm may have an issue with "mov [123], <reg>".
> What about "mov dword ptr [3148725999], eax"?  If that doesn't work, put 0xbbadbeef in one register and use it as the destination address.

Same error with "mov dword ptr [3148725999], eax".
Only other option seems to be to go via another register, as you suggest :)