-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Check alignment of raw pointers in debug mode #54915
Comments
See also Zig, that exposes such alignment information in the type system itself: Using syntax like:
The run-time check perf impact is probably acceptable because currently debug builds are often 1000%+ slower. I suggest to add a switch like -Z force-overflow-checks that could be switched on in "efficient experiment" release builds too if desired, because sometimes debug builds are so slow you can't use them well to test some code. More generally, what other basic invariants like that could rustc verify automatically (in debug builds)? |
Good point. We have something similar for integer overflow, don't we?
Tons of them, but which have a good perf-benefit trade-off? Also note that this wouldn't reliably verify the alignment invariant, one can still Some other things coming to my mind are:
For the last (or the last two) to be viable, however, we need a libstd compiled with debug assertion to be used when compiling programs in debug mode. Everything else I can think of involves some kind of type-based recursion on e.g. the return value of Finally, we'd probably have to do some communication work to get people to run their test suites in debug mode, so that this actually reaches all the projects out there. Is there any way to get debug assertions, but also get optimizations, so that this doesn't make everything quite so much slower? It does seem useful to me to have code with extra checks optimized; that means the compiler can assume fewer things (less UB) but it can still e.g. optimize away the checks if it can prove they always succeed, and it can do all the other optimizations (inlining and whatnot). |
Sure enough. |
Ah, nice! And that will also affect integer overflow checks? |
Integer overflow checks will be on (they are controlled by |
With Cargo, the flags are placed in [profile.*] sections. The following are two of the defaults for dev builds # The development profile, used for `cargo build`.
[profile.dev]
opt-level = 0 # controls the `--opt-level` the compiler builds with.
# 0-1 is good for debugging. 2 is well-optimized. Max is 3.
# 's' attempts to reduce size, 'z' reduces size even more.
debug-assertions = true # controls whether debug assertions are enabled
# (e.g. debug_assert!() and arithmetic overflow checks)
Fortunately, the default travis-ci rust set-up runs |
Cc #51713 |
Insert alignment checks for pointer dereferences when debug assertions are enabled Closes rust-lang/rust#54915 - [x] Jake tells me this sounds like a place to use `MirPatch`, but I can't figure out how to insert a new basic block with a new terminator in the middle of an existing basic block, using `MirPatch`. (if nobody else backs up this point I'm checking this as "not actually a good idea" because the code looks pretty clean to me after rearranging it a bit) - [x] Using `CastKind::PointerExposeAddress` is definitely wrong, we don't want to expose. Calling a function to get the pointer address seems quite excessive. ~I'll see if I can add a new `CastKind`.~ `CastKind::Transmute` to the rescue! - [x] Implement a more helpful panic message like slice bounds checking. r? `@oli-obk`
Insert alignment checks for pointer dereferences when debug assertions are enabled Closes rust-lang/rust#54915 - [x] Jake tells me this sounds like a place to use `MirPatch`, but I can't figure out how to insert a new basic block with a new terminator in the middle of an existing basic block, using `MirPatch`. (if nobody else backs up this point I'm checking this as "not actually a good idea" because the code looks pretty clean to me after rearranging it a bit) - [x] Using `CastKind::PointerExposeAddress` is definitely wrong, we don't want to expose. Calling a function to get the pointer address seems quite excessive. ~I'll see if I can add a new `CastKind`.~ `CastKind::Transmute` to the rescue! - [x] Implement a more helpful panic message like slice bounds checking. r? `@oli-obk`
Insert alignment checks for pointer dereferences when debug assertions are enabled Closes rust-lang/rust#54915 - [x] Jake tells me this sounds like a place to use `MirPatch`, but I can't figure out how to insert a new basic block with a new terminator in the middle of an existing basic block, using `MirPatch`. (if nobody else backs up this point I'm checking this as "not actually a good idea" because the code looks pretty clean to me after rearranging it a bit) - [x] Using `CastKind::PointerExposeAddress` is definitely wrong, we don't want to expose. Calling a function to get the pointer address seems quite excessive. ~I'll see if I can add a new `CastKind`.~ `CastKind::Transmute` to the rescue! - [x] Implement a more helpful panic message like slice bounds checking. r? `@oli-obk`
Insert alignment checks for pointer dereferences when debug assertions are enabled Closes rust-lang/rust#54915 - [x] Jake tells me this sounds like a place to use `MirPatch`, but I can't figure out how to insert a new basic block with a new terminator in the middle of an existing basic block, using `MirPatch`. (if nobody else backs up this point I'm checking this as "not actually a good idea" because the code looks pretty clean to me after rearranging it a bit) - [x] Using `CastKind::PointerExposeAddress` is definitely wrong, we don't want to expose. Calling a function to get the pointer address seems quite excessive. ~I'll see if I can add a new `CastKind`.~ `CastKind::Transmute` to the rescue! - [x] Implement a more helpful panic message like slice bounds checking. r? `@oli-obk`
It is UB to
*r
a raw pointer ("dereference", but this includes&*r
) when that pointer is not aligned to its type. I propose that when compiling in debug mode, we insert a check for this to emit a panic when the pointer is not sufficiently aligned. That would essentially be the "big brother" of #52972, and the cousin of #53871.This would have caught #54908.
However, I guess before someone starts implementing this, we should get @rust-lang/compiler to say that they think this is a good idea. It's in debug mode only, and it's a rather cheap check (as alignment is always a power of two), so I expect the perf impact to be acceptable.
The text was updated successfully, but these errors were encountered: