Replace `object` function arguments in WebGL with typed arrays #20396

Xanewok · 2018-03-23T00:27:15Z

Could use a servo/rust-mozjs#402 in some places, as this should simplify a little bit and remove unnecessary #[allow(unsafe_code)] attributes.

I sort of hacked my way through for #20394 since I encountered this issue as well. I agree that the comment above makes me feel uneasy about this and feels like I'm missing something and this shouldn't be the way we eventually resolve the overloads with auto rootable types (talking about this: https://github.com/servo/servo/pull/20396/files#diff-60d01595cff328c165842fea9e4ccbc2R428).

r? @jdm

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #20342 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because if the bindings compile now, it works!

This change is

highfive · 2018-03-23T00:27:20Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/webgl2renderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webglrenderingcontext.rs
@fitzgen: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/webgl2renderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webglrenderingcontext.rs
@KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/webgl2renderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webglrenderingcontext.rs

highfive · 2018-03-23T00:27:21Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

jdm · 2018-03-23T02:57:20Z

@bors-servo try

bors-servo · 2018-03-23T02:57:25Z

⌛ Trying commit 7a157ce with merge b082f84...


        Auto merge of #20396 - Xanewok:webgl-typed-arrays, r=<try>

@jdm

Replace `object` function arguments in WebGL with typed arrays  Could use a servo/rust-mozjs#402 in some places, as this should simplify a little bit and remove unnecessary `#[allow(unsafe_code)]` attributes. I sort of hacked my way through for #20394 since I encountered this issue as well. I agree that the comment above makes me feel uneasy about this and feels like I'm missing something and this shouldn't be the way we eventually resolve the overloads with auto rootable types. r? @jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #20342 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because if the bindings compile now, it works!    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20396)

bors-servo · 2018-03-23T03:23:05Z

💔 Test failed - linux-rel-css

jdm · 2018-03-23T03:39:18Z

  ▶ CRASH [expected OK] /_mozilla/webgl/conformance-1.0.3/conformance/more/functions/vertexAttribBadArgs.html
  │ 
  │ VMware, Inc.
  │ softpipe
  │ 3.3 (Core Profile) Mesa 17.3.0-devel
  │ index out of bounds: the len is 0 but the index is 0 (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(NonZero(NonZero(1))) }, at /checkout/src/liballoc/vec.rs:1551)
  │ stack backtrace:
  │    0:     0x7fc7ec2b1d4c - backtrace::backtrace::trace::h73d318fa86b64c9a
  │    1:     0x7fc7ec2b0ef2 - backtrace::capture::Backtrace::new::hd1c50bb2491b7ea5
  │    2:     0x7fc7e9385c1b - servo::main::{{closure}}::ha3221c6785682ba0
  │    3:     0x7fc7ec2bf8f5 - std::panicking::rust_panic_with_hook::haa00f5a9417cd684
  │                         at libstd/panicking.rs:577
  │    4:     0x7fc7ec2bf77e - std::panicking::begin_panic::ha956683198499384
  │                         at libstd/panicking.rs:537
  │    5:     0x7fc7ec2bf67a - std::panicking::begin_panic_fmt::h587e116719f14631
  │                         at libstd/panicking.rs:521
  │    6:     0x7fc7ec2bf612 - rust_begin_unwind
  │                         at libstd/panicking.rs:497
  │    7:     0x7fc7ec2f4730 - core::panicking::panic_fmt::h1d64949939b0af2f
  │                         at libcore/panicking.rs:71
  │    8:     0x7fc7ec2f46d3 - core::panicking::panic_bounds_check::h93d5bf91b62abb88
  │                         at libcore/panicking.rs:58
  │    9:     0x7fc7e9abc659 - <script::dom::webglrenderingcontext::WebGLRenderingContext as script::dom::bindings::codegen::Bindings::WebGLRenderingContextBinding::WebGLRenderingContextBinding::WebGLRenderingContextMethods>::VertexAttrib1fv_::h46019afa299fa59b
  │   10:     0x7fc7ea52f971 - <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once::h0edede5d357f1913
  │   11:     0x7fc7ea4e458f - _ZN3std9panicking3try7do_call17h5de7737a9c8933b5E.llvm.27392D34
  │   12:     0x7fc7ec2e8ece - __rust_maybe_catch_panic
  │                         at libpanic_unwind/lib.rs:102
  │   13:     0x7fc7ea0a681d - script::dom::bindings::codegen::Bindings::WebGLRenderingContextBinding::WebGLRenderingContextBinding::vertexAttrib1fv::hff0a43f235a8c2aa
  │   14:     0x7fc7ebaba1d4 - CallJitMethodOp
  │   15:     0x7fc7e9cff241 - script::dom::bindings::utils::generic_call::h95fc30bf0ed8997f
  │   16:     0x7fc7e9cff77e - script::dom::bindings::utils::generic_method::h3a1e63fab82a8f63
  │   17:     0x7fc7ebe16410 - CallJSNative
  │                         at /home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/mozjs_sys-0.50.1/mozjs/js/src/jscntxtinlines.h:232
  │                          - _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
  │                         at /home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/mozjs_sys-0.50.1/mozjs/js/src/vm/Interpreter.cpp:453
  │   18:     0x7fc7ebe165c4 - _ZN2js4CallEP9JSContextN2JS6HandleINS2_5ValueEEES5_RKNS_13AnyInvokeArgsENS2_13MutableHandleIS4_EE
  │                         at /home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/mozjs_sys-0.50.1/mozjs/js/src/vm/Interpreter.cpp:517
  │   19:     0x7fc7ebd1429f - _ZN2js9fun_applyEP9JSContextjPN2JS5ValueE
  │                         at /home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/mozjs_sys-0.50.1/mozjs/js/src/jsfun.cpp:1325
  │   20:     0x7fc7ebe16410 - CallJSNative
  │                         at /home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/mozjs_sys-0.50.1/mozjs/js/src/jscntxtinlines.h:232
  │                          - _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
  │                         at /home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/mozjs_sys-0.50.1/mozjs/js/src/vm/Interpreter.cpp:453
  │   21:     0x7fc7ebae9054 - DoCallFallback
  │                         at /home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/mozjs_sys-0.50.1/mozjs/js/src/jit/BaselineIC.cpp:5970
  │   22:     0x7fc7e00d7f58 - <unknown>
  │ ERROR:servo: index out of bounds: the len is 0 but the index is 0
  └ Pipeline failed in hard-fail mode.  Crashing!

