Bug 129807 – [Win64] Compile error after r165128.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

Changeset r165128 introduced new compile errors for Win64.

The LLINT x86 backend is not enabled for Win64, so we should not generate an asm file.

Created attachment 226018 [details]
Patch

Created attachment 226133 [details]
Patch

[Alex Christensen]

This seems good to me, but someone who works more with the LLINT_C_LOOP would have to review it.  I'm not sure if using the return value of LLIntOffsetsExtractor is the best way to tell if the LLINT_C_LOOP is enabled or if the comment should mention the x86 backend.  This is only for Windows, right?

The MSVC HAVE_COMPUTED_GOTO change seems a little strange, though.  Did MSVC used to have computed goto and it doesn't anymore, or did it never have computed goto?  Was your r165128 wrong to add this?  Should this be in a separate patch?  It doesn't seem related.

(In reply to comment #3)

Thanks for looking into this :)

> This seems good to me, but someone who works more with the LLINT_C_LOOP would have to review it.  I'm not sure if using the return value of LLIntOffsetsExtractor is the best way to tell if the LLINT_C_LOOP is enabled or if the comment should mention the x86 backend.  This is only for Windows, right?
>

Yes, Windows only.
 
> The MSVC HAVE_COMPUTED_GOTO change seems a little strange, though.  Did MSVC used to have computed goto and it doesn't anymore, or did it never have computed goto?  Was your r165128 wrong to add this?  Should this be in a separate patch?  It doesn't seem related.

No, MSVC never had it, r165128 added this, but that was wrong.
Win64 failed to compile because of this (when using the label value operator &&), so it is reverted reverted in this patch,
but then Win32 needed some compile fixes, since it had been compiling with HAVE_COMPUTED_GOTO.

[Mark Lam]

Comment on attachment 226133 [details]
Patch

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

> Source/JavaScriptCore/bytecode/Opcode.h:82
> -#if ENABLE(COMPUTED_GOTO_OPCODES)
> +#if ENABLE(COMPUTED_GOTO_OPCODES) || !ENABLE(LLINT_C_LOOP)

Why do you need to add the !ENABLE(LLINT_C_LOOP) conditional?  The 2 seems independent to me.  I think this is not needed.

> Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:96
> +
> +    // The return value is used by the build system to determine if we are building with the C loop backend or the x86 backend.
> +    // See WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\LLInt\LLIntAssembly\build-LLIntAssembly.sh
> +
> +#if ENABLE(LLINT_C_LOOP)
>      return 0;
> +#else
> +    return 1;
> +#endif

This feels like a hack because you’re making the LLIntOffsetsExtractor do more than what it is intended to do.  Is there a better way to solve this problem?

[Mark Lam]

Comment on attachment 226133 [details]
Patch

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

> Source/JavaScriptCore/JavaScriptCore.vcxproj/LLInt/LLIntAssembly/build-LLIntAssembly.sh:40
> -# When enabling LLINT and switching to the x86 backend, use "LowLevelInterpreterWin.asm" as output file when running asm.rb.
> +# The return value of LLIntOffsetsExtractor.exe is used to determine
> +# if the C loop backend is enabled, and we should generate LLIntAssembly.h, 
> +# or the x86 backend is enabled, and we should generate LowLevelInterpreterWin.asm
> +
> +${BUILT_PRODUCTS_DIR}/LLIntOffsetsExtractor/LLIntOffsetsExtractor${3}.exe
> +
> +if [ "$?" == "1" ]; then
> +    OUTPUTFILENAME="LowLevelInterpreterWin.asm"
> +else
> +    OUTPUTFILENAME="LLIntAssembly.h"
> +fi
>  
> -/usr/bin/env ruby "${SRCROOT}/offlineasm/asm.rb" "${SRCROOT}/llint/LowLevelInterpreter.asm" "${BUILT_PRODUCTS_DIR}/LLIntOffsetsExtractor/LLIntOffsetsExtractor${3}.exe" "LowLevelInterpreterWin.asm" || exit 1
> +/usr/bin/env ruby "${SRCROOT}/offlineasm/asm.rb" "${SRCROOT}/llint/LowLevelInterpreter.asm" "${BUILT_PRODUCTS_DIR}/LLIntOffsetsExtractor/LLIntOffsetsExtractor${3}.exe" "${OUTPUTFILENAME}" || exit 1

