Bug 181313 – TypedArrays and Wasm should use index masking.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Keith Miller]

...

[Keith Miller]

Created attachment 330507 [details]
Patch

[Saam Barati]

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

[Keith Miller]

Created attachment 330515 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Keith Miller]

Created attachment 330533 [details]
fix bad merge

[JF Bastien]

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.

[Keith Miller]

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.

[Keith Miller]

(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.

[Keith Miller]

Created attachment 330571 [details]
Patch

[Michael Saboff]

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.

[Saam Barati]

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.

[Keith Miller]

Committed r226461: <https://trac.webkit.org/changeset/226461>

[Radar WebKit Bug Importer]

<rdar://problem/36326253>

[Saam Barati]

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?

[Maciej Stachowiak]

*** Bug 180920 has been marked as a duplicate of this bug. ***