Bug 178690 - WebAssembly: topEntryFrame on Wasm::Instance
Summary: WebAssembly: topEntryFrame on Wasm::Instance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebAssembly (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords: InRadar
Depends on: 178699
Blocks: 177472
  Show dependency treegraph
 
Reported: 2017-10-23 16:37 PDT by JF Bastien
Modified: 2017-11-15 12:58 PST (History)
9 users (show)

See Also:


Attachments
patch (25.22 KB, patch)
2017-10-23 16:46 PDT, JF Bastien
saam: review+
Details | Formatted Diff | Diff
patch (25.16 KB, patch)
2017-10-23 16:54 PDT, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2017-10-23 16:37:09 PDT
topEntryFrame is usually on VM, but for a no-VM WebAssembly we need to hold topEntryFrame elsewhere, and generated code cannot hard-code where topEntryFrame live. Do this at creation time of Wasm::Instance, and then generated code will just load from wherever Wasm::Instance was told topEntryFrame is. In a JavaScript embedding this is still from VM, so all of the unwinding machinery stays the same.
Comment 1 JF Bastien 2017-10-23 16:46:47 PDT
Created attachment 324611 [details]
patch
Comment 2 Saam Barati 2017-10-23 16:49:35 PDT
Comment on attachment 324611 [details]
patch

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

> Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:137
> +    EntryFrame** m_topEntryFramePointer { nullptr };

Why have this in both JSWebAssemblyInstance and WasmInstance?
Comment 3 JF Bastien 2017-10-23 16:51:48 PDT
(In reply to Saam Barati from comment #2)
> Comment on attachment 324611 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324611&action=review
> 
> > Source/JavaScriptCore/wasm/js/JSWebAssemblyInstance.h:137
> > +    EntryFrame** m_topEntryFramePointer { nullptr };
> 
> Why have this in both JSWebAssemblyInstance and WasmInstance?

JSWebAssemblyInstance copies a bunch of things from Wasm::Instance for now, because the goal I'm driving towards is having Wasm::Context be Wasm::Instance (not JSWebAssemblyInstance), and I want the swap to be as trivial as possible (i.e. change one line to do that swap, then delete all the copies from JSWebAssemblyInstance). If I do it right everything else will be the same.
Comment 4 JF Bastien 2017-10-23 16:54:31 PDT
Created attachment 324613 [details]
patch
Comment 5 WebKit Commit Bot 2017-10-23 17:29:43 PDT
Comment on attachment 324613 [details]
patch

Clearing flags on attachment: 324613

Committed r223866: <https://trac.webkit.org/changeset/223866>
Comment 6 WebKit Commit Bot 2017-10-23 17:29:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 WebKit Commit Bot 2017-10-23 19:31:27 PDT
Re-opened since this is blocked by bug 178699
Comment 9 Keith Miller 2017-10-23 20:20:37 PDT
Committed r223875: <https://trac.webkit.org/changeset/223875>
Comment 10 JF Bastien 2017-10-23 21:58:33 PDT
(In reply to Keith Miller from comment #7)
> I think this broke the windows build?
> https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> 5466/steps/compile-webkit/logs/stdio

lolwut?
Comment 11 Radar WebKit Bug Importer 2017-11-15 12:58:18 PST
<rdar://problem/35568509>