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

MaybeUninit::zeroed isn't actually zeroed #111616

Closed
lukas-code opened this issue May 15, 2023 · 5 comments
Closed

MaybeUninit::zeroed isn't actually zeroed #111616

lukas-code opened this issue May 15, 2023 · 5 comments
Labels
A-codegen Area: Code generation C-bug Category: This is a bug.

Comments

@lukas-code
Copy link
Member

Found while looking at #111608.

I tried this code: playground

use std::mem::MaybeUninit;
use std::ptr;

pub unsafe fn write_uninit(x: *mut MaybeUninit<(u8, u16)>) {
    let zeroed = MaybeUninit::<(u8, u16)>::zeroed();
    *x = zeroed;
}

fn main() {
    let mut bytes = [1u8; 4];
    unsafe {
        write_uninit(ptr::addr_of_mut!(bytes).cast());
    }
    println!("{bytes:?}");
}

I expected to see this happen: outputs [0, 0, 0, 0]

Instead, this happened: outputs [0, 1, 0, 0]

MaybeUninit<T> should behave like an untyped bag of bytes that happens to have the same size and align as T. In particular, it should not have padding.

This affects debug and release mode.

The emitted LLVM IR with -C opt-level=3 -C no-prepopulate-passes looks like this, so this is on rustc and not on LLVM: goldbolt

define void @_ZN7example12write_uninit17hac03fe9cd267adb4E(ptr noundef %x) unnamed_addr #0 !dbg !6 {
start:
  %u = alloca { i8, i16 }, align 2
  call void @llvm.lifetime.start.p0(i64 4, ptr %u), !dbg !11
  call void @llvm.memset.p0.i64(ptr align 2 %u, i8 0, i64 4, i1 false), !dbg !19
  %0 = getelementptr inbounds { i8, i16 }, ptr %u, i32 0, i32 0, !dbg !34
  %zeroed.0 = load i8, ptr %0, align 2, !dbg !34
  %1 = getelementptr inbounds { i8, i16 }, ptr %u, i32 0, i32 1, !dbg !34
  %zeroed.1 = load i16, ptr %1, align 2, !dbg !34
  call void @llvm.lifetime.end.p0(i64 4, ptr %u), !dbg !35
  %2 = getelementptr inbounds { i8, i16 }, ptr %x, i32 0, i32 0, !dbg !36
  store i8 %zeroed.0, ptr %2, align 2, !dbg !36
  %3 = getelementptr inbounds { i8, i16 }, ptr %x, i32 0, i32 1, !dbg !36
  store i16 %zeroed.1, ptr %3, align 2, !dbg !36
  ret void, !dbg !38
}

Meta

rustc --version --verbose:

Playground 1.71.0-nightly (2023-05-14 18bfe5d8a9ca0e226171), presumably affects all versions

@rustbot label A-codegen

@lukas-code lukas-code added the C-bug Category: This is a bug. label May 15, 2023
@rustbot rustbot added the A-codegen Area: Code generation label May 15, 2023
@lukas-code
Copy link
Member Author

Another example:

use std::mem::MaybeUninit;

pub fn foo(thing: MaybeUninit<(u8, u16)>) {
    let bytes: [u8; 4] = unsafe { core::mem::transmute(thing) };
    println!("bytes = {bytes:?}");
}

fn main() {
    foo(MaybeUninit::zeroed())
}
bytes = [0, 86, 0, 0]

This happens, because MaybeUninit<(u8, u16)> has the same ABI as (u8, u16): ScalarPair (and it must have, per the docs). This causes the u8 and u16 to be passes as two individual function args and "forgets" the padding byte.

@5225225
Copy link
Contributor

5225225 commented May 15, 2023

Looks like a side effect of #99604

This is... not easy to fix.

@saethlin
Copy link
Member

saethlin commented May 15, 2023

Since the problem in these examples is the behavior of copying MaybeUninit and not the value returned by MaybeUninit::zeroed, should this be closed?

@quaternic
Copy link

quaternic commented May 15, 2023

MaybeUninit<T> should behave like an untyped bag of bytes that happens to have the same size and align as T. In particular, it should not have padding.

Why? I don't see anything in the docs of MaybeUninit<T> that would require copying padding bytes of T. In contrast, ptr::copy is explicitly an untyped copy that will copy padding bytes:

// this is currently not overwriting the second byte
pub unsafe fn write_uninit(x: *mut MaybeUninit<(u8, u16)>) {
    let zeroed = [0u8; 4];
    *x = *(&zeroed as *const _ as *const _);
}
// this does zero all four
pub unsafe fn ptr_copy_uninit(x: *mut MaybeUninit<(u8, u16)>) {
    let zeroed = [0u8; 4];
    ptr::copy::<MaybeUninit<(u8, u16)>>(&zeroed as *const _ as *const _, x, 1);
}

edit: playground (the code above has UB due to alignment, fixed in the linked code, but it didn't affect the results)

@lukas-code
Copy link
Member Author

Since the problem in these examples is the behavior of copying MaybeUninit and not the value returned by MaybeUninit::zeroed, should this be closed?

Yeah the problem here is not MaybeUninit::zeroed, but that MaybeUninit<T> currently does have the padding of T. Whether that's desirable is a different question, but I'll close this as duplicate of #99604.

@lukas-code lukas-code closed this as not planned Won't fix, can't repro, duplicate, stale May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

5 participants