Bug 162689

Summary: B3 should support trapping memory accesses
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, saam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 162692, 162699    
Bug Blocks: 162688, 162693    
Attachments:
Description Flags
work in progress
none
the patch
none
the patch ggaren: review+

Filip Pizlo
Reported 2016-09-28 10:50:27 PDT
WebAssembly can be made to run faster if you let virtual memory do some safety checks for you. But that means that B3 needs to know that memory accesses can side-exit in an observable way.
Attachments
work in progress (25.72 KB, patch)
2016-09-30 12:29 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (43.55 KB, patch)
2016-09-30 15:21 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (43.53 KB, patch)
2016-09-30 16:03 PDT, Filip Pizlo 		ggaren: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2016-09-28 10:50:54 PDT
Keith: which WebAssembly bug should this block?
Filip Pizlo
Comment 2 2016-09-30 12:29:11 PDT
Created attachment 290363 [details] work in progress
Filip Pizlo
Comment 3 2016-09-30 15:21:40 PDT
Created attachment 290399 [details] the patch
Filip Pizlo
Comment 4 2016-09-30 16:03:50 PDT
Created attachment 290402 [details] the patch
Geoffrey Garen
Comment 5 2016-09-30 16:07:18 PDT
Comment on attachment 290402 [details] the patch r=me > Source/JavaScriptCore/b3/B3LowerToAir.cpp:212 > + ArgPromise(ArgPromise&& other) > + { > + swap(other); > + } > + > + ArgPromise& operator=(ArgPromise&& other) > + { > + swap(other); > + return *this; > + } It is generally an anti-pattern for move construction or assignment to swap. If ArgPromise had non-POD data, which benefited from swap, its destructors would run at the wrong time. Since ArgPromise has only POD data, there's no problem, but also no reason to swap. I recommend simple assignment.
Filip Pizlo
Comment 6 2016-09-30 16:13:40 PDT
(In reply to comment #5) > Comment on attachment 290402 [details] > the patch > > r=me > > > Source/JavaScriptCore/b3/B3LowerToAir.cpp:212 > > + ArgPromise(ArgPromise&& other) > > + { > > + swap(other); > > + } > > + > > + ArgPromise& operator=(ArgPromise&& other) > > + { > > + swap(other); > > + return *this; > > + } > > It is generally an anti-pattern for move construction or assignment to swap. > > If ArgPromise had non-POD data, which benefited from swap, its destructors > would run at the wrong time. > > Since ArgPromise has only POD data, there's no problem, but also no reason > to swap. > > I recommend simple assignment. But there is a reason to swap! See: ~ArgPromise() { if (m_wasConsumed) RELEASE_ASSERT(m_wasWrapped); } I get that the fields of ArgPromise look like PODs, but that doesn't matter, since they're used to implement very un-POD-like logic.
Filip Pizlo
Comment 7 2016-09-30 17:11:35 PDT
Landed in https://trac.webkit.org/changeset/206694
Note You need to log in before you can comment on or make changes to this bug.