Bug 169794

Summary:

WebAssembly: spec-tests/memory.wast.js fails in debug

Product:

WebKit

Reporter:

JF Bastien <jfbastien>

Component:

JavaScriptCore

Assignee:

JF Bastien <jfbastien>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, fpizlo, jfbastien, keith_miller, mark.lam, msaboff, saam

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

159775

Attachments:

Description	Flags
patch	jfbastien: review-, jfbastien: commit-queue-
patch	keith_miller: review-, keith_miller: commit-queue-
patch	none
patch	keith_miller: review+, keith_miller: commit-queue-
patch	none

JF Bastien

Reported 2017-03-16 15:36:52 PDT

Looks related to Signaling versus BoundsChecking when calling current_memory. (cd ./JSTests/wasm/ && ../../current-debug/bin/jsc -m --useWebAssembly=1 ./spec-tests/memory.wast.js); echo $? ASSERTION FAILED: wasmFunction->instance()->codeBlock()->isSafeToRun(wasmFunction->instance()->memory()) ../Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp(60) : EncodedJSValue JSC::callWebAssemblyFunction(JSC::ExecState *) 1 0x108d2bd7d WTFCrash 2 0x108c3e190 JSC::callWebAssemblyFunction(JSC::ExecState*) 3 0x10878ce8a JSC::LLInt::handleHostCall(JSC::ExecState*, JSC::Instruction*, JSC::JSValue, JSC::CodeSpecializationKind) 4 0x1087890bd JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) 5 0x108788383 JSC::LLInt::genericCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind) 6 0x108788273 llint_slow_path_call 7 0x108795ba9 llint_entry 8 0x1087959cf llint_entry 9 0x1087959cf llint_entry 10 0x10878e29e vmEntryToJavaScript 11 0x108723aee JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 12 0x1086d99ad JSC::Interpreter::execute(JSC::ModuleProgramExecutable*, JSC::ExecState*, JSC::JSModuleEnvironment*) 13 0x1089fee5c JSC::JSModuleRecord::evaluate(JSC::ExecState*) 14 0x1089f677e JSC::JSModuleLoader::evaluate(JSC::ExecState*, JSC::JSValue, JSC::JSValue, JSC::JSValue) 15 0x108a8f7d1 JSC::moduleLoaderPrototypeEvaluate(JSC::ExecState*) 16 0x445c14801028 17 0x1087959cf llint_entry 18 0x1087959cf llint_entry 19 0x1087959cf llint_entry 20 0x10878e29e vmEntryToJavaScript 21 0x108723aee JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 22 0x1086d81d2 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 23 0x1088eede8 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 24 0x1088ef05a JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 25 0x1089f0eb8 JSC::JSJobMicrotask::run(JSC::ExecState*) 26 0x108b56243 JSC::QueuedTask::run() 27 0x108b55e77 JSC::VM::drainMicrotasks() 28 0x10780b430 runWithScripts(GlobalObject*, WTF::Vector<Script, 0ul, WTF::CrashOnOverflow, 16ul> const&, WTF::String const&, bool, bool, bool) 29 0x1077cfcdf jscmain(int, char**)::$_9::operator()(JSC::VM&, GlobalObject*) const 30 0x1077c2e82 int runJSC<jscmain(int, char**)::$_9>(CommandLine, jscmain(int, char**)::$_9 const&) 31 0x1077c1b18 jscmain(int, char**) 139 The failing spec tests do the following: (module (memory (data)) (func (export "memsize") (result i32) (current_memory))) (assert_return (invoke "memsize") (i32.const 0)) (module (memory (data "")) (func (export "memsize") (result i32) (current_memory))) (assert_return (invoke "memsize") (i32.const 0)) (module (memory (data "x")) (func (export "memsize") (result i32) (current_memory))) (assert_return (invoke "memsize") (i32.const 1))

Attachments
patch (18.47 KB, patch) 2017-03-17 17:07 PDT, JF Bastien	jfbastien: review- jfbastien: commit-queue-	Details Formatted Diff Diff
patch (21.14 KB, patch) 2017-03-20 14:38 PDT, JF Bastien	keith_miller: review- keith_miller: commit-queue-	Details Formatted Diff Diff
patch (36.75 KB, patch) 2017-03-21 12:46 PDT, JF Bastien	no flags	Details Formatted Diff Diff
patch (34.12 KB, patch) 2017-03-23 17:04 PDT, JF Bastien	keith_miller: review+ keith_miller: commit-queue-	Details Formatted Diff Diff
patch (32.53 KB, patch) 2017-03-24 14:42 PDT, JF Bastien	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

JF Bastien

Comment 1 2017-03-17 16:53:00 PDT

I'm taking a look at this. I have a partial patch. Will continue next week.

JF Bastien

Comment 2 2017-03-17 17:07:06 PDT

Created attachment 304839 [details] patch Turns out I did it now.

WebKit Commit Bot

Comment 3 2017-03-17 17:08:36 PDT

Attachment 304839 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:241: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:100: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:261: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:89: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

JF Bastien

Comment 4 2017-03-17 17:14:09 PDT

Comment on attachment 304839 [details] patch Actually there's a bug when there *are* loads and stores. What I need to do is change all provably out-of-bounds loads and stores to traps, and add a bunch of tests. Also, test when the memory is passed in as None... Ugh, this zero-sized memory thing is silly.

JF Bastien

Comment 5 2017-03-20 14:38:52 PDT

Created attachment 304950 [details] patch Fix a few logic errors.

JF Bastien

Comment 6 2017-03-20 14:39:46 PDT

