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.

Sign up for GitHub

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

Jump to bottom

Test and reject out-of-bounds shuffle vectors #76768

Merged
merged 3 commits into from
Oct 4, 2020
Merged

Test and reject out-of-bounds shuffle vectors #76768

merged 3 commits into from
Oct 4, 2020

Conversation

workingjubilee
Copy link
Member

Fixes #73542.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2020
@jyn514 jyn514 added A-simd Area: SIMD (Single Instruction Multiple Data) A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 16, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 16, 2020

r? @RalfJung

@RalfJung
Copy link
Member

Thanks, this looks good!

Is simd_shuffle4 the only intrinsic that needs such a check?

@workingjubilee
Copy link
Member Author

workingjubilee commented Sep 16, 2020 •
edited
Loading

Well, I can macro the test over the various other LLVM shuffle intrinsics. It didn't seem immediately necessary because they would all follow the pattern, so I figured any old canary would tell us how it is in the coalmine. But there are actually a few other tests for this floating in the compiler that verify this error code is emitted (since it basically exists for this issue). As far as I can tell, it would be only the shuffles, and maybe insert/extract, that could apply out-of-bounds logic in the first place.

@RalfJung
Copy link
Member

Yeah, would be good to have all intrinsic bounds checks covered here. Canaries are great but without knowing how the check works internally it is easy to think one has covered all code paths but forget about some odd special case...

When I grep for that error code, most existing tests seem to be about other aspects? In fact I can see none that says anything about "out of bounds". The test should cover all intrinsics that can have that particular error. The error code is for all sorts of problems with intrinsics.

@workingjubilee
Copy link
Member Author

That's completely fair, will generalize this fully then (and figure out if insert/extract can go OOB... if they can, I'll include them).

@RalfJung RalfJung added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2020
@RalfJung
Copy link
Member

That's completely fair, will generalize this fully then (and figure out if insert/extract can go OOB... if they can, I'll include them).

@workingjubilee From what I can see, this is still outstanding.

@workingjubilee
Copy link
Member Author

Indeed it was!

@rustbot modify labels: -S-waiting-on-author, +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2020
@workingjubilee
Copy link
Member Author

Force pushes were tidy thrash, whoops.

@RalfJung
Copy link
Member

RalfJung commented Oct 3, 2020

r=me with or without //~|.

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2020
@workingjubilee
Copy link
Member Author

@bors r=RalfJung
does this work? :^)
@rustbot modify labels: -S-waiting-on-author, +S-waiting-on-review if not

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2020

Error: Label if can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

@bors
Copy link
Contributor

bors commented Oct 3, 2020

@workingjubilee: 🔑 Insufficient privileges: Not in reviewers

@workingjubilee
Copy link
Member Author

RIP me. 💦

@jyn514
Copy link
Member

jyn514 commented Oct 3, 2020

@bors r=ralfjung rollup

@bors
Copy link
Contributor

bors commented Oct 3, 2020

📌 Commit 2fcd183 has been approved by ralfjung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 3, 2020
@RalfJung
Copy link
Member

RalfJung commented Oct 3, 2020

RIP me. sweat_drops

Sorry -- next time I'll let bors know that you can do this. :)

@JohnTitor JohnTitor mentioned this pull request Oct 4, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 4, 2020
@bors
Auto merge of rust-lang#77517 - JohnTitor:rollup-msbd49e, r=JohnTitor
2251766 
Rollup of 11 pull requests

Successful merges:

 - rust-lang#75143 (Use `tracing` spans to trace the entire MIR interp stack)
 - rust-lang#75699 (Uplift drop-bounds lint from clippy)
 - rust-lang#76768 (Test and reject out-of-bounds shuffle vectors)
 - rust-lang#77190 (updated p! macro to accept literals)
 - rust-lang#77388 (Add some regression tests)
 - rust-lang#77419 (Create E0777 error code for invalid argument in derive)
 - rust-lang#77447 (BTreeMap: document DrainFilterInner better)
 - rust-lang#77468 (Fix test name)
 - rust-lang#77469 (Improve rustdoc error for failed intra-doc link resolution)
 - rust-lang#77473 (Make --all-targets in x.py check opt-in)
 - rust-lang#77508 (Fix capitalization in blog post name)

Failed merges:

r? `@ghost`
@bors bors merged commit 65e4488 into rust-lang:master Oct 4, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 4, 2020
@workingjubilee workingjubilee mentioned this pull request Feb 5, 2021
Merged
@workingjubilee workingjubilee deleted the reject-oob-shuffles branch April 23, 2021 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

Labels
A-simd Area: SIMD (Single Instruction Multiple Data) A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.49.0
Development

Successfully merging this pull request may close these issues.

Reject OOB shufflevector intrinsics
7 participants
@workingjubilee @rust-highfive @jyn514 @RalfJung @rustbot @bors @Mark-Simulacrum