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

__IncompleteArrayField shouldn't implement Clone / Copy. #1431

Closed
Stargateur opened this issue Oct 30, 2018 · 9 comments · Fixed by #1664
Closed

__IncompleteArrayField shouldn't implement Clone / Copy. #1431

Stargateur opened this issue Oct 30, 2018 · 9 comments · Fixed by #1664

Comments

@Stargateur
Copy link

Stargateur commented Oct 30, 2018

struct fam {
    size_t len;
    int data[];
};
    let bindings = bindgen::Builder::default()
        .header("fam.h")
        .generate()
        .expect("Unable to generate bindings");
#[repr(C)]
#[derive(Default)]
pub struct __IncompleteArrayField<T>(::std::marker::PhantomData<T>);
impl<T> __IncompleteArrayField<T> {
    #[inline]
    pub fn new() -> Self {
        __IncompleteArrayField(::std::marker::PhantomData)
    }
    #[inline]
    pub unsafe fn as_ptr(&self) -> *const T {
        ::std::mem::transmute(self)
    }
    #[inline]
    pub unsafe fn as_mut_ptr(&mut self) -> *mut T {
        ::std::mem::transmute(self)
    }
    #[inline]
    pub unsafe fn as_slice(&self, len: usize) -> &[T] {
        ::std::slice::from_raw_parts(self.as_ptr(), len)
    }
    #[inline]
    pub unsafe fn as_mut_slice(&mut self, len: usize) -> &mut [T] {
        ::std::slice::from_raw_parts_mut(self.as_mut_ptr(), len)
    }
}
impl<T> ::std::fmt::Debug for __IncompleteArrayField<T> {
    fn fmt(&self, fmt: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result {
        fmt.write_str("__IncompleteArrayField")
    }
}
impl<T> ::std::clone::Clone for __IncompleteArrayField<T> {
    #[inline]
    fn clone(&self) -> Self {
        Self::new()
    }
}
impl<T> ::std::marker::Copy for __IncompleteArrayField<T> {}

#[repr(C)]
#[derive(Debug)]
pub struct fam {
    pub len: usize,
    pub data: __IncompleteArrayField<::std::os::raw::c_int>,
}

I wonder if it's not a bug or something because you can't copy a FAM like that. This is not a pointer. As I expected this produce complete undefined behavior when you use Copy or Clone trait.

#[no_mangle]
pub fn print_data(fam: &fam) {
    let data = fam.data.clone();
    let data = unsafe { data.as_slice(fam.len) };
    for x in data {
        println!("{:?}", x);
    }
}

Fail completely. Or I missing something ?

@emilio
Copy link
Contributor

emilio commented Oct 30, 2018

Well, it's using the same semantics as C, bindgen doesn't know about whether it's a variable length array, or just a separator so bitfields pack differently, or something else. The following C code compiles also without problems:

struct fam {
    unsigned len;
    int data[];
};

void use(const struct fam* unused) {};
void copy(const struct fam* fam)
{
  struct fam cp = *fam;
  use(&cp);
}

In general bindgen tries not to make assumptions about your C code.

@emilio
Copy link
Contributor

emilio commented Oct 30, 2018

More to the point, there's nothing bindgen can do about this particular case if you move your array, which will effectively not copy the extra space. And there's no way to make a type immovable itself, though I believe in newer rust versions you can use Pin.

So even if we didn't derive Copy or Clone, you could always move the value out and it'd be unsound anyway.

@emilio emilio closed this as completed Oct 30, 2018
@Stargateur
Copy link
Author

Stargateur commented Oct 30, 2018

@emilio I think you totally miss the point, struct fam cp = *fam; copy only the part of the struct that is not a FAM, that not what you introduce by implementing copy and clone on the FAM, you are doing something that don't make sense in C, you can't copy an FAM, that totally different of struct fam cp = *fam; you are doing something like that magic cp = fam.data, that simply don't make sense and will not compile on any C implementation.

@emilio emilio reopened this Oct 30, 2018
@emilio
Copy link
Contributor

emilio commented Oct 30, 2018

Oh, I see what you mean. Yeah, I agree copying just the data member doesn't really make sense. And apparently we are already smart enough to prevent deriving Copy / Clone on the structs that contain it.

I agree we should probably fix that. It's trivial, assuming nothing depends on it.

@emilio emilio changed the title Why flexible member array implement copy and clone ? __IncompleteArrayField shouldn't implement Clone / Copy. Oct 30, 2018
@highfive
Copy link

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@emilio
Copy link
Contributor

emilio commented Oct 30, 2018

Sorry for the initial misunderstanding @Stargateur, I totally misread your example :)

@honggoff
Copy link
Contributor

honggoff commented Nov 3, 2019

I'd like to take this issue to get familiar with the project.
@highfive: assign me please.

@highfive
Copy link

highfive commented Nov 3, 2019

Hey @honggoff! Thanks for your interest in working on this issue. It's now assigned to you!

honggoff added a commit to honggoff/rust-bindgen that referenced this issue Nov 3, 2019
Flexible array members are represented in the generated binding by a
struct __IncompleteArrayField<T>. Since such members do not contain any
information about how big they are, it is impossible to automatically
clone or copy them, either in C or rust.

Fixes rust-lang#1431.
@honggoff
Copy link
Contributor

honggoff commented Nov 3, 2019

It looks like the implementation for Copy has already been removed with #1477. I will remove the remaining implementation for Clone.

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

Successfully merging a pull request may close this issue.

4 participants