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

Support AsBytes for fields with a size set as a generic constant #1182

Closed
Tracked by #671
saltatory opened this issue May 3, 2024 · 12 comments
Closed
Tracked by #671

Support AsBytes for fields with a size set as a generic constant #1182

saltatory opened this issue May 3, 2024 · 12 comments
Labels
blocking-next-release This issue should be resolved before we release on crates.io bug Something isn't working

Comments

@saltatory
Copy link

saltatory commented May 3, 2024

Use Case

I am working on a high-throughput IO project reading/writing fixed size blocks to disk. I have a broadly used compilation parameter for the block id size.

I have a struct like the following:

#[derive(FromBytes, FromZeroes, AsBytes, Unaligned)]
#[repr(packed)]
pub struct IndexEntryFlags(u8);

bitflags! {
    impl IndexEntryFlags: u8 {
        const NONE = 0b00;
        const DELETE = 0b01;
    }
}

#[derive(FromBytes, FromZeroes, AsBytes, Unaligned)]
#[repr(packed)]
pub struct IndexEntry<const SIZE_BLOCK_ID: usize> {
    block_number: U64,
    flags: IndexEntryFlags,
    block_id: [u8; SIZE_BLOCK_ID] 
}

Problem

This generates an error:

error[E0793]: reference to packed field is unaligned

... even though the exact same compilation works with:

...
   block_id: [u8; 32]
...

Request

Support generic constants in fixed size arrays for AsBytes.

@saltatory saltatory added the customer-request Documents customer requests. label May 3, 2024
@joshlf
Copy link
Member

joshlf commented May 3, 2024

Oh fascinating. I copied your code almost verbatim to the playground and didn't get this error. The only thing I had to change was replacing U64 with [u8; 8] (which is what U64 is under the hood) since the playground presumably doesn't compile with feature = "byteorder".

Are you able to repro this on the playground so that we can play around with it?

@saltatory
Copy link
Author

Sorry no. I tried several things including this.

  • I could not manage to find zerocopy::native_endian::U64.
  • I tried several ways to get the compiler to complain as it does on my machine ^^^
  • I assume that the U64 is/was causing the compiler to complain but I cannot be certain.
  • I had tried shifting to #[repr(C)] but got the error message about not accepting generics

@joshlf
Copy link
Member

joshlf commented May 4, 2024

Hmmm. I'll have chance to look at this more deeply later, but for the time being, maybe try with our latest alpha release? Total shot in the dark, but a lot has changed between 0.7 and 0.8, and maybe we accidentally fixed whatever the issue is.

@saltatory
Copy link
Author

Thanks for your quick and thoughtful response! I'll give that a rip. Likely tomorrow-ish.

@joshlf
Copy link
Member

joshlf commented May 4, 2024

One more thing: What toolchain are you using? It's possible that this is a new rustc bug.

joshlf added a commit that referenced this issue May 4, 2024
In #1182, a failure was reported for `AsBytes` (now renamed to
`IntoBytes`). We haven't yet been able to reproduce this failure, but
this commit adds that failure case verbatim so that we don't regress in
the future.
@joshlf
Copy link
Member

joshlf commented May 4, 2024

I've added a new test case for this so at a minimum we hopefully won't regress on it: #1186

@saltatory
Copy link
Author

I was on rustc 1.72.0 but I upgraded to test and had the same issue

rustup 1.27.0 (bbb9276d2 2024-03-08)
info: This is the version for the rustup toolchain manager, not the rustc compiler.
info: The currently active `rustc` version is `rustc 1.78.0 (9b00956e5 2024-04-29)`

@joshlf
Copy link
Member

joshlf commented May 4, 2024

Can you maybe paste the entire error you got, not just this single line?

error[E0793]: reference to packed field is unaligned

github-merge-queue bot pushed a commit that referenced this issue May 7, 2024
In #1182, a failure was reported for `AsBytes` (now renamed to
`IntoBytes`). We haven't yet been able to reproduce this failure, but
this commit adds that failure case verbatim so that we don't regress in
the future.
@saltatory
Copy link
Author

