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

Move alignment checks to codegen #117473

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Nov 1, 2023

Implementing UB checks entirely in a MIR transform is quite limiting. Since MIR transforms work on polymorphic MIR we don't know for sure what all our types are, and sometimes we just have to give up on inserting a check. For example we used to emit MIR to compute the alignment mask at runtime, because the pointee type could be generic. and we used to skip alignment checks where we weren't sure the pointee was sized. Implementing the checks in codegen frees us from those problems, because we get to deal with monomorphized types.

Initially I implemented this by stripping down the MIR pass to insert a new terminator, which codegen would lower to a check if it saw fit. That's the perf run that has no regression: #117473 (comment). Since then, I've decided that the better strategy is to do this entirely in codegen. Only touching codegen dramatically reduces the amount of code in the compiler that this needs to touch, and it means we will insert checks into functions from the standard library which get codegenned in a crate compiled with debug assertions. Previously, *misaligned_ptr would be checked, but misaligned_ptr.read() would not. With this PR, now it is. With this PR, we get checks in ptr::read. That's this perf run: #117473 (comment)

The only thing that jumps out at me about this codegen change is that between any two statements, codegen can change which backend block it is generating code for without changing the current MIR block. We already do insert blocks on the fly for panics, but in that case we don't stay in the new block.


I'm writing this with the expectation that I implement the niche checks in the same manner, because they have the same problem with polymorphic MIR, possibly worse.

I did a GitHub code search and the only users of the old opt-out which was -Zmir-enable-passes=-CheckAlignment were turning it off because of the problem with i686-pc-windows-msvc, but that shouldn't be a problem anymore because we don't emit alignment checks on that target. Note that -Zmir-enable-passes=-CheckAlignment will silently stop doing anything. We never check that the passes given to -Zmir-enable-passes actually match the names of any actual passes.

If the new checks cause issues, users now have the opt-out from #123411: -Zub-checks=no.

@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 Nov 1, 2023
@saethlin saethlin added the T-opsem Relevant to the opsem team label Nov 1, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@saethlin
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 12, 2023
@bors
Copy link
Contributor

bors commented Nov 12, 2023

⌛ Trying commit 165048a with merge 8d257b9...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 12, 2023
…=<try>

Move alignment checks to codegen

Implementing UB checks entirely in a MIR transform is quite limiting, we don't know for sure what all our types are so we need to make a lot of sacrifices. For example in here we used to emit MIR to compute the alignment mask at runtime, because the pointee type could be generic. Implementing the checks in codegen frees us from that requirement, because we get to deal with monomorphized types.

But I don't think we can move these checks entirely into codegen, because inserting the check needs to insert a new terminator into a basic block, which splits the previous basic block into two. We can't add control flow like this in codegen, but we can in MIR.

So now the MIR transform just inserts a `TerminatorKind::UbCheck` which is effectively a `Goto` that also reads an `Operand` (because it either goes to the target block or terminates), and codegen expands that new terminator into the actual check.

---

Also I'm writing this with the expectation that I implement the niche checks in the same manner, because they have the same problem with polymorphic MIR, possibly worse.

r? `@ghost`
@bors
Copy link
Contributor

bors commented Nov 12, 2023

☀️ Try build successful - checks-actions
Build commit: 8d257b9 (8d257b961e0625aa5abc7146a6ee857f6182c524)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8d257b9): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

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)
2.4% [0.4%, 4.4%] 2
Regressions ❌
(secondary)
1.8% [0.7%, 4.2%] 4
Improvements ✅
(primary)
-1.7% [-4.1%, -0.0%] 4
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.3% [-4.1%, 4.4%] 6

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)
0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-2.7%, -2.1%] 4
All ❌✅ (primary) 0.5% [0.5%, 0.5%] 1

Binary size

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.1% [0.0%, 0.5%] 10
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.0% [-0.1%, -0.0%] 8
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.0% [-0.1%, 0.5%] 18

Bootstrap: 674.671s -> 673.178s (-0.22%)
Artifact size: 311.12 MiB -> 311.05 MiB (-0.02%)

@saethlin
Copy link
Member Author

saethlin commented Apr 7, 2024

r? oli-obk

@saethlin saethlin marked this pull request as ready for review April 7, 2024 04:31
@rustbot
Copy link
Collaborator

rustbot commented Apr 7, 2024

