Bug 129807

Created attachment 226018 [details] Patch

Created attachment 226133 [details] Patch

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.

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?

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

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

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

Comment on attachment 226158 [details] Patch r=me

Comment on attachment 226158 [details] Patch r=me

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

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

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

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