Skip to content
This repository has been archived by the owner on Jan 29, 2025. It is now read-only.

Msl bounds checks #1532

Merged
merged 7 commits into from
Dec 6, 2021
Merged

Msl bounds checks #1532

merged 7 commits into from
Dec 6, 2021

Conversation

jimblandy
Copy link
Member

@jimblandy jimblandy commented Nov 15, 2021

Fixes #1531.

Remaining work:

  • fix macOS validation errors
  • Don't forget cast to unsigned in bounds checks
  • Generate MSL for policy-mix.wgsl too
  • handle AccessIndex on dynamic arrays
  • bother to check policy for each index in analyze_function
    • consider caching policy decisions in IndexInfo?
  • make choose_checks reuse a Vec to save allocation
  • (leave for followup) Unify GuardedIndex and back::spv's MaybeKnown
  • produce zero values for non-scalar loads

@jimblandy
Copy link
Member Author

Because GitHub doesn't understand dependent PRs, this just incorporates a bunch of commits that I think could be reviewed and landed separately. I'll sort these out tomorrow.

@jimblandy jimblandy force-pushed the msl-bounds-checks branch 3 times, most recently from c166082 to 33fde28 Compare November 16, 2021 07:06
src/back/msl/writer.rs Outdated Show resolved Hide resolved
src/back/msl/writer.rs Show resolved Hide resolved
src/back/msl/writer.rs Outdated Show resolved Hide resolved
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Looks great!

src/back/msl/writer.rs Outdated Show resolved Hide resolved
src/back/msl/writer.rs Show resolved Hide resolved
src/back/msl/writer.rs Outdated Show resolved Hide resolved
writeln!(self.out, ")")?;
self.put_unchecked_store(pointer, value, level.next(), context)
} else {
self.put_unchecked_store(pointer, value, level, context)
Copy link
Member

Choose a reason for hiding this comment

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

nit: we could move this out of if/else

Copy link
Member

Choose a reason for hiding this comment

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

I'm a tiny bit concerned if at some point put_unchecked_store implementation may decide to become multiple statements. Doing this in the "if (xx) y = z;" style will no longer work. Maybe we should anticipate this and use "{"/"}"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed this to always put the store in brackets. That did make it awkward to move the put_unchecked_store out of the if. Since this was "nit", I didn't worry about it - but if you think it would still look better to hoist out that call (and add plumbing to indent it properly), we can totally do that.

src/proc/index.rs Outdated Show resolved Hide resolved
src/proc/index.rs Outdated Show resolved Hide resolved
@jimblandy jimblandy force-pushed the msl-bounds-checks branch 5 times, most recently from 379a012 to 4baffc8 Compare December 3, 2021 06:15
@jimblandy jimblandy marked this pull request as ready for review December 3, 2021 06:20
@jimblandy
Copy link
Member Author

Rebased, and a lot simpler.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Almost ready to go, amazing work!
Just a few last notes

src/back/msl/writer.rs Show resolved Hide resolved
src/back/msl/writer.rs Outdated Show resolved Hide resolved
tests/snapshots.rs Outdated Show resolved Hide resolved
tests/snapshots.rs Show resolved Hide resolved
tests/in/bounds-check-restrict.wgsl Show resolved Hide resolved
src/back/msl/writer.rs Outdated Show resolved Hide resolved
@jimblandy jimblandy requested a review from kvark December 4, 2021 00:44
Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

A few uber-nits left, almost good to go!

src/back/msl/writer.rs Show resolved Hide resolved
src/back/msl/writer.rs Show resolved Hide resolved
src/back/msl/writer.rs Outdated Show resolved Hide resolved
src/proc/mod.rs Outdated Show resolved Hide resolved
@kvark kvark merged commit f51f468 into gfx-rs:master Dec 6, 2021
@jimblandy jimblandy deleted the msl-bounds-checks branch February 15, 2022 23:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metal output needs index bounds checks
2 participants