(In reply to comment #4) > Comment on attachment 304839 [details] > patch > > Actually there's a bug when there *are* loads and stores. > > What I need to do is change all provably out-of-bounds loads and stores to > traps, and add a bunch of tests. Also, test when the memory is passed in as > None... > > Ugh, this zero-sized memory thing is silly. I didn't do this: I ended up changing the checks so that Signaling code can't be used with None as well as BoundsChecking memory. Much simpler.

WebKit Commit Bot

Comment 7 2017-03-20 14:41:39 PDT

Attachment 304950 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:243: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:100: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:261: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:89: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Keith Miller

Comment 8 2017-03-20 22:47:01 PDT

Comment on attachment 304950 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=304950&action=review Can't we just have the Memory object point to null or something and fix weirdness with setMemory using fake memories. It seems like an anti-pattern to make everyone deal with all the edge cases of empty memories. > Source/JavaScriptCore/wasm/WasmMemory.h:54 > + enum class Mode { Can we not make this an enum class? or if you do at least remove it from the Memory class. Writing Wasm::Memory::Mode everywhere is going to be super lame...

JF Bastien

Comment 9 2017-03-21 00:31:25 PDT

(In reply to comment #8) > Comment on attachment 304950 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=304950&action=review > > Can't we just have the Memory object point to null or something and fix > weirdness with setMemory using fake memories. It seems like an anti-pattern > to make everyone deal with all the edge cases of empty memories. I don't believe it would. We have the following cases to handle in the InstanceConstructor: 1. An imported memory which isn't empty. 2. An imported memory which is empty (max=0). 3. No memory. 4. A section-declared empty memory (max=0). 5. A section-declared memory. I should rename "None" to "Empty" to express this. What you're asking is: should we put 2., 3., and 4. together? I don't think so: having *no* memory means there can be no memory accesses *at all* in the module's code. That's a much stronger guarantee from saying that there can be no memory *access* which execute dynamically, but getting memory size and growing it to 0 is fine. You'll note that we do collapse 2. and 4. together: that's because once instantiated, where the memory came from doesn't matter because the lifetime is controlled by a WebAssembly.Memory object. > > Source/JavaScriptCore/wasm/WasmMemory.h:54 > > + enum class Mode { > > Can we not make this an enum class? or if you do at least remove it from the > Memory class. Writing Wasm::Memory::Mode everywhere is going to be super > lame... I can drop that change. I think it reads better ("the memory mode is signaling" is more accurate than "the memory is signaling"), but that distracts from the patch. I could do MemoryMode::Signaling too. In general I like enum class much more! It's way harder to mess up.

JF Bastien

Comment 10 2017-03-21 12:46:43 PDT

Created attachment 305014 [details] patch Updated patch.

Build Bot

Comment 11 2017-03-22 00:32:20 PDT

Attachment 305014 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:255: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:71: The parameter name "mode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:100: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:261: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:89: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 5 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

JF Bastien

Comment 12 2017-03-23 17:04:56 PDT

Created attachment 305246 [details] patch Remove the enum value I was adding, Keith was right the code stays simpler without the new value. Empty is just denoted by having max=0.

Build Bot

Comment 13 2017-03-23 17:08:10 PDT

Attachment 305246 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:286: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:71: The parameter name "mode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:88: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:92: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 4 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Keith Miller

Comment 14 2017-03-24 10:52:30 PDT

Comment on attachment 305246 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=305246&action=review r=me if you fix the copyright thingy. > Source/JavaScriptCore/wasm/WasmMemory.cpp:171 > + , m_mode(MemoryMode::BoundsChecking) Nit: I don't think you need this. It has an initializer in the class definition. > Source/JavaScriptCore/wasm/WasmMemory.cpp:235 > + // FIXME: should this ever occur? https://bugs.webkit.org/show_bug.cgi?id=169890 I think we have proven this can happen. I'm not sure this FIXME is needed. > Source/JavaScriptCore/wasm/WasmMemory.cpp:298 > + // FIXME: should this ever occur? https://bugs.webkit.org/show_bug.cgi?id=169890 ditto. > Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:65 > + // because the page protection detect out-of-bounds accesses. typo: detects > Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp:2 > + * Copyright (C) 2016-2017 Apple Inc. All rights reserved. Did we have changes in here earlier that we forgot to update? If so, I think you should state that's why you are changing the copyright in the Changelog. > Source/JavaScriptCore/wasm/js/WebAssemblyMemoryConstructor.cpp:2 > + * Copyright (C) 2016-2017 Apple Inc. All rights reserved. ditto.

JF Bastien

Comment 15 2017-03-24 14:42:41 PDT

Created attachment 305324 [details] patch > > Source/JavaScriptCore/wasm/WasmMemory.cpp:235 > > + // FIXME: should this ever occur? https://bugs.webkit.org/show_bug.cgi?id=169890 > > I think we have proven this can happen. I'm not sure this FIXME is needed. > > > Source/JavaScriptCore/wasm/WasmMemory.cpp:298 > > + // FIXME: should this ever occur? https://bugs.webkit.org/show_bug.cgi?id=169890 > > ditto. Right, I want to add checks for errno though because some of the failure cases mean we messed up. So the FIXME should stay there, I'll address as part of bug #169890.

Build Bot

Comment 16 2017-03-24 14:43:54 PDT

Attachment 305324 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:286: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:71: The parameter name "mode" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:88: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:92: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 4 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

WebKit Commit Bot

Comment 17 2017-03-24 15:09:19 PDT

Comment on attachment 305324 [details] patch Clearing flags on attachment: 305324 Committed r214380: <http://trac.webkit.org/changeset/214380>

WebKit Commit Bot

Comment 18 2017-03-24 15:09:25 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.