Xanewok · 2018-03-23T10:10:02Z

Right, I wanted to be so smart about the deduplication in function overloads, so I actually didn't move sanity check to the proper function in vertex attribs. Oops!

Hopefully this should work now, this passes webgl test suite for me locally.

jdm

I'm really excited to see these changes! The typed array binding support is a big improvement :)

jdm · 2018-03-23T14:58:06Z

components/script/dom/webglrenderingcontext.rs

@@ -825,15 +825,13 @@ impl WebGLRenderingContext {
    }

    // TODO(emilio): Move this logic to a validator.
-    #[allow(unsafe_code)]
-    unsafe fn validate_tex_image_2d_data(&self,
+    fn validate_tex_image_2d_data(&self,
                                         width: u32,


nit: indentation

jdm · 2018-03-23T14:58:06Z

components/script/dom/bindings/codegen/CodegenRust.py

-                        "arg%d" % distinguishingIndex)
+                        argName)
+                    if type_needs_auto_root(type):
+                        testCode.append(CGGeneric("auto_root!(in(cx) let %s = %s);" % (argName, argName)))


What if we move this into instantiateJSToNativeConversionTemplate instead? Can we also add codegen tests for:

two overloads - ArrayBuffer and DOMString

three overloads - a union containing ArrayBuffer and DOMString

Specifically, if we add a field to JSToNativeConversionInfo and make getJSToNativeConversionInfo set it appropriately, instantiateJSToNativeConversionTemplate could add the extra auto_root! declaration.

Ah, now that I read your reply, it seems I misread it. Do you think 01c6bee is sufficient for now or should I change it further?

Re overloads:
Currently unions at distinguishable indices are not supported by CodegenRust (see WebGLRenderingContext.webidl:493), so this still blocks us on removing last object? workaround function here.

I can definitely add codegen tests for overloads with auto rootable types here though (e.g. overloads on ArrayBuffer and DOMString)

I'm a bit worried that we'll keep hitting similar issues with missing CustomAutoRoot wrappers if we keep doing spot-fixes that only change a single call-site at a time.

Yeah, I agree completely.

However in this particular case I'm not sure what might be the best approach.
In general getJSToNativeConversionInfo returns template, declType and default, which is then later passed manually to instantiateJSToNativeConversionTemplate.

In the updated 01c6bee, for the instantiateJSToNativeConversionTemplate call I'm passing the information if the intantiated template needs the rooting (which is later done inside the call).

If I'm not mistaken, do you mean that we should return an additional field (e.g. needsAutoRooting) from getJSToNativeConversionInfo and pass it later manually for every following instantiateJSToNativeConversionTemplate call if needed, as well?

Bleah. We should really refactor the code generation to pass the JSToNativeConversionInfo class directly to instantiateJSToNativeConversionTemplate so it's easier to make changes like this. I'll file a separate issue to do that and clean up the changes from this PR.