Sorry for the delay ... I had moved on in my project and now I cannot repro the error! I swear I was not making it up! Today, with the same computer/toolchain, I created the following test and it compiled fine.

use bitflags::bitflags;
use zerocopy::{AsBytes, FromBytes, FromZeroes, Unaligned};
use zerocopy::big_endian::U64;

#[derive(FromBytes, AsBytes, FromZeroes, Unaligned)]
#[repr(packed)]
pub struct Bar(u8);
bitflags! {
    impl Bar: u8 {
        const NONE = 0;
    }
}

#[derive(FromBytes, AsBytes, FromZeroes, Unaligned)]
#[repr(packed)]
pub struct Foo<const C: usize> {
    data: [u8; C],
    bar: Bar,
    baz: U64
}

#[test]
pub fn t1() {
    let foo = Foo::<32>{ data: [0u8; 32], baz: U64::new(32), bar: Bar::NONE};
}

Thanks sincerely for your help. Closing for now.

@saltatory
Copy link
Author

Apologies for the long delay. I re-ran into this issue. It's real. As I think you can see,

Sample Code

use zerocopy::{AsBytes, FromBytes, FromZeroes, Unaligned};

#[derive(FromBytes, AsBytes, FromZeroes, Unaligned)]
#[repr(packed)]
struct Foo<const SIZE: usize> {

    data: [u8; SIZE]

}

impl<const SIZE: usize> Foo<SIZE> {

    pub fn set_data(&mut self, data: &[u8]) {
        self.data.copy_from_slice(data);
    }

}

#[derive(FromBytes, AsBytes, FromZeroes)]
#[repr(C)]
struct Bar<const SIZE: usize> {

    data: [u8; SIZE]

}

impl<const SIZE: usize> Bar<SIZE> {

    pub fn set_data(&mut self, data: &[u8]) {
        self.data.copy_from_slice(data);
    }

}

#[derive(FromBytes, AsBytes, FromZeroes, Unaligned)]
#[repr(packed)]
struct Baz {

    data: [u8; 32]

}

impl Baz {

    pub fn set_data(&mut self, data: &[u8]) {
        self.data.copy_from_slice(data);
    }

}

Error

   Compiling zc-error v0.1.0 (/mnt/workspace/zc-error)
error: unsupported on generic structs that are not repr(transparent) or repr(packed)
  --> src/lib.rs:19:21
   |
19 | #[derive(FromBytes, AsBytes, FromZeroes)]
   |                     ^^^^^^^
   |
   = note: this error originates in the derive macro `AsBytes` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0793]: reference to packed field is unaligned
  --> src/lib.rs:14:9
   |
14 |         self.data.copy_from_slice(data);
   |         ^^^^^^^^^
   |
   = note: packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses
   = note: creating a misaligned reference is undefined behavior (even if that reference is never dereferenced)
   = help: copy the field contents to a local variable, or replace the reference with a raw pointer and use `read_unaligned`/`write_unaligned` (loads and stores via `*p` must be properly aligned even when using raw pointers)

For more information about this error, try `rustc --explain E0793`.
error: could not compile `zc-error` (lib) due to 2 previous errors

@saltatory saltatory reopened this May 31, 2024
@joshlf
Copy link
Member

joshlf commented May 31, 2024

Okay, that repro's for me on the Rust Playground. Thanks! Just a heads up, there's a good chance we won't be able to put time into this until at least late June, but we'll make sure to get to it eventually.

@joshlf joshlf added bug Something isn't working blocking-next-release This issue should be resolved before we release on crates.io and removed customer-request Documents customer requests. labels May 31, 2024
@joshlf joshlf mentioned this issue May 31, 2024
63 tasks
@joshlf
Copy link
Member

joshlf commented Jul 1, 2024

Hey, I discussed this with @jswrenn and realized that this is actually a fundamental limitation in zerocopy-derive. In particular, with just repr(C), the const parameter can affect whether or not the type has padding. Our padding type is unable to handle generics, including const generics. Unfortunately we can't support this without changes from Rust itself.

@joshlf joshlf closed this as completed Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking-next-release This issue should be resolved before we release on crates.io bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants