Bug 147761 – Add MacroAssembler::patchableBranch64 and fix ARM64's patchableBranchPtr

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

[Yusuke Suzuki]

Yup. Currently, there's no patchableBranch64 helper function in MacroAssembler; it can reduce the following

cmp some condition
jump_ok pass
patchable jump
pass:

to

cmp some condition
patchable jump_ng

There's patchableBranchPtr in MacroAssembler under 64bit environments.
However, the current MacroAssemblerARM64's patchableBranchPtr seems wrong.

2344     PatchableJump patchableBranchPtr(RelationalCondition cond, Address left, TrustedImmPtr right = TrustedImmPtr(0))
2345     {
2346         m_makeJumpPatchable = true;
2347         Jump result = branch32(cond, left, TrustedImm32(right));
2348         m_makeJumpPatchable = false;
2349         return PatchableJump(result);
2350     }

It truncate the pointer to imm32. (And there's no patchableBranchPtr user in WebKit currently)

[Yusuke Suzuki]

Created attachment 258437 [details]
Patch

[Yusuke Suzuki]

The fix becomes pretty small :)

[Mark Lam]

Comment on attachment 258437 [details]
Patch

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

r=me

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:760
> +    PatchableJump patchableBranch64(RelationalCondition cond, RegisterID reg, TrustedImm64 imm = TrustedImm64(0))

I think the TrustedImm64 arg should not have a default value.  This conditional branch requires a comparison between 2 values.  It would read weird at the call site if of the values being compared is not specified.  Note: I see that compare64(), branch64(), and branchPtr() emitters all do not allow a default value for the argument as well.  The ARM64 version of this patchableBranch64() also do not.

[Yusuke Suzuki]

Comment on attachment 258437 [details]
Patch

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

Thank you for your review.

>> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:760
>> +    PatchableJump patchableBranch64(RelationalCondition cond, RegisterID reg, TrustedImm64 imm = TrustedImm64(0))
> 
> I think the TrustedImm64 arg should not have a default value.  This conditional branch requires a comparison between 2 values.  It would read weird at the call site if of the values being compared is not specified.  Note: I see that compare64(), branch64(), and branchPtr() emitters all do not allow a default value for the argument as well.  The ARM64 version of this patchableBranch64() also do not.

Make sense. I've dropped it. And I've dropped the default arg in ARM64 patchableBranchPtr as well.

[Yusuke Suzuki]

Committed r188135: <http://trac.webkit.org/changeset/188135>