[webkit-reviews] review granted: [Bug 240443] Add ARM64 disassembler support for symbolic dumping of some well known constants. : [Attachment 459468] proposed patch.

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 240443: Add ARM64 disassembler support for symbolic dumping of some well
known constants.
https://bugs.webkit.org/show_bug.cgi?id=240443

Attachment 459468: proposed patch.

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




--- Comment #11 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 459468
  --> https://bugs.webkit.org/attachment.cgi?id=459468
proposed patch.

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

r=me

> Source/JavaScriptCore/ChangeLog:119
> +	   7. Enhanced the Integrity::isSanePointer() check to reject pointer
values that
> +	      are less than 4G on CPU(ADDRESS64) targets.  No sane pointer
should be in the
> +	      lowest 4G address region.
> +
> +	      This also helps the disassembler more quickly filter out constant
values that
> +	      aren't pointers.
> +
> +	      This change affects DFG's speculationFromCell(), which is the
only place that
> +	      may affect user facing performance.  However, I think the impact
should be
> +	      insignificant (since the added check is cheap).

Is it true for all supported OSes (Windows, Linux, FreeBSD)? Or, should we have
`OS(DARWIN)` guard?

> Source/JavaScriptCore/disassembler/Disassembler.cpp:52
> +using LabelMap = HashMap<void*, std::variant<CString, const char*>>;
> +LazyNeverDestroyed<LabelMap> labelMap;
> +
> +static LabelMap& ensureLabelMap()
> +{
> +    static std::once_flag onceKey;
> +    std::call_once(onceKey, [] {
> +	   labelMap.construct();
> +    });
> +    return labelMap.get();
> +}

We need a lock around access to this hashmap since concurrent compilers &
concurrent VMs on workers can put things into this hashmap.

> Source/WTF/ChangeLog:13
> +USE(APPLE_INTERNAL_SDK) && ENABLE(DISASSEMBLER) && CPU(ARM64E)

Is it a typo?