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

miri: protect Move() function arguments during the call #113569

Merged
merged 9 commits into from Jul 12, 2023

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 11, 2023

This gives Move operands a meaning specific to function calls:

  • for the duration of the call, the place the operand comes from is protected, making all read and write accesses insta-UB.
  • the contents of that place are reset to Uninit, so looking at them again after the function returns, we cannot observe their contents

Turns out we can replace the existing "retag return place" hack with the exact same sort of protection on the return place, which is nicely symmetric.

Fixes #112564
Fixes rust-lang/miri#2927

This starts with a Miri rustc-push, since we'd otherwise conflict with a PR that recently landed in Miri.
(The "miri tree borrows" commit is an unrelated cleanup I noticed while doing the PR. I can remove it if you prefer.)
r? @oli-obk

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 11, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 11, 2023

The Miri subtree was changed

cc @rust-lang/miri

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

@bjorn3
Copy link
Member

bjorn3 commented Jul 11, 2023

Other than the pointer identity issue you mentioned I think this is everything we need to justify the current codegen of calls.

///
/// These places need to be protected to make sure the program cannot tell whether the
/// argument/return value was actually copied or passed in-place..
fn protect_in_place_function_argument(
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'm not happy with the name for this machine hook, since it is also used for return places ("output arguments"). Any suggestions?

Also I wonder if I should move the write_uninit out of this hook and into the place where it is called... can't imagine an implementation of this hook that wouldn't want to do write_uninit.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems similarly fragile to have to repeat it at both call sites, so 🤷 I don't have a preference.

Arguments seems fine, it's a bit odd from the high level language perspective, but at the MIR level it all becomes a bit confusing anyway.

@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the miri branch 2 times, most recently from a266010 to 9a665af Compare July 11, 2023 13:45
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

That's some clippy failure which seems entirely unrelated to my PR?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 12, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jul 12, 2023

📌 Commit e7c6db7 has been approved by oli-obk

It is now in the queue for this repository.

@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 Jul 12, 2023
@bors
Copy link
Contributor

bors commented Jul 12, 2023

⌛ Testing commit e7c6db7 with merge 136dab6...

@bors
Copy link
Contributor

bors commented Jul 12, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 136dab6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 12, 2023
@bors bors merged commit 136dab6 into rust-lang:master Jul 12, 2023
12 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Jul 12, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (136dab6): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.8% [0.5%, 1.1%] 13
Regressions ❌
(secondary)
0.9% [0.4%, 1.4%] 11
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.8% [0.5%, 1.1%] 13

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.3% [-4.3%, -1.2%] 8
Improvements ✅
(secondary)
-3.5% [-3.7%, -3.2%] 2
All ❌✅ (primary) -2.3% [-4.3%, -1.2%] 8

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.9% [1.0%, 6.0%] 8
Regressions ❌
(secondary)
2.8% [2.8%, 2.8%] 1
Improvements ✅
(primary)
-2.2% [-2.2%, -2.2%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.3% [-2.2%, 6.0%] 9

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 657.866s -> 657.918s (0.01%)

@rustbot rustbot added the perf-regression Performance regression. label Jul 12, 2023
@Kobzol
Copy link
Contributor

Kobzol commented Jul 12, 2023

This PR took ~3 hours to compile serde_derive on CI, but for some reason it's not recorded in the results 🤔

@RalfJung RalfJung deleted the miri branch July 12, 2023 20:32
@RalfJung
Copy link
Member Author

@Kobzol could that be some CI fluke? Seems unlikely that serde_derive does heavier CTFE things than our ctfe-stress test.

@Kobzol
Copy link
Contributor

Kobzol commented Jul 13, 2023

Probably yes. @lqd tried it locally and didn't experience any timeouts.

@lqd
Copy link
Member

lqd commented Jul 13, 2023

Yeah. It'd be good to keep an eye on the runtime of other PRs, in case something is up with the perf server.

@pnkfelix
Copy link
Member

  • RalfJ is investigating; has potential fix up in PR #113630, ...
  • ... but its not totally certain that PR is a real fix (i.e. the regression may already have been masked or otherwise resolved independently).
  • But meanwhile, I am hypothesizing that the regression reported here is spurious (discussion)
  • marking as triaged

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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
10 participants