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

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

Summary: Add ARM64 disassembler support for symbolic dumping of some well known consta...

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	JavaScriptCore (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Mark Lam

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2022-05-15 21:46 PDT by Mark Lam
Modified:	2022-05-17 02:13 PDT (History)
CC List:	10 users (show)

See Also:

Attachments
patch for EWS testing. (41.42 KB, patch) 2022-05-15 21:47 PDT, Mark Lam	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
for EWS testing. (41.43 KB, patch) 2022-05-15 22:22 PDT, Mark Lam	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
for EWS testing (43.46 KB, patch) 2022-05-15 22:43 PDT, Mark Lam	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
EWS testing. (43.47 KB, patch) 2022-05-15 23:01 PDT, Mark Lam	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
EWS testing. (43.42 KB, patch) 2022-05-15 23:08 PDT, Mark Lam	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
EWS testing (43.61 KB, patch) 2022-05-15 23:22 PDT, Mark Lam	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
EWS testing (43.60 KB, patch) 2022-05-15 23:27 PDT, Mark Lam	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
EWS testing (43.63 KB, patch) 2022-05-15 23:37 PDT, Mark Lam	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
EWS testing. (47.60 KB, patch) 2022-05-16 13:30 PDT, Mark Lam	no flags	Details \| Formatted Diff \| Diff
proposed patch. (57.82 KB, patch) 2022-05-16 17:46 PDT, Mark Lam	ysuzuki: review+	Details \| Formatted Diff \| Diff
patch for landing. (58.05 KB, patch) 2022-05-16 18:46 PDT, Mark Lam	ews-feeder: commit-queue-	Details \| Formatted Diff \| Diff
Show Obsolete (10) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Mark Lam 2022-05-15 21:46:36 PDT

Patch coming.

Comment 1 Mark Lam 2022-05-15 21:47:22 PDT

Created attachment 459388 [details]
patch for EWS testing.

Comment 2 Mark Lam 2022-05-15 22:22:05 PDT

Created attachment 459393 [details]
for EWS testing.

Comment 3 Mark Lam 2022-05-15 22:43:06 PDT

Created attachment 459397 [details]
for EWS testing

Comment 4 Mark Lam 2022-05-15 23:01:36 PDT

Created attachment 459399 [details]
EWS testing.

Comment 5 Mark Lam 2022-05-15 23:08:01 PDT

Created attachment 459400 [details]
EWS testing.

Comment 6 Mark Lam 2022-05-15 23:22:49 PDT

Created attachment 459404 [details]
EWS testing

Comment 7 Mark Lam 2022-05-15 23:27:23 PDT

Created attachment 459405 [details]
EWS testing

Comment 8 Mark Lam 2022-05-15 23:37:44 PDT

Created attachment 459406 [details]
EWS testing

Comment 9 Mark Lam 2022-05-16 13:30:14 PDT

Created attachment 459451 [details]
EWS testing.

Comment 10 Mark Lam 2022-05-16 17:46:06 PDT

Created attachment 459468 [details]
proposed patch.

Comment 11 Yusuke Suzuki 2022-05-16 18:09:57 PDT

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?

Comment 12 Mark Lam 2022-05-16 18:19:04 PDT

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.

Comment 13 Mark Lam 2022-05-16 18:46:27 PDT

Created attachment 459471 [details]
patch for landing.

Comment 14 Mark Lam 2022-05-16 22:22:35 PDT

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

Comment 15 Radar WebKit Bug Importer 2022-05-16 22:23:15 PDT

<rdar://problem/93398681>