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: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 159775 | ||||||||||||||
Attachments: |
|
Description
JF Bastien
2017-03-16 15:36:52 PDT
I'm taking a look at this. I have a partial patch. Will continue next week. Created attachment 304839 [details]
patch
Turns out I did it now.
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.
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.
Created attachment 304950 [details]
patch
Fix a few logic errors.
(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. 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.
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... (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. Created attachment 305014 [details]
patch
Updated patch.
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.
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.
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.
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. 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. 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.
Comment on attachment 305324 [details] patch Clearing flags on attachment: 305324 Committed r214380: <http://trac.webkit.org/changeset/214380> All reviewed patches have been landed. Closing bug. |