Bug 212382

Summary: SamplingProfiler::takeSample() should not assume that ENABLE(WEBASSEMBLY) means Wasm is enabled.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
saam: review+, saam: commit-queue-
proposed patch. none

Description Mark Lam 2020-05-26 12:17:41 PDT
Fixing this will allow sampling profiler tests to run with JSC_useJIT=0 without crashing.
Comment 1 Mark Lam 2020-05-26 12:20:45 PDT
Created attachment 400267 [details]
proposed patch.
Comment 2 Saam Barati 2020-05-26 12:23:25 PDT
Comment on attachment 400267 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:3
> +        SamplingProfiler::takeSample() should assume that ENABLE(WEBASSEMBLY) means Wasm is enabled.

should => should not
Comment 3 Saam Barati 2020-05-26 12:26:10 PDT
Comment on attachment 400267 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/SamplingProfiler.cpp:357
> +        Lock unusedLock;
> +        auto wasmCalleesLocker =  holdLock(Wasm::isSupported() ? Wasm::CalleeRegistry::singleton().getLock() : unusedLock);

why not just use Optional<LockHolder>? Seems cleaner than this unusedLock approach
Comment 4 Mark Lam 2020-05-26 13:49:15 PDT
Created attachment 400270 [details]
proposed patch.
Comment 5 EWS 2020-05-26 14:32:54 PDT
Committed r262161: <https://trac.webkit.org/changeset/262161>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400270 [details].
Comment 6 Radar WebKit Bug Importer 2020-05-26 14:33:25 PDT
<rdar://problem/63642209>