Bug 240443 – Add ARM64 disassembler support for symbolic dumping of some well known constants.

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

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 459388 [details]
patch for EWS testing.

[Mark Lam]

Created attachment 459393 [details]
for EWS testing.

[Mark Lam]

Created attachment 459397 [details]
for EWS testing

[Mark Lam]

Created attachment 459399 [details]
EWS testing.

[Mark Lam]

Created attachment 459400 [details]
EWS testing.

[Mark Lam]

Created attachment 459404 [details]
EWS testing

[Mark Lam]

Created attachment 459405 [details]
EWS testing

[Mark Lam]

Created attachment 459406 [details]
EWS testing

[Mark Lam]

Created attachment 459451 [details]
EWS testing.

[Mark Lam]

Created attachment 459468 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 459468 [details]
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?

[Mark Lam]

Comment on attachment 459468 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/ChangeLog:119
>> +           insignificant (since the added check is cheap).
> 
> Is it true for all supported OSes (Windows, Linux, FreeBSD)? Or, should we have `OS(DARWIN)` guard?

I recall that this is true, but maybe I might have remembered wrongly.  I'll add the OS(DARWIN) guard to be safe.

>> Source/JavaScriptCore/disassembler/Disassembler.cpp:52
>> +}
> 
> We need a lock around access to this hashmap since concurrent compilers & concurrent VMs on workers can put things into this hashmap.

I was thinking that all initialization happens at boot, but that's actually not true.  Thunks can be added lazily IIRC.  I'll add a lock.

>> Source/WTF/ChangeLog:13
>> +USE(APPLE_INTERNAL_SDK) && ENABLE(DISASSEMBLER) && CPU(ARM64E)
> 
> Is it a typo?

Fixed.  Thanks.

[Mark Lam]

Created attachment 459471 [details]
patch for landing.

[Mark Lam]

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

[Radar WebKit Bug Importer]

<rdar://problem/93398681>