...
Created attachment 330507 [details] Patch
Comment on attachment 330507 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330507&action=review > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:672 > + remove whitespace
Created attachment 330515 [details] Patch
Comment on attachment 330515 [details] Patch Attachment 330515 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5935799 Number of test failures exceeded the failure limit.
Created attachment 330523 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 330515 [details] Patch Attachment 330515 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5935815 Number of test failures exceeded the failure limit.
Created attachment 330524 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 330515 [details] Patch Attachment 330515 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5935839 New failing tests: webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-array-buffer-view.html fast/canvas/webgl/drawingbuffer-test.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-canvas-rgb565.html webgl/1.0.2/conformance/rendering/line-loop-tri-fan.html fast/canvas/webgl/uninitialized-test.html webgl/1.0.3/conformance/extensions/oes-texture-float-linear.html webgl/1.0.2/conformance/uniforms/uniform-default-values.html fast/canvas/webgl/oes-texture-half-float.html webgl/1.0.2/conformance/glsl/samplers/glsl-function-texture2dlod.html webgl/1.0.2/conformance/glsl/samplers/glsl-function-texture2d-bias.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-canvas-rgba5551.html fast/canvas/webgl/object-deletion-behaviour.html webgl/1.0.2/conformance/canvas/drawingbuffer-hd-dpi-test.html webgl/1.0.2/conformance/glsl/misc/shader-with-short-circuiting-operators.html webgl/1.0.2/conformance/extensions/oes-texture-float-with-canvas.html fast/canvas/webgl/multisample-resolve-consistency.html webgl/1.0.2/conformance/misc/uninitialized-test.html webgl/1.0.2/conformance/uniforms/out-of-bounds-uniform-array-access.html fast/canvas/webgl/premultiplyalpha-test.html webgl/1.0.2/conformance/attribs/gl-vertex-attrib-zero-issues.html webgl/1.0.2/conformance/textures/texture-size-cube-maps.html webgl/1.0.2/conformance/attribs/gl-disabled-vertex-attrib.html webgl/1.0.2/conformance/extensions/webgl-compressed-texture-s3tc.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-canvas.html webgl/1.0.2/conformance/textures/texture-size.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-canvas-rgba4444.html
Created attachment 330525 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 330515 [details] Patch Attachment 330515 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5935930 Number of test failures exceeded the failure limit.
Created attachment 330529 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 330533 [details] fix bad merge
Comment on attachment 330533 [details] fix bad merge View in context: https://bugs.webkit.org/attachment.cgi?id=330533&action=review I'm a bit OCD... So the datamembers in WasmMemory are laid out as memory, size, indexMask. But we don't load them in that order! Could you load them in their layout order? r=me on the WebAssembly part with some fixes. The rest looks fine, but it's better to have someone else review that as well. > Source/JavaScriptCore/b3/B3LowerToAir.cpp:3279 > + // should not actually change the value of the pointer we should be OK. I'd say this differently: if masking changes the pointer then we know we're executing speculatively and the branch would have gone the other way. > Source/JavaScriptCore/b3/testb3.cpp:15888 > + root->appendNew<WasmBoundsCheckValue>(proc, Origin(), pinned, InvalidGPRReg, left, offset); Can you also test with a pinned mask register, to make sure the masking doesn't get hoisted and truncate the check? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:672 > } FIXME to do something mask when offset >= redzone? I think csel is fine here instead of mask (since it's not a power of 2). > Source/JavaScriptCore/wasm/WasmMemoryInformation.h:40 > +} Include wtf/Forward.h instead. > Source/WTF/wtf/MathExtras.h:478 > +inline uint32_t computeIndexingMask(uint32_t size) I think you need to have mask == 0 when size == 0. Right now it'll be 0xFFFFFFFF. > Source/WTF/wtf/MathExtras.h:480 > + return static_cast<uint64_t>(static_cast<uint32_t>(-1)) >> std::clz(size); FWIW on MSVC this requires at least a Haswell machine, which is 2013. Since it's in WTF I assume we're fine with this.
Comment on attachment 330533 [details] fix bad merge View in context: https://bugs.webkit.org/attachment.cgi?id=330533&action=review >> Source/JavaScriptCore/b3/testb3.cpp:15888 >> + root->appendNew<WasmBoundsCheckValue>(proc, Origin(), pinned, InvalidGPRReg, left, offset); > > Can you also test with a pinned mask register, to make sure the masking doesn't get hoisted and truncate the check? We don't hoist in Air but sure. >> Source/JavaScriptCore/wasm/WasmMemoryInformation.h:40 >> +} > > Include wtf/Forward.h instead. Fixed. >> Source/WTF/wtf/MathExtras.h:478 >> +inline uint32_t computeIndexingMask(uint32_t size) > > I think you need to have mask == 0 when size == 0. Right now it'll be 0xFFFFFFFF. If size == 0 this function will return 0. >> Source/WTF/wtf/MathExtras.h:480 >> + return static_cast<uint64_t>(static_cast<uint32_t>(-1)) >> std::clz(size); > > FWIW on MSVC this requires at least a Haswell machine, which is 2013. Since it's in WTF I assume we're fine with this. ? std::clz will always return 32 if size == 0. I explicitly put the branch there for the case where we don't have LZCNT on X86.
(In reply to JF Bastien from comment #13) > Comment on attachment 330533 [details] > > I'm a bit OCD... So the datamembers in WasmMemory are laid out as memory, > size, indexMask. But we don't load them in that order! Could you load them > in their layout order? We load them in different orders in different places... If you want to clean that up in a follow up patch rs=me but I'm not sure it's worth holding this patch back on that.
Created attachment 330571 [details] Patch
Comment on attachment 330571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330571&action=review r=me > Source/JavaScriptCore/b3/B3Validate.cpp:477 > + Eliminate extra added blank line. > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:-1481 > - Revert this white space change.
Comment on attachment 330571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330571&action=review r=me w/ comments > Source/JavaScriptCore/b3/B3LowerToAir.cpp:3279 > + // Hypothetically, this could write to the pointer value. Which we didn't claim we did but since we assume the indexing mask > + // should not actually change the value of the pointer we should be OK. I'm not sure the wording of this is great. We really need to write to the pointer because we want Air to pick the same tmp for load following this bounds check. It's still worth keeping the text about pointer not changing. Obviously something is badly wrong if the index masking changes the pointer. > Source/JavaScriptCore/b3/B3WasmBoundsCheckValue.h:79 > + remove newline > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13149 > + LValue mask = m_out.load32(base, m_heaps.JSObject_butterflyMask); What about doing this in LLInt/Baseline? > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1481 > + revert > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1299 > + jit.loadPtr(CCallHelpers::Address(baseMemory, Memory::offsetOfIndexingMask()), pinnedRegs.indexingMask); // Indexing mask. > jit.loadPtr(CCallHelpers::Address(baseMemory, Memory::offsetOfSize()), sizeRegs[0].sizeRegister); // Memory size. > jit.loadPtr(CCallHelpers::Address(baseMemory, Memory::offsetOfMemory()), baseMemory); // Memory::void*. These comments seem useless. I know you're just following what the code around it does, but we should really just remove these comments since they're self evident. > Source/JavaScriptCore/wasm/WasmBinding.cpp:71 > + jit.loadPtr(CCallHelpers::Address(baseMemory, Memory::offsetOfIndexingMask()), pinnedRegs.indexingMask); // Indexing mask. > jit.loadPtr(JIT::Address(baseMemory, Wasm::Memory::offsetOfSize()), sizeRegs[0].sizeRegister); // Memory size. > jit.loadPtr(JIT::Address(baseMemory, Wasm::Memory::offsetOfMemory()), baseMemory); // Wasm::Memory::void*. ditto.
Committed r226461: <https://trac.webkit.org/changeset/226461>
<rdar://problem/36326253>
Comment on attachment 330571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330571&action=review >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13149 >> + LValue mask = m_out.load32(base, m_heaps.JSObject_butterflyMask); > > What about doing this in LLInt/Baseline? Can you address this?
*** Bug 180920 has been marked as a duplicate of this bug. ***