[webkit-reviews] review granted: [Bug 240370] Enhance the ARM64Disassembler to print pc offsets and better branch target labels. : [Attachment 459279] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 240370: Enhance the ARM64Disassembler to print pc offsets and better branch
target labels.
https://bugs.webkit.org/show_bug.cgi?id=240370

Attachment 459279: proposed patch.

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




--- Comment #4 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 459279
  --> https://bugs.webkit.org/attachment.cgi?id=459279
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