[webkit-reviews] review denied: [Bug 128115] [Win] LLINT is not working. : [Attachment 224206] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied peavo at outlook.com's request for
review:
Bug 128115: [Win] LLINT is not working.
https://bugs.webkit.org/show_bug.cgi?id=128115

Attachment 224206: Patch
https://bugs.webkit.org/attachment.cgi?id=224206&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
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”.