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>