Bug 237624

Created attachment 454160 [details] Patch

Created attachment 454161 [details] Patch

Created attachment 454162 [details] Patch

Created attachment 454163 [details] Patch

Created attachment 454164 [details] Patch

Comment on attachment 454161 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454161&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:832 > +llintOpWithsetDestinationAndDispatch(op_argument_count, OpArgumentCount, macro (size, get, dispatch, setDestinationAndDispatch) This should be llintOpWithDestination, and same at all the place below. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:839 > +llintOpWithsetDestinationAndDispatch(op_get_scope, OpGetScope, macro (size, get, dispatch, setDestinationAndDispatch) llintOpWithSet... > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:873 > +llintOpWithsetDestinationAndDispatch(op_mov, OpMov, macro (size, get, dispatch, setDestinationAndDispatch) Ditto. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:880 > +llintOpWithsetDestinationAndDispatch(op_not, OpNot, macro (size, get, dispatch, setDestinationAndDispatch) Ditto ... and all the place below too. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:976 > + llintOpWithsetDestinationAndDispatch(op_%opcodeName%, opcodeStruct, macro (size, get, dispatch, setDestinationAndDispatch) Ditto > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1083 > +llintOpWithsetDestinationAndDispatch(op_to_string, OpToString, macro (size, get, dispatch, setDestinationAndDispatch) Ditto. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2366 > + dosetDestinationAndDispatch() Wrong search and replace: should be doReturn? > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2370 > +llintOpWithsetDestinationAndDispatch(op_to_primitive, OpToPrimitive, macro (size, get, dispatch, setDestinationAndDispatch) llintOpWithDestination > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2384 > +llintOpWithsetDestinationAndDispatch(op_to_property_key, OpToPropertyKey, macro (size, get, dispatch, setDestinationAndDispatch) llintOpWithDestination > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2450 > + dosetDestinationAndDispatch() Wrong search and replace: should be doReturn? > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2915 > +llintOpWithsetDestinationAndDispatch(op_get_parent_scope, OpGetParentScope, macro (size, get, dispatch, setDestinationAndDispatch) llintOpWithDestination > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:2979 > +llintOpWithsetDestinationAndDispatch(op_get_rest_length, OpGetRestLength, macro (size, get, dispatch, setDestinationAndDispatch) Ditto. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:3201 > + dosetDestinationAndDispatch() Wrong search and replace: should be doReturn? > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2479 > - # prepareCall in LLInt does not untag return address. So we need to untag that in the trampoline separately. > + # prepareCall in LLInt does not untag setDestinationAndDispatch address. So we need to untag that in the trampoline separately. Wrong search replace? Should this one really be "return address"? > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2557 > - # prepareCall in LLInt does not untag return address. So we need to untag that in the trampoline separately. > + # prepareCall in LLInt does not untag setDestinationAndDispatch address. So we need to untag that in the trampoline separately. Ditto: Wrong search replace?

Comment on attachment 454164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454164&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:-1023 > - # return result; Will revert > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:-2479 > - # prepareCall in LLInt does not untag return address. So we need to untag that in the trampoline separately. Will revert

Created attachment 454168 [details] Patch

Comment on attachment 454168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454168&action=review r=me. This patch is huge, and it's very difficult to review this by eyeballing, and be confident that it was done right. It's too easy to miss something, and perhaps introduce a subtle change. Just for a sanity check (if you haven't already done so), please generate a LLIntAssembly.h before and after your patch, and diff the 2 LLIntAssembly.h files. If there are no errors, they should be identical. > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:529 > llintOpWithMetadata(opcodeName, opcodeStruct, macro(size, get, dispatch, metadata, return) /return/setDestinationAndDispatch/ to be consistent. > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2557 > + # prepareCall in LLInt does not untag setDestinationAndDispatch address. So we need to untag that in the trampoline separately. /setDestinationAndDispatch address/the return address/ > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2795 > - macro returnConstantScope() > + macro setDestinationAndDispatchConstantScope() In this case, `returnConstantScope` would be a better name if not for the fact that there's no actual "return" operation. How about `setDestinationAndDispatchWithConstantScope` instead? `setDestinationAndDispatchConstantScope` just doesn't read like English.

<rdar://problem/90337195>