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

Check for inline assembly in THIR unsafeck #85259

Merged
merged 2 commits into from
May 16, 2021
Merged

Check for inline assembly in THIR unsafeck #85259

merged 2 commits into from
May 16, 2021

Conversation

syvb
Copy link
Contributor

@syvb syvb commented May 13, 2021 •
edited
Loading

#83129 was merged recently and added a THIR unsafe checker. This adds a check for inline assembly. (and this is 2x simpler than the MIR version, which has to check for asm and llvm_asm in two separate spots!)

see also rust-lang/project-thir-unsafeck#7

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 May 13, 2021
@syvb syvb mentioned this pull request May 13, 2021
Merged
LeSeulArtichaut
LeSeulArtichaut reviewed May 13, 2021
View reviewed changes
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut left a comment

Choose a reason for hiding this comment

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

To my untrained eye this looks good.

Maybe add a thir revision to the existing UI tests for inline assembly, if any?

@LeSeulArtichaut LeSeulArtichaut mentioned this pull request May 13, 2021
Closed
13 tasks
LeSeulArtichaut
LeSeulArtichaut approved these changes May 13, 2021
View reviewed changes
Copy link
Contributor

@LeSeulArtichaut LeSeulArtichaut left a comment

Choose a reason for hiding this comment

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

Looks like inline assembly being unsafe is never tested in the suite, seems weird to me. Anyway I think this could be merged. I'm not a reviewer though, so let's ask for signoff from r? @nikomatsakis

@syvb
Copy link
Contributor Author

syvb commented May 13, 2021

@LeSeulArtichaut yep, I couldn't find any testing of inline assembly either. I added some thirunsafeck revisions to a few assembly tests that use unsafe properly though.

@nikomatsakis
Copy link
Contributor

@bors r+

This looks good!

@bors
Copy link
Contributor

bors commented May 13, 2021

📌 Commit 3e6ec4eeacf92d7a9dec11a6a8506dac5d9797b3 has been approved by nikomatsakis

@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-review Status: Awaiting review from the assignee but also interested parties. labels May 13, 2021
@bors
Copy link
Contributor

bors commented May 14, 2021

☔ The latest upstream changes (presumably #84107) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 14, 2021
@syvb syvb force-pushed the thir-unsafeck-inline-asm branch from 3e6ec4e to f23d231 Compare May 14, 2021 13:23
@syvb
Copy link
Contributor Author

syvb commented May 14, 2021

Merge conflicts resolved.

r? @nikomatsakis

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 15, 2021

📌 Commit f23d231 has been approved by nikomatsakis

@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 May 15, 2021
@bors
Copy link
Contributor

bors commented May 16, 2021

⌛ Testing commit f23d231 with merge 6d525d5...

@bors
Copy link
Contributor

bors commented May 16, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 6d525d5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 16, 2021
@bors bors merged commit 6d525d5 into rust-lang:master May 16, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 9, 2021
@bors
Auto merge of rust-lang#85263 - Smittyvb:thir-unsafeck-union-field, r…
240ff4c 
…=oli-obk

Check for union field accesses in THIR unsafeck

see also rust-lang#85259, rust-lang#83129, rust-lang/project-thir-unsafeck#7

r? `@LeSeulArtichaut`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

@LeSeulArtichaut LeSeulArtichaut LeSeulArtichaut approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.54.0
Development

Successfully merging this pull request may close these issues.
7 participants
@syvb @rust-highfive @nikomatsakis @bors @LeSeulArtichaut @davidtwco @rustbot