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

Introduce the beginning of a THIR unsafety checker #83129

Merged
merged 5 commits into from
May 13, 2021
Merged

Introduce the beginning of a THIR unsafety checker #83129

merged 5 commits into from
May 13, 2021

Conversation

LeSeulArtichaut
Copy link
Contributor

This poses the foundations for the THIR unsafety checker, so that it can be implemented incrementally:

  • implements a rudimentary Visitor for the THIR (which will definitely need some tweaking in the future)
  • introduces a new -Zthir-unsafeck flag which tells the compiler to use THIR unsafeck instead of MIR unsafeck
  • implements detection of unsafe functions
  • adds revisions to the UI tests to test THIR unsafeck alongside MIR unsafeck

This uses a very simple query design, where bodies are unsafety-checked on a body per body basis. This however has some big flaws:

  • the unsafety-checker builds the THIR itself, which means a lot of work is duplicated with MIR building constructing its own copy of the THIR
  • unsafety-checking closures is currently completely wrong: closures should take into account the "safety context" in which they are created, here we are considering that closures are always a safe context

I had intended to fix these problems in follow-up PRs since they are always gated under the -Zthir-unsafeck flag (which is explicitely noted to be unsound).

r? @nikomatsakis
cc rust-lang/project-thir-unsafeck#3 rust-lang/project-thir-unsafeck#7

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 14, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@LeSeulArtichaut
Copy link
Contributor Author

Wow, you can't use TODOs in the compiler :D

@LeSeulArtichaut LeSeulArtichaut mentioned this pull request Mar 14, 2021
Closed
13 tasks
@bstrie bstrie added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2021
nikomatsakis
nikomatsakis reviewed Apr 1, 2021
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Ack! Just saw this. Will review Asap =)

nikomatsakis
nikomatsakis requested changes Apr 3, 2021
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This looks like a great start! I left a few nits.

compiler/rustc_mir_build/src/check_unsafety.rs Outdated Show resolved Hide resolved
compiler/rustc_mir_build/src/thir/visit.rs Outdated Show resolved Hide resolved
@LeSeulArtichaut
Copy link
Contributor Author

LeSeulArtichaut commented Apr 3, 2021 •
edited
Loading

I implemented the changes you requested.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
nikomatsakis
nikomatsakis requested changes May 12, 2021
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This looks great, @LeSeulArtichaut! See my comment below.

compiler/rustc_interface/src/passes.rs Show resolved Hide resolved
@nikomatsakis nikomatsakis 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 May 12, 2021
nikomatsakis
nikomatsakis approved these changes May 13, 2021
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

My bad! Everything looks good.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 13, 2021

📌 Commit 985fb4c 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 13, 2021
@bors
Copy link
Contributor

bors commented May 13, 2021

⌛ Testing commit 985fb4c with merge 17b60b8...

@bors
Copy link
Contributor

bors commented May 13, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 17b60b8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 13, 2021
@bors bors merged commit 17b60b8 into rust-lang:master May 13, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 13, 2021
@syvb syvb mentioned this pull request May 13, 2021
Merged
@LeSeulArtichaut LeSeulArtichaut deleted the thir-unsafeck branch May 13, 2021 14:45
@syvb syvb mentioned this pull request May 13, 2021
Merged
@LeSeulArtichaut LeSeulArtichaut mentioned this pull request May 13, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 16, 2021
@bors
Auto merge of rust-lang#85259 - Smittyvb:thir-unsafeck-inline-asm, r=…
6d525d5 
…nikomatsakis

Check for inline assembly in THIR unsafeck

rust-lang#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
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

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.

None yet
7 participants
@LeSeulArtichaut @rust-log-analyzer @bors @nikomatsakis @bstrie @rust-highfive @rustbot