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

[bugzilla-daemon at webkit.org]

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


Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #224206|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #46 from Mark Lam <mark.lam at apple.com>  2014-02-14 10:03:26 PST ---
(From update of attachment 224206)
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”.

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