Do not give function allocations alignment in consteval and Miri.#144706
Do not give function allocations alignment in consteval and Miri.#144706bors merged 2 commits intorust-lang:masterfrom
Conversation
rustbot
added
the
S-waiting-on-review
rustbot
added
the
T-compiler
This comment has been minimized.
This comment has been minimized.
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
So this essentially reverts #140072... Cc @folkertdev. But I agree, #140072 is just wrong, so we should probably revert and reconsider. |
|
@zachs18 you can just remove that failing Miri test. |
|
r? @RalfJung lacking context (and not sure I want to know about function pointer alignment issues for now :>) |
|
For wasm we already basically decided to limit the maximum alignment to just 1 (the only value that makes sense on that target). It's unfortunate because then any code that uses an alignment higher than 1 is non-portable, but function alignment is a highly specific tool anyway so that should be OK.
I guess that works? Seems a bit hacky but at least const eval won't need to actually know about the pointer tagging. |
|
That's just the one case where we know about this kind of problem though. Who knows what other current or future architectures come up with. We should have a very high bar for such architecture-specific logic in the interpreter and I am not convinced this meets that bar. |
|
Given that this turns out to be a non-trivial question, I think that until we have a clear motivation for turning |
|
The Miri subtree was changed cc @rust-lang/miri |
rustbot
added
S-waiting-on-review
zachs18
changed the title
|
Can't we base this on the Fn vs Fi information in the data layout? That's what LLVM does. |
|
I don't even know what these words mean.^^ But this is a discussion for #140261. It clearly needs some thought whether we want to expose this inside the language. |
|
@RalfJung I'm referring to:
The ARM targets specify |
|
Interesting. So far I don't think rustc parses the LLVM data layout string, or if it does then at least the interpreter doesn't. This seems non-trivial enough that it warrants t-lang involvement IMO, so I added this as an unresolved question in #140261. For now I think a revert is the right call. |
Replace commented-out code with link to context for change. Co-authored-by: Ralf Jung <post@ralfj.de>
|
Thanks! @bors r+ |
bors
added
S-waiting-on-bors
Do not give function allocations alignment in consteval and Miri. We do not yet have a (clear and T-lang approved) design for how `#[align(N)]` on functions should affect function pointers' addresses on various platforms, so for now do not give function pointers alignment in consteval and Miri. ---- Old summary: Not a full solution to <rust-lang#144661>, but fixes the immediate issue by making function allocations all have alignment 1 in consteval, ignoring `#[rustc_align(N)]`, so the compiler doesn't know if any offset other than 0 is non-null. A more "principlied" solution would probably be to make function pointers to `#[instruction_set(arm::t32)]` functions be at offset 1 of an align-`max(2, align attribute)` allocation instead of at offset 0 of their allocation during consteval, and on wasm to either disallow `#[align(N)]` where N > 1, or to pad the function table such that the function pointer of a `#[align(N)]` function is a multiple of `N` at runtime.
Rollup of 13 pull requests Successful merges: - #143857 (Port #[macro_export] to the new attribute parsing infrastructure) - #144070 (Implement `hash_map` macro ) - #144322 (Add lint against dangling pointers from local variables) - #144667 (`AlignmentEnum` should just be `repr(usize)` now) - #144706 (Do not give function allocations alignment in consteval and Miri.) - #144790 (Multiple bounds checking elision failures) - #144794 (Port `#[coroutine]` to the new attribute system) - #144805 (compiletest: Preliminary cleanup of `ProcRes` printing/unwinding) - #144808 (`Interner` arg to `EarlyBinder` does not affect auto traits) - #144816 (Update E0562 to account for the new impl trait positions) - #144822 (Return a struct with named fields from `hash_owner_nodes`) - #144824 (Updated test links in compiler) - #144829 (Use full flag name in strip command for Darwin) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 12 pull requests Successful merges: - #142205 (Mark `slice::swap_with_slice` unstably const) - #144188 (`available_parallelism`: Add documentation for why we don't look at `ulimit`) - #144322 (Add lint against dangling pointers from local variables) - #144497 (tests: Add test for basic line-by-line stepping in a debugger) - #144559 (Enable extract-insert-dyn.rs test on RISC-V (riscv64)) - #144667 (`AlignmentEnum` should just be `repr(usize)` now) - #144706 (Do not give function allocations alignment in consteval and Miri.) - #144746 (resolve: Cleanups and micro-optimizations to extern prelude) - #144785 (Regression test for LLVM error with unsupported expression in static initializer for const pointer in array on macOS.) - #144811 (Stylize `*-lynxos178-*` target maintainer handle to make it easier to copy/paste) - #144848 (For "stage 1" ui-fulldeps, use the stage 1 compiler to query target info) - #144853 (Remove unnecessary `rust_` prefixes) Failed merges: - #144794 (Port `#[coroutine]` to the new attribute system) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #144706 - zachs18:fix-144661, r=RalfJung Do not give function allocations alignment in consteval and Miri. We do not yet have a (clear and T-lang approved) design for how `#[align(N)]` on functions should affect function pointers' addresses on various platforms, so for now do not give function pointers alignment in consteval and Miri. ---- Old summary: Not a full solution to <#144661>, but fixes the immediate issue by making function allocations all have alignment 1 in consteval, ignoring `#[rustc_align(N)]`, so the compiler doesn't know if any offset other than 0 is non-null. A more "principlied" solution would probably be to make function pointers to `#[instruction_set(arm::t32)]` functions be at offset 1 of an align-`max(2, align attribute)` allocation instead of at offset 0 of their allocation during consteval, and on wasm to either disallow `#[align(N)]` where N > 1, or to pad the function table such that the function pointer of a `#[align(N)]` function is a multiple of `N` at runtime.
Rollup of 12 pull requests Successful merges: - rust-lang/rust#142205 (Mark `slice::swap_with_slice` unstably const) - rust-lang/rust#144188 (`available_parallelism`: Add documentation for why we don't look at `ulimit`) - rust-lang/rust#144322 (Add lint against dangling pointers from local variables) - rust-lang/rust#144497 (tests: Add test for basic line-by-line stepping in a debugger) - rust-lang/rust#144559 (Enable extract-insert-dyn.rs test on RISC-V (riscv64)) - rust-lang/rust#144667 (`AlignmentEnum` should just be `repr(usize)` now) - rust-lang/rust#144706 (Do not give function allocations alignment in consteval and Miri.) - rust-lang/rust#144746 (resolve: Cleanups and micro-optimizations to extern prelude) - rust-lang/rust#144785 (Regression test for LLVM error with unsupported expression in static initializer for const pointer in array on macOS.) - rust-lang/rust#144811 (Stylize `*-lynxos178-*` target maintainer handle to make it easier to copy/paste) - rust-lang/rust#144848 (For "stage 1" ui-fulldeps, use the stage 1 compiler to query target info) - rust-lang/rust#144853 (Remove unnecessary `rust_` prefixes) Failed merges: - rust-lang/rust#144794 (Port `#[coroutine]` to the new attribute system) r? `@ghost` `@rustbot` modify labels: rollup
8 participants
We do not yet have a (clear and T-lang approved) design for how
#[align(N)]on functions should affect function pointers' addresses on various platforms, so for now do not give function pointers alignment in consteval and Miri.Old summary:
Not a full solution to #144661, but fixes the immediate issue by making function allocations all have alignment 1 in consteval, ignoring
#[rustc_align(N)], so the compiler doesn't know if any offset other than 0 is non-null.A more "principlied" solution would probably be to make function pointers to
#[instruction_set(arm::t32)]functions be at offset 1 of an align-max(2, align attribute)allocation instead of at offset 0 of their allocation during consteval, and on wasm to either disallow#[align(N)]where N > 1, or to pad the function table such that the function pointer of a#[align(N)]function is a multiple ofNat runtime.