Bug 170782 - WebAssembly: don't expose any WebAssembly JS object if JIT is off
Summary: WebAssembly: don't expose any WebAssembly JS object if JIT is off
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on:
Blocks: 159775 170998
  Show dependency treegraph
 
Reported: 2017-04-12 11:31 PDT by JF Bastien
Modified: 2017-04-19 10:12 PDT (History)
8 users (show)

See Also:


Attachments
patch (3.78 KB, patch)
2017-04-19 00:07 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-04-12 11:31:40 PDT
It's kinda misleading of `typeof WebAssembly !== "undefined"` but we know that compilation will never succeed. It's also weird to have WebAssembly.Memory if JIT is off.

A WebKit1 embedding would be a weird place to expose WebAssembly JS objects.
Comment 1 JF Bastien 2017-04-19 00:07:25 PDT
Created attachment 307465 [details]
patch
Comment 2 JF Bastien 2017-04-19 08:26:08 PDT
One thing I noticed when writing this: our tests run with useFTLJIT=0! That's because of BASE_OPTIONS which runWithOutputHandler uses (and in turn run calls runWithOutputHandler).

Initially I'd disabled WebAssembly if useJIT or useFTLJIT was false, but that would require changing the tests and I'm not sure the distinction matters. B3 is really what's used by wasm, and a special blend of it, so I'm not sure useFTLJIT=false is relevant to wasm.

Thoughts?
Comment 3 Saam Barati 2017-04-19 09:52:42 PDT
(In reply to JF Bastien from comment #2)
> One thing I noticed when writing this: our tests run with useFTLJIT=0!
> That's because of BASE_OPTIONS which runWithOutputHandler uses (and in turn
> run calls runWithOutputHandler).
> 
> Initially I'd disabled WebAssembly if useJIT or useFTLJIT was false, but
> that would require changing the tests and I'm not sure the distinction
> matters. B3 is really what's used by wasm, and a special blend of it, so I'm
> not sure useFTLJIT=false is relevant to wasm.
> 
> Thoughts?

I would not tie Wasm being enabled to the ftl being enabled. That said, Wasm tests should run with the FTL enabled.
Comment 4 Keith Miller 2017-04-19 09:57:26 PDT
Does this prevent me from doing --useJIT=0 --dumpDisassembly=1 --useWebAssembly=1? I like doing that because it lets me see the wasm disassembly. I guess we could just add a dumpWebAssemblyDisassembly (wow! that's a mouthful) option.
Comment 5 JF Bastien 2017-04-19 10:02:00 PDT
(In reply to Saam Barati from comment #3)
> (In reply to JF Bastien from comment #2)
> > One thing I noticed when writing this: our tests run with useFTLJIT=0!
> > That's because of BASE_OPTIONS which runWithOutputHandler uses (and in turn
> > run calls runWithOutputHandler).
> > 
> > Initially I'd disabled WebAssembly if useJIT or useFTLJIT was false, but
> > that would require changing the tests and I'm not sure the distinction
> > matters. B3 is really what's used by wasm, and a special blend of it, so I'm
> > not sure useFTLJIT=false is relevant to wasm.
> > 
> > Thoughts?
> 
> I would not tie Wasm being enabled to the ftl being enabled. That said, Wasm
> tests should run with the FTL enabled.

Sounds good.

I filed a bug:
  https://bugs.webkit.org/show_bug.cgi?id=170998
Will address later.



(In reply to Keith Miller from comment #4)
> Does this prevent me from doing --useJIT=0 --dumpDisassembly=1
> --useWebAssembly=1? I like doing that because it lets me see the wasm
> disassembly. I guess we could just add a dumpWebAssemblyDisassembly (wow!
> that's a mouthful) option.

Correct. Without JIT it won't generate the stubs to C++ code anyways, no?
Comment 6 WebKit Commit Bot 2017-04-19 10:12:41 PDT
Comment on attachment 307465 [details]
patch

Clearing flags on attachment: 307465

Committed r215517: <http://trac.webkit.org/changeset/215517>
Comment 7 WebKit Commit Bot 2017-04-19 10:12:42 PDT
All reviewed patches have been landed.  Closing bug.