This PR changes MIR

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

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@@ -246,7 +246,6 @@ pub enum AssertMessage {
RemainderByZero(Operand),
ResumedAfterReturn(CoroutineKind),
ResumedAfterPanic(CoroutineKind),
MisalignedPointerDereference { required: Operand, found: Operand },
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please just mark this as deprecated for now instead or removing it? Thanks

@bors
Copy link
Contributor

bors commented May 10, 2024

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

@celinval
Copy link
Contributor

Has anyone considered creating MIR passes on monomorphic MIR? I see a pattern of pushing things to codegen that should really be implemented as instrumentation passes. The code generator shouldn't be creating new basic blocks.

@saethlin
Copy link
Member Author

Has anyone considered creating MIR passes on monomorphic MIR?

Yes. Many times. Nobody is happy with the amount of cleverness in codegen.

MIR is monomorphized on-the-fly as an optimization, because otherwise we'd have to clone all MIR bodies at codegen so that we can mutate them. Or we could probably have a really complicated accessor for the MIR like MirPatch? MirPatch doesn't support most of the operations I've done in codegen, and it doesn't fold in access to the patched state.

@rust-log-analyzer

This comment has been minimized.

@celinval
Copy link
Contributor

Do you know what the overhead would be if we clone the bodies lazily, just for functions that need transformation? For example, I'm assuming these checks would only be required in functions that perform unsafe operations.

@saethlin
Copy link
Member Author

Do you know what the overhead would be if we clone the bodies lazily, just for functions that need transformation? For example, I'm assuming these checks would only be required in functions that perform unsafe operations.

I think you have a bit of an optimistic view of the situation based on only looking at the changes in this PR. Consider also #121174. And also, if we had such a change I would like to use it to do SimplifyCfg on monomorphic MIR, to clean up the result of this traversal strategy:

let reachable_blocks = traversal::mono_reachable_as_bitset(mir, cx.tcx(), instance);
// Codegen the body of each block using reverse postorder
for (bb, _) in traversal::reverse_postorder(mir) {
if reachable_blocks.contains(bb) {
fx.codegen_block(bb);
} else {
// We want to skip this block, because it's not reachable. But we still create
// the block so terminators in other blocks can reference it.
fx.codegen_block_as_unreachable(bb);
}
}

I know from looking at the IR we produce that the mono-reachable traversal produces goto chains. Like everything else here, that optimization is possible to implement in a lazy fashion without some MIR to mutate, but it would be complicated.

@saethlin
Copy link
Member Author

Based on what I'm seeing in #125025, maybe cloning all the MIR is not too expensive.

@celinval
Copy link
Contributor

Based on what I'm seeing in #125025, maybe cloning all the MIR is not too expensive.

That's similar to our findings when we migrated to using StableMIR in Kani. StableMIR supports monomorphic bodies for instances.

@RalfJung
Copy link
Member

FWIW, MIR also supports monomorphic bodies -- in the MIR-to-MiniRust translation, we monomorphize the entire MIR body before translating it.

@celinval
Copy link
Contributor

FWIW, MIR also supports monomorphic bodies -- in the MIR-to-MiniRust translation, we monomorphize the entire MIR body before translating it.

Yes, that's how StableMIR is implemented too, but I believe you still need to clone the body.


pub fn pointers_to_check<F>(
statement: &mir::Statement<'_>,
required_align_of: F,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the API design here so that a large part of the logic lies with the caller? Is this expected to be used differently from different places in the future? Or was it just to avoid passing self?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 28, 2024

Please add a codegen test that shows how we now also have alignment checks in generic code that didn't have them before

@bors
Copy link
Contributor

bors commented Jul 20, 2024

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

@saethlin saethlin marked this pull request as draft July 21, 2024 18:51
@saethlin
Copy link
Member Author

It turns out that since we're only checking that reads and writes are done to a place based on an aligned pointer, we actually don't run into the unsized pointee case anymore. Previously this pass was designed to check all derefs, not just reads and writes.

That means that some of the pass logic can be cleaned up, though it still needs a carve-out for unsized locals.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 24, 2024

r? mir-opt I'm going on leave next week

@rustbot rustbot assigned wesleywiser and unassigned oli-obk Jul 24, 2024
@Dylan-DPC Dylan-DPC 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 Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-opsem Relevant to the opsem team
Projects
None yet
Development

Successfully merging this pull request may close these issues.