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

E0587 error on packed and aligned structures from C #1538

Open
ahomescu opened this issue Mar 13, 2019 · 10 comments
Open

E0587 error on packed and aligned structures from C #1538

ahomescu opened this issue Mar 13, 2019 · 10 comments

Comments

@ahomescu
Copy link

ahomescu commented Mar 13, 2019

I ran into the following issue when converting some kernel headers with bindgen 0.48.1:

When converting C structures that are both packed and aligned using either C2Rust or bindgen, such as:

struct xregs_state {
	struct fxregs_state		i387;
	struct xstate_header		header;
	u8				extended_state_area[0];
} __attribute__ ((packed, aligned (64)));

the code that either tool produces looks something like:

#[repr(C, packed(64))]
#[repr(align(64))]
pub struct xregs_state {
    pub i387: fxregs_state,
    pub header: xstate_header,
    pub extended_state_area: __IncompleteArrayField<u8>,
}

This Rust code fails to compile due to error E0587:

error[E0587]: type has conflicting packed and align representation hints
    --> .../out/bindings.rs:3894:1
     |
3894 | / pub struct xregs_state {
3895 | |     pub i387: fxregs_state,
3896 | |     pub header: xstate_header,
3897 | |     pub extended_state_area: __IncompleteArrayField<u8>,
3898 | | }
     | |_^

We can work around this in C2Rust by emitting an aligned outer/packed inner structure pair (I think bindgen could do the same), but I'm wondering if it would be better to fix this on the Rust language/compiler side (I also opened rust-lang/rust#59154 for a discussion on this).

@emilio
Copy link
Contributor

emilio commented Mar 13, 2019

I ran into the following issue when converting some kernel headers with bindgen 0.48.3:

I assume you mean 0.47.3?

We can work around this in C2Rust by emitting an aligned outer/packed inner structure pair (I think bindgen could do the same), but I'm wondering if it would be better to fix this on the Rust language/compiler side (I also opened rust-lang/rust#59154 for a discussion on this).

Can you put an example? I assume the workaround would look like:

#[repr(align(64))]
pub struct xregs_state(pub xreg_state_inner);

#[repr(packed)]
pub struct xreg_state_inner {
    // fields
}

Is that right? If so, that's feasible. But it only solves this very specific case, which is slightly unfortunate.

I haven't dug deep into how to properly generate rust code for aligned structs inside packed structs and such. That's something that rustc errors on, but C compilers happily allow it without issues.

Actually, for this test-case, it seems we're being naive, and we could just generate #[repr(packed(64)], it's the whole purpose of that. So that's an even better fix for this that I'm happy to write.

@emilio emilio closed this as completed Mar 13, 2019
@emilio
Copy link
Contributor

emilio commented Mar 13, 2019

Err, wrong button of course.

@emilio
Copy link
Contributor

emilio commented Mar 13, 2019

Ah, I was wrong. #[repr(packed(64)] doesn't generate the expected output, since it doesn't affect the size of the struct.

@emilio
Copy link
Contributor

emilio commented Mar 13, 2019

There's a further complication, which is that there's no good way I know of distinguishing this case from #pragma packed(N) using libclang...

@emilio
Copy link
Contributor

emilio commented Mar 13, 2019

I submitted https://reviews.llvm.org/D59299 to be able to distinguish that case, eventually.

@alex
Copy link
Member

alex commented Mar 13, 2019

Woo, thanks for jumping on this.

@ahomescu
Copy link
Author

I assume you mean 0.47.3?

Sorry, I meant 0.48.1 (the latest version).

@ahomescu
Copy link
Author

Can you put an example? I assume the workaround would look like:

#[repr(align(64))]
pub struct xregs_state(pub xreg_state_inner);

#[repr(packed)]
pub struct xreg_state_inner {
    // fields
}

Yes, that's basically what we had in mind, but I'm hoping that this can be fixed in rustc instead.

@ahomescu
Copy link
Author

ahomescu commented Mar 13, 2019

Worth mentioning: 0.47.3 generates code that compiles just fine because the definition for that structure is:

#[repr(C)]
pub struct xregs_state {
    pub _bindgen_opaque_blob: [u8; 576usize],
}

This is only a problem on newer bindgen because it now generates the full definition for the structure.

@emilio
Copy link
Contributor

emilio commented Mar 13, 2019

Well, that's not fine (as in, it doesn't have the right alignment), but as I said you can go back to that (and with the right alignment thanks to #1530) using the opaque_type / --opaque-type feature.

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

No branches or pull requests

3 participants