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

slice::from_raw_parts returns a different address in const context for u8 #105536

Closed
mxk opened this issue Dec 10, 2022 · 10 comments · Fixed by #115764
Closed

slice::from_raw_parts returns a different address in const context for u8 #105536

mxk opened this issue Dec 10, 2022 · 10 comments · Fixed by #115764
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-slice Area: `[T]` C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mxk
Copy link

mxk commented Dec 10, 2022

Possibly as a result of some u8-specific optimizations, a const &[u8] returned by std::slice::from_raw_parts has a different address than all other uses of the same expression. Forum discussion.

I tried this code:

Playground

use std::ptr::NonNull;
use std::slice::from_raw_parts;

const PTR_U8: *const u8 = NonNull::dangling().as_ptr();
const CONST_U8_REF: &[u8] = unsafe { from_raw_parts(PTR_U8, 0) };
const CONST_U8_PTR: *const u8 = unsafe { from_raw_parts(PTR_U8, 0).as_ptr() };
static STATIC_U8_REF: &[u8] = unsafe { from_raw_parts(PTR_U8, 0) };

const PTR_U16: *const u16 = NonNull::dangling().as_ptr();
const CONST_U16_REF: &[u16] = unsafe { from_raw_parts(PTR_U16, 0) };

const fn const_u8_fn() -> &'static [u8] {
    unsafe { from_raw_parts(PTR_U8, 0) }
}

fn main() {
    let ptr_u8 = unsafe { from_raw_parts(PTR_U8, 0) }.as_ptr();
    let ptr_u16 = unsafe { from_raw_parts(PTR_U16, 0) }.as_ptr();

    assert_eq!(ptr_u8, PTR_U8); // OK
    assert_eq!(ptr_u8, CONST_U8_PTR); // OK
    assert_eq!(ptr_u8, const_u8_fn().as_ptr()); // OK
    assert_eq!(ptr_u8, STATIC_U8_REF.as_ptr()); // OK
    assert_eq!(ptr_u16, CONST_U16_REF.as_ptr()); // OK
    assert_eq!(ptr_u8, CONST_U8_REF.as_ptr()); // ERROR
}

I expected to see this happen: All assertions should pass.

Instead, this happened:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `0x1`,
 right: `0x55f434eaa038`', src/main.rs:25:5

Meta

rustc --version --verbose:

