Skip to content

Compiler isn't trusting programmer in unsafe code #14903

@engstad

Description

@engstad

With the recent transmute changes, several safe cases are now reported as errors. The most trivial is:

unsafe fn f<T>(i: T) ->T { std::mem::transmute(i) }

Another is:

struct Plane<T> { values : [T, ..4] }
struct Vec4<T>  { values : [T, ..4] }
impl<T> Plane<T> {
   unsafe fn to_vec4(&self) -> Vec4<T> { 
       std::mem::transmute(self.values) 
   }
}

There are obviously workarounds, but these are examples that are safe where the type-checker gets in the way. It is also unfortunate that the code is marked as "unsafe", yet the compiler rejects the code.

Activity

emberian

emberian commented on Jun 14, 2014

@emberian
Member

"unsafe" doesn't mean "anything goes". You can't call methods on types which have no method with that name, for example, or create a trait object out of a type which doens't implement the trait. This is a deliberate result of #14859 and #12898, closing as wontfix.

emberian

emberian commented on Jun 14, 2014

@emberian
Member

(Sorry)

emberian

emberian commented on Jun 14, 2014

@emberian
Member

IRC: "okay then, then I think the core team needs to re-consider what they want with the language. in essence, things like this will send the message that "the compiler knows better", which will result in c/c++ people to not use the language."

emberian

emberian commented on Jun 14, 2014

@emberian
Member

Reopening for discussion at a meeting, but I recommend wontfix/notabug.

emberian

emberian commented on Jun 14, 2014

@emberian
Member

In particular, this just revolves around the new restriction on the transmute intrinsic.

emberian

emberian commented on Jun 14, 2014

@emberian
Member

A function which does the same thing as the old transmute, although doesn't care about same-sizedness:

use std::{mem, ptr};

unsafe fn transmute_harder<T, U>(from: T) -> U {
    let mut to: U = mem::uninitialized();
    // copy_memory and forget can't trigger failure, so `from` and `to` won't have destructor run
    // extraneously
    ptr::copy_memory(&mut to as *mut U, &from as *_ as *U, 1);
    mem::forget(from);
    to
}

struct Plane<T> {
    data: [T, ..4]
}

struct Vec4<T> {
    data: [T, ..4]
}

fn main() {
    let plane = Plane { data: [1i, 2, 3, 4] };
    let vec: Vec4<int> = unsafe { transmute_harder(plane) };
    println!("{}", vec.data.as_slice());
}
huonw

huonw commented on Jun 14, 2014

@huonw
Member

(You can use ptr::read rather than explicit copy_memory too.)

eddyb

eddyb commented on Jun 14, 2014

@eddyb
Member

This exposed a more important issue: we don't emphasize enough that unsafe shouldn't be required for performance in many cases, and that premature micro-optimization should be avoided.
I am glad for this change, as it removes many misuses of transmute.

Just to point out how bad transmuting can be, I've done a comparison, for this specific example.
Even with --opt-level=3, it's still inferior to the safe code (the only remaining difference being the tail modifier on the call to memcpy.

engstad

engstad commented on Jun 15, 2014

@engstad
Author

Here's another example that now fails, in gl-rs. It was fixed by this commit: rust-windowing/gl-rs@b01df8e

pub struct FnPtr<F> { f: F, is_loaded: bool }

impl<F> FnPtr<F> {
    pub fn new(ptr: Option<extern "system" fn()>, failing_fn: F) -> FnPtr<F> {
        use std::mem::transmute;
        match ptr {
            std::option::Some(p) => FnPtr { f: unsafe { transmute(p) }, is_loaded: true },
            None => FnPtr { f: failing_fn, is_loaded: false },
        }
    }
}

The idea here is to trampoline to the C-function if it exists, and to call the fail function if it doesn't.

eddyb

eddyb commented on Jun 15, 2014

@eddyb
Member

In that case, FnPtr is too generic, under VG it would be:

pub struct FnPtr<..A, R> {
    f: extern "system" fn(..A) -> R,
    is_loaded: bool
}

A better (short term) solution would be to expand FnPtr::new for each load_with:

// now
pub fn load_with(loadfn: |symbol: &str| -> Option<extern "system" fn()>) {
    unsafe { ::storage::$name = ::FnPtr::new(loadfn($sym), ::failing::$name) }
}
// after
pub fn load_with(loadfn: |symbol: &str| -> Option<extern "system" fn()>) {
    match loadfn($sym) {
        std::option::Some(f) => unsafe {
            ::storage::$name = ::FnPtr { f: std::mem::transmute(f), is_loaded: true }
        },
        None => {}
    }
}
pnkfelix

pnkfelix commented on Jun 15, 2014

@pnkfelix
Member

I have to admit, the Plane<T> ←→ Vec4<T> case does seem unfortunate.

#12898 did explicitly say that the plan would be to only permit transmutes of generics "through pointers" (which I interpret to mean "TypeExpr can only be transmuted behind a * or &"). Still, it seems like we could be more aggressive about recognizing that the two types have the same structure given that they are both applied to the same type T.

bluss

bluss commented on Jun 15, 2014

@bluss
Member

If you are talking workarounds, transmute_copy already exists, maybe it is good enough for this use case.

alexcrichton

alexcrichton commented on Jun 16, 2014

@alexcrichton
Member

Closing, as has been noted many times in this thread, this was an explicit decision and this is working as-is today.

I believe it is too soon (this only landed 3 days ago) to start revisiting our initial decision. With this patch we have achieved the point where it is impossible to fail in codegen due to upstream crates. It is only possible to fail for a deterministically known issue in the local crate.

It would be possible to extend the language with a compiler-defined trait that signifies that two type parameters T and U are the same size, but this is part of the revisiting which can happen at a later date.

If there is a legitimate bug in the new transmute restrictions, then that is definitely something that should be dealt with. I would recommend opening a separate issue about perhaps adding a case of transmuting between the same type parameter, which is guaranteed to have the same size.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @alexcrichton@eddyb@pnkfelix@emberian@huonw

        Issue actions

          Compiler isn't trusting programmer in unsafe code · Issue #14903 · rust-lang/rust