OK, here’s an idea:  note how MSVC sets the env var $BUILT_PRODUCT_DIR.  I think MSVC should already have an env var for 64-bit vs 32-bit.  Use that determine which approach of LLINT you want to take instead.

(In reply to comment #6)
> (From update of attachment 226133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226133&action=review
> 
> > Source/JavaScriptCore/JavaScriptCore.vcxproj/LLInt/LLIntAssembly/build-LLIntAssembly.sh:40
> > -# When enabling LLINT and switching to the x86 backend, use "LowLevelInterpreterWin.asm" as output file when running asm.rb.
> > +# The return value of LLIntOffsetsExtractor.exe is used to determine
> > +# if the C loop backend is enabled, and we should generate LLIntAssembly.h, 
> > +# or the x86 backend is enabled, and we should generate LowLevelInterpreterWin.asm
> > +
> > +${BUILT_PRODUCTS_DIR}/LLIntOffsetsExtractor/LLIntOffsetsExtractor${3}.exe
> > +
> > +if [ "$?" == "1" ]; then
> > +    OUTPUTFILENAME="LowLevelInterpreterWin.asm"
> > +else
> > +    OUTPUTFILENAME="LLIntAssembly.h"
> > +fi
> >  
> > -/usr/bin/env ruby "${SRCROOT}/offlineasm/asm.rb" "${SRCROOT}/llint/LowLevelInterpreter.asm" "${BUILT_PRODUCTS_DIR}/LLIntOffsetsExtractor/LLIntOffsetsExtractor${3}.exe" "LowLevelInterpreterWin.asm" || exit 1
> > +/usr/bin/env ruby "${SRCROOT}/offlineasm/asm.rb" "${SRCROOT}/llint/LowLevelInterpreter.asm" "${BUILT_PRODUCTS_DIR}/LLIntOffsetsExtractor/LLIntOffsetsExtractor${3}.exe" "${OUTPUTFILENAME}" || exit 1
> 
> OK, here’s an idea:  note how MSVC sets the env var $BUILT_PRODUCT_DIR.  I think MSVC should already have an env var for 64-bit vs 32-bit.  Use that determine which approach of LLINT you want to take instead.

Thanks, will try that :)

(In reply to comment #5)
> (From update of attachment 226133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226133&action=review
> 
> > Source/JavaScriptCore/bytecode/Opcode.h:82
> > -#if ENABLE(COMPUTED_GOTO_OPCODES)
> > +#if ENABLE(COMPUTED_GOTO_OPCODES) || !ENABLE(LLINT_C_LOOP)
> 
> Why do you need to add the !ENABLE(LLINT_C_LOOP) conditional?  The 2 seems independent to me.  I think this is not needed.

This was done to fix compile errors for Win32 when COMPUTED_GOTO_OPCODES is not enabled, but this is probably not the best way to fix it ... :)

e.g.

1>..\llint\LLIntData.cpp(59): error C2440: 'static_cast' : cannot convert from 'void *' to 'JSC::Opcode' There is no context in which this conversion is possible

[Mark Lam]

Comment on attachment 226133 [details]
Patch

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

>>> Source/JavaScriptCore/bytecode/Opcode.h:82
>>> +#if ENABLE(COMPUTED_GOTO_OPCODES) || !ENABLE(LLINT_C_LOOP)
>> 
>> Why do you need to add the !ENABLE(LLINT_C_LOOP) conditional?  The 2 seems independent to me.  I think this is not needed.
> 
> This was done to fix compile errors for Win32 when COMPUTED_GOTO_OPCODES is not enabled, but this is probably not the best way to fix it ... :)
> 
> e.g.
> 
> 1>..\llint\LLIntData.cpp(59): error C2440: 'static_cast' : cannot convert from 'void *' to 'JSC::Opcode' There is no context in which this conversion is possible

Sounds like the real issue is that for 32-bit Win, you want the equivalent of COMPUTED_GOTO_OPCODES, but for 64-bit Win, you don’t.  It doesn’t directly have to do with LLINT_C_LOOP, but I guess it is fair to add !ENABLE(LLINT_C_LOOP) as a condition because the ASM LLINT only use the equivalent of computed gotos.  But instead of adding “|| !ENABLE(LLINT_C_LOOP)” all over, lets’s change, in Platform.h, the condition for "#define ENABLE_COMPUTED_GOTO_OPCODES 1” to "#if HAVE(COMPUTED_GOTO) || !ENABLE(LLINT_C_LOOP)”.

(In reply to comment #9)
> (From update of attachment 226133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226133&action=review
> 
> >>> Source/JavaScriptCore/bytecode/Opcode.h:82
> >>> +#if ENABLE(COMPUTED_GOTO_OPCODES) || !ENABLE(LLINT_C_LOOP)
> >> 
> >> Why do you need to add the !ENABLE(LLINT_C_LOOP) conditional?  The 2 seems independent to me.  I think this is not needed.
> > 
> > This was done to fix compile errors for Win32 when COMPUTED_GOTO_OPCODES is not enabled, but this is probably not the best way to fix it ... :)
> > 
> > e.g.
> > 
> > 1>..\llint\LLIntData.cpp(59): error C2440: 'static_cast' : cannot convert from 'void *' to 'JSC::Opcode' There is no context in which this conversion is possible
> 
> Sounds like the real issue is that for 32-bit Win, you want the equivalent of COMPUTED_GOTO_OPCODES, but for 64-bit Win, you don’t.  It doesn’t directly have to do with LLINT_C_LOOP, but I guess it is fair to add !ENABLE(LLINT_C_LOOP) as a condition because the ASM LLINT only use the equivalent of computed gotos.  But instead of adding “|| !ENABLE(LLINT_C_LOOP)” all over, lets’s change, in Platform.h, the condition for "#define ENABLE_COMPUTED_GOTO_OPCODES 1” to "#if HAVE(COMPUTED_GOTO) || !ENABLE(LLINT_C_LOOP)”.

Great, thanks, patch coming up :)

Created attachment 226154 [details]
Patch

[Mark Lam]

Comment on attachment 226154 [details]
Patch

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

Looks much better, but need one more comment so that other folks who look at this will understand why the "|| !ENABLE(LLINT_C_LOOP)" was added, and won't unknowingly remove it.

> Source/WTF/ChangeLog:8
> +        * wtf/Platform.h: MSVC does not support computed goto.

You should also add a comment about the COMPUTED_GOTO_OPCODES e.g.:
"Also enabled COMPUTED_GOTO_OPCODES when !ENABLE(LLINT_C_LOOP).  This is needed because the ASM LLINT operates like COMPUTED_GOTO_OPCODES and relies on the related data structures being defined to support this.  On Win32, the platform does not HAVE_COMPUTED_GOTO support, but does want ENABLE_COMPUTED_GOTO_OPCODES because it uses the ASM LLINT."

Created attachment 226158 [details]
Patch

[Mark Lam]

Comment on attachment 226158 [details]
Patch

r=me

[Mark Lam]

Comment on attachment 226158 [details]
Patch

r=me

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

Thanks!

[WebKit Commit Bot]

Comment on attachment 226158 [details]
Patch

Rejecting attachment 226158 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 226158, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
of 1 hunk FAILED -- saving rejects to file Source/JavaScriptCore/JavaScriptCore.vcxproj/LLInt/LLIntAssembly/build-LLIntAssembly.sh.rej
patching file Source/WTF/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WTF/wtf/Platform.h
Hunk #1 FAILED at 790.
1 out of 1 hunk FAILED -- saving rejects to file Source/WTF/wtf/Platform.h.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Mark Lam']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/6239574170796032

[Brent Fulgham]

(In reply to comment #17)
> (From update of attachment 226158 [details])
> Rejecting attachment 226158 [details] from commit-queue.
> 
> Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 226158, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit
> 
> Last 500 characters of output:
> of 1 hunk FAILED -- saving rejects to file Source/JavaScriptCore/JavaScriptCore.vcxproj/LLInt/LLIntAssembly/build-LLIntAssembly.sh.rej
> patching file Source/WTF/ChangeLog
> Hunk #1 succeeded at 1 with fuzz 3.
> patching file Source/WTF/wtf/Platform.h
> Hunk #1 FAILED at 790.
> 1 out of 1 hunk FAILED -- saving rejects to file Source/WTF/wtf/Platform.h.rej
> 
> Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Mark Lam']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
> 
> Full output: http://webkit-queues.appspot.com/results/6239574170796032

Does this still need to be landed? It doesn't look like the commit queue processed it.

(In reply to comment #18)
> 
> Does this still need to be landed? It doesn't look like the commit queue processed it.

I think it was committed in changeset 165296.

[Brent Fulgham]

Committed in r165296. <http://trac.webkit.org/changeset/165296>