Filed #20403.


        Change WebGL function signatures accepting typed arrays


        Adapt function bodies for usage with typed array args


        Adapt uniform[fv] and similar to accept typed array args


        Appease test-tidy


        Fix sanity check in vertex attrib


        Fix indentation


        Unify argument auto rooting in codegen


        Add codegen test for function overloads taking typed array args


        Use safe to_vec() for typed arrays in WebGL bindings

Xanewok · 2018-03-23T18:39:52Z

Rebased and force-pushed to take advantage of servo/rust-mozjs#402 (less unsafe! 🎉)

jdm · 2018-03-23T19:15:34Z

@bors-servo r+

bors-servo · 2018-03-23T19:15:35Z

📌 Commit 80c6891 has been approved by jdm

bors-servo · 2018-03-23T19:15:41Z

⌛ Testing commit 80c6891 with merge e8fdc67...


        Auto merge of #20396 - Xanewok:webgl-typed-arrays, r=jdm

@jdm

Replace `object` function arguments in WebGL with typed arrays  Could use a servo/rust-mozjs#402 in some places, as this should simplify a little bit and remove unnecessary `#[allow(unsafe_code)]` attributes. I sort of hacked my way through for #20394 since I encountered this issue as well. I agree that the comment above makes me feel uneasy about this and feels like I'm missing something and this shouldn't be the way we eventually resolve the overloads with auto rootable types (talking about this: https://github.com/servo/servo/pull/20396/files#diff-60d01595cff328c165842fea9e4ccbc2R428). r? @jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #20342 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because if the bindings compile now, it works!    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20396)

bors-servo · 2018-03-23T20:49:58Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing e8fdc67 to master...

highfive assigned jdm Mar 23, 2018

highfive added the S-awaiting-review label Mar 23, 2018

highfive added the S-tests-failed label Mar 23, 2018

highfive removed the S-tests-failed label Mar 23, 2018

jdm requested changes Mar 23, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 23, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 23, 2018

Xanewok added 9 commits Mar 22, 2018

Change WebGL function signatures accepting typed arrays

36f39ce

Adapt function bodies for usage with typed array args

c1c74ed

Adapt uniform[fv] and similar to accept typed array args

20f21cb

Appease test-tidy

daceeb4

Fix sanity check in vertex attrib

cb85a95

Fix indentation

c42127b

Unify argument auto rooting in codegen

2437a84

Add codegen test for function overloads taking typed array args

fbfcc76

Use safe to_vec() for typed arrays in WebGL bindings

Loading status checks…

80c6891

Xanewok force-pushed the Xanewok:webgl-typed-arrays branch from 2584199 to 80c6891 Mar 23, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 23, 2018

bors-servo merged commit 80c6891 into servo:master Mar 23, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Mar 23, 2018

Xanewok mentioned this pull request Mar 23, 2018

Generate bindings properly for overloads with typed arrays #20394

Open

Xanewok deleted the Xanewok:webgl-typed-arrays branch Mar 23, 2018

servo / servo

Replace `object` function arguments in WebGL with typed arrays #20396

Replace `object` function arguments in WebGL with typed arrays #20396

Xanewok commented Mar 23, 2018 •

edited

highfive commented Mar 23, 2018

highfive commented Mar 23, 2018

jdm commented Mar 23, 2018

bors-servo commented Mar 23, 2018

bors-servo commented Mar 23, 2018

jdm commented Mar 23, 2018

Xanewok commented Mar 23, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xanewok commented Mar 23, 2018

jdm commented Mar 23, 2018

bors-servo commented Mar 23, 2018

bors-servo commented Mar 23, 2018

bors-servo commented Mar 23, 2018

servo / servo

Join GitHub today

Replace `object` function arguments in WebGL with typed arrays #20396

Replace `object` function arguments in WebGL with typed arrays #20396

Conversation

Xanewok commented Mar 23, 2018 • edited

highfive commented Mar 23, 2018

highfive commented Mar 23, 2018

jdm commented Mar 23, 2018

bors-servo commented Mar 23, 2018

bors-servo commented Mar 23, 2018

jdm commented Mar 23, 2018

Xanewok commented Mar 23, 2018

jdm left a comment

This comment has been minimized.

jdm Mar 23, 2018

This comment has been minimized.

jdm Mar 23, 2018

This comment has been minimized.

jdm Mar 23, 2018

This comment has been minimized.

Xanewok Mar 23, 2018 • edited

This comment has been minimized.

jdm Mar 23, 2018

This comment has been minimized.

Xanewok Mar 23, 2018

This comment has been minimized.

jdm Mar 23, 2018

This comment has been minimized.

jdm Mar 23, 2018

Xanewok commented Mar 23, 2018

jdm commented Mar 23, 2018

bors-servo commented Mar 23, 2018

bors-servo commented Mar 23, 2018

bors-servo commented Mar 23, 2018

Xanewok commented Mar 23, 2018 •

edited

Xanewok Mar 23, 2018 •

edited