Bug 240370 – Enhance the ARM64Disassembler to print pc indices and better branch target labels.

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

[Mark Lam]

Details and patch coming.

[Mark Lam]

Created attachment 459277 [details]
proposed patch.

[Mark Lam]

Comment on attachment 459277 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:47
> +           Introduced a FINALIZE_AND_RETURN_THUNK macro that will be used instead of
> +           "return FINALIZE_CODE".  By doing so, thunk labels will automatically be

The first sentence here should read: 
        Introduced a FINALIZE_AND_RETURN_THUNK macro that will be used instead of
        "return FINALIZE_CODE" in thunk generators."

Will fix before landing.

[Mark Lam]

Created attachment 459279 [details]
proposed patch.

[Saam Barati]

Comment on attachment 459279 [details]
proposed patch.

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

Nice. r=me

> Source/JavaScriptCore/ChangeLog:33
> +        2. Relative branches now show the branch target as a pc offset in addition to the
> +           pc address e.g.

maybe "pc offset" => "offset from the start of the function"?

> Source/JavaScriptCore/ChangeLog:44
> +               <828> 0x10e12033c:    bl       0x10e0f0a00 -> <thunk: get_from_scope thunk>
> +              <1476> 0x10e120dc4:    cbnz     x16, 0x10e104100 -> <thunk: handleExceptionWithCallFrameRollback>
> +              <2368> 0x10e121140:    b        0x10e10c000 -> <thunk: DFG OSR exit generation thunk>

Nice!

> Source/JavaScriptCore/ChangeLog:57
> +               <168> 0x10e1002e8:    b        0x10e120b60 -> <external: JIT PC>

I think based on this algorithm, you mean external just in the sense that it's not owned by this exact compilation. It still might be "internal" to this particular CodeBlock. I wonder if this may confuse some people. But maybe it's fine. 

I'd be tempted to just label all three of these things in this section without "external: " prefixing them.

so just <LLInt PC>, <JIT PC>, <unknown>

> Source/JavaScriptCore/llint/LLIntThunks.cpp:112
> +    FINALIZE_AND_RETURN_THUNK(patchBuffer, tag, "LLInt %s jump to prologue thunk", thunkKind);

I think this is more characters than just "return FINALIZE_THUNK", and doing the return manually is less magic

[Mark Lam]

Comment on attachment 459279 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/ChangeLog:33
>> +           pc address e.g.
> 
> maybe "pc offset" => "offset from the start of the function"?

Fixed.

>> Source/JavaScriptCore/ChangeLog:57
>> +               <168> 0x10e1002e8:    b        0x10e120b60 -> <external: JIT PC>
> 
> I think based on this algorithm, you mean external just in the sense that it's not owned by this exact compilation. It still might be "internal" to this particular CodeBlock. I wonder if this may confuse some people. But maybe it's fine. 
> 
> I'd be tempted to just label all three of these things in this section without "external: " prefixing them.
> 
> so just <LLInt PC>, <JIT PC>, <unknown>

SGTM.  Changed.

>> Source/JavaScriptCore/llint/LLIntThunks.cpp:112
>> +    FINALIZE_AND_RETURN_THUNK(patchBuffer, tag, "LLInt %s jump to prologue thunk", thunkKind);
> 
> I think this is more characters than just "return FINALIZE_THUNK", and doing the return manually is less magic

The only reason I did this was because I needed to execute another statement before evaluating FINALIZE_CODE to produce the result that would be returned.  However, it looks like I can use a comma separated expression list to achieve this.  So, I'll go ahead and change all these to "return FINALIZE_THUNK".

[Mark Lam]

Created attachment 459323 [details]
patch for landing.

[Mark Lam]

Landed in r294180: <http://trac.webkit.org/r294180>.

[Radar WebKit Bug Importer]

<rdar://problem/93270571>