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

Provided RW impl for fixed arrays. #95

Merged
merged 8 commits into from
Dec 28, 2023
Merged

Conversation

Frostie314159
Copy link
Contributor

This PR makes reading and writing possible for fixed length arrays of any type implementing TryFromCtx or TryIntoCtx.

@Frostie314159
Copy link
Contributor Author

I finally managed to implement this without the whole merge conflict fiasko and as a little bonus made it work for every type. I'm aware, that this uses unsafe, but I think my intuition, that unsafe is necessarily bad was wrong. If we just use it sparingly and with safety comments we should be good to go.

Copy link
Owner

@m4b m4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to address unsafe use of zeroed as per my comment

src/ctx.rs Outdated Show resolved Hide resolved
@m4b
Copy link
Owner

m4b commented Dec 18, 2023

I've never seen this error before, looks like transmute doesn't work with const generics, which is a bit sad:

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
   --> src/ctx.rs:815:26
    |
815 |             Ok((unsafe { std::mem::transmute::<_, Self>(buf) }, offset))
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: source type: `[MaybeUninit<T>; N]` (this type does not have a fixed size)
    = note: target type: `[T; N]` (this type does not have a fixed size)

@m4b
Copy link
Owner

m4b commented Dec 18, 2023

btw i've been testing with this code, helpful to see where drops occur:

#[derive(Debug)]
struct StrDrop<'a> {
    #[allow(unused)]
    s: &'a str,
}

impl<'a> scroll::ctx::TryFromCtx<'a, scroll::Endian> for StrDrop<'a> {
    type Error = scroll::Error;
    fn try_from_ctx(
        bytes: &'a [u8],
        _ctx: scroll::Endian,
    ) -> ::std::result::Result<(Self, usize), Self::Error> {
        let mut offset = 0;
        let s = bytes.gread(&mut offset)?;
        Ok((Self { s }, offset))
    }
}

impl<'a> Drop for StrDrop<'a> {
    fn drop(&mut self) {
        println!("Dropping {self:?}");
    }
}

#[derive(Debug)]
struct StringDrop {
    #[allow(unused)]
    s: String,
}

impl<'a> scroll::ctx::TryFromCtx<'a, scroll::Endian> for StringDrop {
    type Error = scroll::Error;
    fn try_from_ctx(
        bytes: &'a [u8],
        _ctx: scroll::Endian,
    ) -> ::std::result::Result<(Self, usize), Self::Error> {
        let mut offset = 0;
        let s: &str = bytes.gread(&mut offset)?;
        Ok((Self { s: String::from(s) }, offset))
    }
}

impl Drop for StringDrop {
    fn drop(&mut self) {
        println!("Dropping {self:?}");
        drop(&mut self.s);
    }
}

#[test]
fn test_fixed_array_str() {
    use scroll::Pread;
    let bytes = Box::new([0x45, 0x42, 0x0, 0x45, 0x41]);
    let res = bytes.pread_with::<[StrDrop; 3]>(0, Default::default());
    println!("{res:?}");
    assert!(false);
}

#[test]
fn test_fixed_array_string() {
    use scroll::Pread;
    let bytes = Box::new([0x45, 0x42, 0x0, 0x45, 0x41, 0x0]);
    let res = bytes.pread_with::<[StringDrop; 3]>(0, Default::default());
    println!("{res:?}");
    assert!(false);
}

what bothers me is why the [StringDrop; 3] is not an erro, but [StrDrop; 2] is; need to figure out what's going on there, probably related to the slice logic that changed slightly for pread

@Frostie314159
Copy link
Contributor Author

I also found a thread, which discusses the problem with transmute.

@Frostie314159
Copy link
Contributor Author

Frostie314159 commented Dec 18, 2023

I think I found the error, the byte array for text_fixed_array_string has one byte too much, which caused it not to error.

tests/api.rs Show resolved Hide resolved
tests/api.rs Outdated Show resolved Hide resolved
tests/api.rs Outdated Show resolved Hide resolved
@m4b m4b merged commit 8778f10 into m4b:master Dec 28, 2023
6 checks passed
@m4b
Copy link
Owner

m4b commented Dec 28, 2023

thank you for your patience @Frostie314159 , and thanks for the PR!

@Frostie314159
Copy link
Contributor Author

No problem, the patience was definitely worth it, since we now have a sound implementation.

@Frostie314159
Copy link
Contributor Author

@m4b I just noticed, in one of my projects, that LLVM fails to optimize this properly, when writing byte arrays. We should probably add a warning about that, since it crippled the performance in my case.

@m4b
Copy link
Owner

m4b commented Aug 12, 2024

@Frostie314159 thanks for noting this; there is another issue about llvm failing to optimize before, I wonder if it's related? let me find it; is it worth investigating filing this upstream to rust about perf issue ?

@m4b
Copy link
Owner

m4b commented Aug 12, 2024

Here was the issue, I don't know if it's relevant anymore, it was years ago: #53

can you paste the assembly if possible, or why you believe the fixed size array is failing to optimize well?

@Frostie314159
Copy link
Contributor Author

This does seem like a compiler bug, since the individual pwrites for every byte, just boil down to an assignment and all of the invariants of the unsafe code, can be validated at compile time. In practice, this should result in a memcpy. Theoretically, this could also be extended, to arrays of integers, with the same endianess as the target architecture. I would really love specialization.

@Frostie314159 Frostie314159 deleted the fixed-array-impl branch August 16, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants