Bug 240370 - Enhance the ARM64Disassembler to print pc indices and better branch target labels.
Summary: Enhance the ARM64Disassembler to print pc indices and better branch target la...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-05-12 23:16 PDT by Mark Lam
Modified: 2022-05-13 15:29 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch. (61.34 KB, patch)
2022-05-12 23:48 PDT, Mark Lam
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
proposed patch. (62.93 KB, patch)
2022-05-13 00:00 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
patch for landing. (62.75 KB, patch)
2022-05-13 15:27 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2022-05-12 23:16:59 PDT
Details and patch coming.
Comment 1 Mark Lam 2022-05-12 23:48:55 PDT
Created attachment 459277 [details]
proposed patch.
Comment 2 Mark Lam 2022-05-12 23:53:56 PDT
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.
Comment 3 Mark Lam 2022-05-13 00:00:12 PDT
Created attachment 459279 [details]
proposed patch.
Comment 4 Saam Barati 2022-05-13 14:33:43 PDT
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
Comment 5 Mark Lam 2022-05-13 14:55:57 PDT
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".
Comment 6 Mark Lam 2022-05-13 15:27:33 PDT
Created attachment 459323 [details]
patch for landing.
Comment 7 Mark Lam 2022-05-13 15:28:57 PDT
Landed in r294180: <http://trac.webkit.org/r294180>.
Comment 8 Radar WebKit Bug Importer 2022-05-13 15:29:14 PDT
<rdar://problem/93270571>