Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] More vertex buffer validation #6804

Merged
merged 4 commits into from
Jan 2, 2025

Conversation

sagudev
Copy link
Contributor

@sagudev sagudev commented Dec 22, 2024

Connections
Fix #6799

Testing
CTS run in servo:

OK /_webgpu/webgpu/cts.https.html?q=webgpu:api,validation,capability_checks,limits,maxVertexAttributes:createRenderPipeline,at_over:*

    PASS [expected FAIL] subtest: :limitTest="atDefault";testValueName="overLimit";async=false
    PASS [expected FAIL] subtest: :limitTest="atDefault";testValueName="overLimit";async=true
    PASS [expected FAIL] subtest: :limitTest="underDefault";testValueName="overLimit";async=false
    PASS [expected FAIL] subtest: :limitTest="underDefault";testValueName="overLimit";async=true
    PASS [expected FAIL] subtest: :limitTest="betweenDefaultAndMaximum";testValueName="overLimit";async=false
    PASS [expected FAIL] subtest: :limitTest="betweenDefaultAndMaximum";testValueName="overLimit";async=true
    PASS [expected FAIL] subtest: :limitTest="atMaximum";testValueName="overLimit";async=false
    PASS [expected FAIL] subtest: :limitTest="atMaximum";testValueName="overLimit";async=true

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@sagudev
Copy link
Contributor Author

sagudev commented Dec 22, 2024

@abey79 could you test if this raises error where you expected to?

abey79 added a commit to abey79/vsvg that referenced this pull request Dec 22, 2024
@abey79
Copy link

abey79 commented Dec 22, 2024

TL;DR: yes, my offending code now panics 👍🏻

edit: on second thought, that message seems wrong: the stride is 8 bytes and the attributes are 32 bytes in total, which is indeed non-compliant (the stride is allowed to be greater, not smaller).

Can be reproduced with this branch (https://github.com/abey79/vsvg/tree/antoine/wgpu-fix-test), which patches wgpu to this commit.

❯ cargo run -p whiskers-web-demo             
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.11s
     Running `target/debug/whiskers-web-demo`
thread 'main' panicked at /Users/hhip/.cargo/git/checkouts/wgpu-53e70f8674b08dd4/b92e624/wgpu/src/backend/wgpu_core.rs:1302:26:
wgpu error: Validation Error

Caused by:
  In Device::create_render_pipeline, label = 'line pipeline'
    Vertex buffer 0 stride 32 exceeds the limit 8


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at library/core/src/panicking.rs:221:5:
panic in a function that cannot unwind
stack backtrace:
   0:        0x1015b9e8c - std::backtrace_rs::backtrace::libunwind::trace::hbebc8679d47bdc2c
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/../../backtrace/src/backtrace/libunwind.rs:116:5
   1:        0x1015b9e8c - std::backtrace_rs::backtrace::trace_unsynchronized::h3a2e9637943241aa
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2:        0x1015b9e8c - std::sys::backtrace::_print_fmt::he430849680584674
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/sys/backtrace.rs:65:5
   3:        0x1015b9e8c - <std::sys::backtrace::BacktraceLock::print::DisplayBacktrace as core::fmt::Display>::fmt::h243268f17d714c7f
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/sys/backtrace.rs:40:26
   4:        0x1015da168 - core::fmt::rt::Argument::fmt::h0d339881c25f3c31
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/fmt/rt.rs:173:76
   5:        0x1015da168 - core::fmt::write::hb3cfb8a30e72d7ff
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/fmt/mod.rs:1182:21
   6:        0x1015b6e8c - std::io::Write::write_fmt::hfb2314975de9ecf1
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/io/mod.rs:1827:15
   7:        0x1015bafec - std::sys::backtrace::BacktraceLock::print::he14461129ccbfef5
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/sys/backtrace.rs:43:9
   8:        0x1015bafec - std::panicking::default_hook::{{closure}}::h14c7718ccf39d316
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:269:22
   9:        0x1015bac10 - std::panicking::default_hook::hc62e60da3be2f352
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:296:9
  10:        0x1015bbab0 - std::panicking::rust_panic_with_hook::h09e8a656f11e82b2
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:800:13
  11:        0x1015bb3d8 - std::panicking::begin_panic_handler::{{closure}}::h1230eb3cc91b241c
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:667:13
  12:        0x1015ba318 - std::sys::backtrace::__rust_end_short_backtrace::hc3491307aceda2c2
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/sys/backtrace.rs:168:18
  13:        0x1015bb0c8 - rust_begin_unwind
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:665:5
  14:        0x10164dbbc - core::panicking::panic_nounwind_fmt::runtime::h5290ab2b4897aadc
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:112:18
  15:        0x10164dbbc - core::panicking::panic_nounwind_fmt::h91ee161184879b56
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:122:5
  16:        0x10164dc34 - core::panicking::panic_nounwind::heab7ebe7a6cd845c
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:221:5
  17:        0x10164ddac - core::panicking::panic_cannot_unwind::hedc43d82620205bf
                               at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:309:5
  18:        0x1008ff94c - winit::platform_impl::macos::app_state::ApplicationDelegate::app_did_finish_launching::h26a705a9210618bb
                               at /Users/hhip/.cargo/registry/src/index.crates.io-6f17d22bba15001f/objc2-0.5.2/src/macros/declare_class.rs:981:25
  19:        0x18767d360 - <unknown>
  20:        0x18770e20c - <unknown>
  21:        0x18770e154 - <unknown>
  22:        0x18764bf9c - <unknown>
  23:        0x188805fd4 - <unknown>
  24:        0x18b1f9c78 - <unknown>
  25:        0x18b1f9a28 - <unknown>
  26:        0x18b1f7e88 - <unknown>
  27:        0x18b1f7a90 - <unknown>
  28:        0x18882ec8c - <unknown>
  29:        0x18882ea84 - <unknown>
thread caused non-unwinding panic. aborting.
zsh: abort      cargo run -p whiskers-web-demo

@sagudev sagudev marked this pull request as ready for review December 22, 2024 10:38
@sagudev sagudev requested a review from a team as a code owner December 22, 2024 10:38
@abey79
Copy link

abey79 commented Dec 22, 2024

Repeating my edit above for visibility: on second thought, that message seems wrong. The stride is 8 bytes and the attributes are 32 bytes in total, which is indeed non-compliant (the stride is allowed to be greater, not smaller).

Signed-off-by: sagudev <[email protected]>
@ErichDonGubler ErichDonGubler self-assigned this Dec 23, 2024
@ErichDonGubler ErichDonGubler self-requested a review December 23, 2024 17:28
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, some comments

wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
@sagudev sagudev requested a review from cwfitzgerald December 24, 2024 07:24
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@cwfitzgerald cwfitzgerald merged commit fb17ee8 into gfx-rs:trunk Jan 2, 2025
27 checks passed
cwfitzgerald pushed a commit that referenced this pull request Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate VertexBufferLayout::array_stride
4 participants