rustc 1.65.0 (897e37553 2022-11-02)
binary: rustc
commit-hash: 897e37553bba8b42751c67658967889d11ecd120
commit-date: 2022-11-02
host: x86_64-pc-windows-msvc
release: 1.65.0
LLVM version: 15.0.0
Backtrace

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `0x1`,
 right: `0x55c4dc25b038`', src/main.rs:25:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:142:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:181:5
   4: playground::main
             at ./[src/main.rs:25](https://play.rust-lang.org/#):5
   5: core::ops::function::FnOnce::call_once
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/ops/function.rs:248:5

@mxk mxk added the C-bug Category: This is a bug. label Dec 10, 2022
@steffahn
Copy link
Member

@rustbot label A-const-eval, T-compiler, A-slice

In the linked URLO discussion, @LegionMammal978 mentioned some part of the compiler that seems relevant for why this behavior only affects u8 slices in particular, the code history is a bit hard to navigate in the git blame, but AFAICT, @oli-obk and @RalfJung are involved.

Maybe they also have more insight into whether this kind of behavior classifies as intended or bug, since there definitely are cases where reference addresses of const-eval results being soft-of “unstable” is intended behavior, albeit surprising behavior to some, but I feel like those cases are usually different in that a reference to some actual static(-promoted) data and memory is created.

@rustbot rustbot added A-const-eval Area: Constant evaluation (MIR interpretation) A-slice Area: `[T]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 10, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Dec 10, 2022

It's not promotion related: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b89aaee74c5da29370ba7107576adc7a

So I'm guessing some sort of codegen issue, because otherwise turning the reference back to a pointer in const land would cause the same issue, but it doesn't: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6ac87b271a9b495b6497015d95b57815

@scottmcm
Copy link
Member

Minimized, and emphasizing the same thing oli just showed about this being related to the const item somehow, not a problem in MIR interpreting itself:

assert_eq!(
    // as_ptr INside the `const{}`
    (const { (unsafe { std::slice::from_raw_parts(3 as *const u8, 0) }).as_ptr() }),
    std::ptr::invalid(3),
); // PASSES
assert_eq!(
    // as_ptr OUTside the `const{}`
    (const { (unsafe { std::slice::from_raw_parts(7 as *const u8, 0) }) }).as_ptr(),
    std::ptr::invalid(7),
); // FAILS, 0x56229d3aa00b != 0x7

https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=5544bc2edee6a4276a57cbeedbb12212

@LegionMammal978
Copy link
Contributor

LegionMammal978 commented Dec 11, 2022

It seems like the whole point of op_to_const() is to make this transformation, which occurs a bit further down in the function:

                debug!("ScalarPair(a: {:?}, b: {:?})", a, b);
                // We know `offset` is relative to the allocation, so we can use `into_parts`.
                let (data, start) = match a.to_pointer(ecx).unwrap().into_parts() {
                    (Some(alloc_id), offset) => {
                        (ecx.tcx.global_alloc(alloc_id).unwrap_memory(), offset.bytes())
                    }
                    (None, _offset) => (
                        ecx.tcx.intern_const_alloc(Allocation::from_bytes_byte_aligned_immutable(
                            b"" as &[u8],
                        )),
                        0,
                    ),
                };
                let len = b.to_machine_usize(ecx).unwrap();
                let start = start.try_into().unwrap();
                let len: usize = len.try_into().unwrap();
                ConstValue::Slice { data, start, end: start + len }

That is, a &[u8] or &str without an AllocId (e.g., one from an invalid pointer) is converted to an arbitrary allocation storing b"" as &[u8]. Looking up the call graph, this operation appears to be invoked by eval_to_const_value_raw() and valtree_to_const_value().

So the reference isn't transformed every time it's used, only in certain contexts, where the slice has to exit const-eval-land and become a real instantiated value. For instance, using a slice as a const-generic parameter triggers the same behavior (Playground):

#![allow(incomplete_features)]
#![feature(adt_const_params)]
use std::slice::from_raw_parts;

struct AsPtr<const SLICE: &'static [u8]>;
impl<const SLICE: &'static [u8]> AsPtr<SLICE> {
    const PTR: *const u8 = SLICE.as_ptr();
}

fn main() {
    let ptr = AsPtr::<{ unsafe { from_raw_parts(1 as *const u8, 0) } }>::PTR;
    println!("{ptr:p}");
}

Notably, this only occurs when the slice is the entire constant value. The address isn't modified if the slice is a field of a struct, enum, or array.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 11, 2022

Considering that we got valtrees now, this will become more common. All addresses in type level constants are now de- or duplicated.

For these ones that are not used in type level consts, we should just remove the ConstValue::Slice variant, as it is not needed anymore except potentially for perf, but we may be able to preserve that

@RalfJung
Copy link
Member

RalfJung commented Dec 11, 2022

op_to_const is a big hack and the sooner we can get rid of it the better...

For the specific case of u8 slices, IIRC that was needed because one can pattern-match on them. How close is the pattern matching logic to being ported away from ConstValue, to valtrees? Once that is the case, some cleanup should be possible here.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 14, 2023

this issue is mitigated by #115764, and we can probably fix it entirely based on that change

@RalfJung
Copy link
Member

I think that PR should already fix this issue completely?

ConstValue::Slice is then only created for string literals and when turning valtrees into constants, none of which should happen in the above code.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 14, 2023

sweet. wanna add a regression test to the PR, too?

@RalfJung
Copy link
Member

Sure, will do.

@mxk note that we do not guarantee that these pointers will always be equal. We always reserve the right to duplicate the values of const.

But in this case the inequality was a symptom of the compiler doing some unnecessary work, and that's why it got fixed.

@bors bors closed this as completed in d97e04f Sep 14, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 15, 2023
some ConstValue refactoring

In particular, use AllocId instead of Allocation in ConstValue::ByRef. This helps avoid redundant AllocIds when a  `ByRef` constant gets put back into the interpreter.

r? `@oli-obk`

Fixes rust-lang/rust#105536
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-slice Area: `[T]` C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants