Summary: | Add ARM64 disassembler support for symbolic dumping of some well known constants. | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, ews-watchlist, keith_miller, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Attachments: |
|
Description
Mark Lam
2022-05-15 21:46:36 PDT
Created attachment 459388 [details]
patch for EWS testing.
Created attachment 459393 [details]
for EWS testing.
Created attachment 459397 [details]
for EWS testing
Created attachment 459399 [details]
EWS testing.
Created attachment 459400 [details]
EWS testing.
Created attachment 459404 [details]
EWS testing
Created attachment 459405 [details]
EWS testing
Created attachment 459406 [details]
EWS testing
Created attachment 459451 [details]
EWS testing.
Created attachment 459468 [details]
proposed patch.
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 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. Created attachment 459471 [details]
patch for landing.
Landed in r294287: <http://trac.webkit.org/r294287>. |